xfs
[Top] [All Lists]

Re: [PATCH V2] mkfs: handle 4k sector devices more cleanly

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH V2] mkfs: handle 4k sector devices more cleanly
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Fri, 08 Jan 2010 11:51:56 -0600
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <20100108174400.GA17634@xxxxxxxxxxxxx>
References: <4B1E9A25.50108@xxxxxxxxxx> <4B476171.4020701@xxxxxxxxxxx> <20100108174400.GA17634@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.23 (Macintosh/20090812)
Christoph Hellwig wrote:
> On Fri, Jan 08, 2010 at 10:46:41AM -0600, Eric Sandeen wrote:
>> +extern void platform_findsizes (char *path, int fd, long long *sz, int 
>> *bsz);
> 
> Can you move the prototype from libxfs/init.h to include/libxfs.h
> instead of adding it to the .c file?

sure, heh, you know - I knew that was a bad idea ;)

>> +    /*
>> +     * MD wants sector size set == block size to avoid switching.
>> +     * Otherwise, if not specfied via command, use device sectorsize
>> +     */
>> +    if (ft.sectoralign || !ssflag) {
>> +            if (ft.sectoralign)
>> +                    sectorsize = blocksize;
>> +            else
>> +                    sectorsize = ft.sectorsize;
> 
> This still confuses the heck out of me. What do you think about the
> incremental patch at the end of the mail?

yeah, that seems good.

>>      if (slflag || ssflag)
>>              xi.setblksize = sectorsize;
>> -    else
>> -            xi.setblksize = 1;
> 
> So for the defaul case we now never set the sector size in the libxfs
> init.  This seems safe to me, but why did we do it before?  Could
> a previous user have left it set to a wrong value?
> 
> Maye we should just do the xi.setblksize = sectorsize unconditionally?

ugh this stuff is messy.

let me see what sectorsize's default is...

If we take out the "1 is special" stuff I should probably chase it through
all the code.

Ok, will give this one more crack :)

Thanks,
-Eric

> 
> Index: xfsprogs-dev/mkfs/xfs_mkfs.c
> ===================================================================
> --- xfsprogs-dev.orig/mkfs/xfs_mkfs.c 2010-01-08 18:33:53.619277529 +0100
> +++ xfsprogs-dev/mkfs/xfs_mkfs.c      2010-01-08 18:39:37.758005711 +0100
> @@ -1561,21 +1561,32 @@ main(
>       memset(&ft, 0, sizeof(ft));
>       get_topology(&xi, &ft);
>  
> -     /*
> -      * MD wants sector size set == block size to avoid switching.
> -      * Otherwise, if not specfied via command, use device sectorsize
> -      */
> +     if (ft.sectoralign) {
> +             /*
> +              * Older Linux software RAID versions want the sector size
> +              * to match the block size to avoid switching I/O sizes.
> +              * For the legacy libdisk case we thus set the sector size to
> +              * match the block size.  For systems using libblkid we assume
> +              * that the kernel is recent enough to not require this and
> +              * ft.sectoralign will never be set.
> +              */
> +             sectorsize = blocksize;
> +     } else if (!ssflag) {
> +             /*
> +              * Unless specified manually on the command line use the
> +              * advertised sector size of the device.
> +              */
> +             sectorsize = ft.sectorsize;
> +     }
> +
>       if (ft.sectoralign || !ssflag) {
> -             if (ft.sectoralign)
> -                     sectorsize = blocksize;
> -             else
> -                     sectorsize = ft.sectorsize;
>               sectorlog = libxfs_highbit32(sectorsize);
>               if (loginternal) {
>                       lsectorsize = sectorsize;
>                       lsectorlog = sectorlog;
>               }
>       }
> +
>       if (sectorsize < XFS_MIN_SECTORSIZE ||
>           sectorsize > XFS_MAX_SECTORSIZE || sectorsize > blocksize) {
>               fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
> 

<Prev in Thread] Current Thread [Next in Thread>