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

[Eric Sandeen <sandeen@sandeen.net>]

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?

actually it seems a little out of place in libxfs/init.h; that probably
works but there are no other platform_* functions there...

Should this be more like an xfs_findsizes in libxfs, which
calls platform_findsizes internally just for consistency? *shrug*

>> +	/*
>> +	 * 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?
> 
>>  	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?

ok so I read this wrong on my previous reply I guess.

The only way this is used is: it's sent to libxfs_init and then from there
only to libxfs_open which does:

        if (!readonly && setblksize && (statb.st_mode & S_IFMT) == S_IFBLK) {
                if (setblksize == 1)
                        /* use the default blocksize */
                        (void)platform_set_blocksize(fd, path, statb.st_rdev, XFS_MIN_SECTORSIZE, 0);
                else {
                        /* given an explicit blocksize to use */
                        if (platform_set_blocksize(fd, path, statb.st_rdev, setblksize, 1))
                            exit(1);
                }
        }

so "1" seems to have the special meaning of "use XFS_MIN_SECTORSIZE"

Hm, ok but in my patch setblksize is 0, so it's not getting set.
I suppose this -might- mess up other utils ...
 
> Maye we should just do the xi.setblksize = sectorsize unconditionally?

I think that's best.  It's already defaulted to XFS_MIN_SECTORSIZE
so should be no functional change if it doesn't get otherwise set - although
I think it -does- get set in all cases now - either from ft.sectoralign/blocksize,
from the explicit -s setting, or the ft.sectorsize by default.

What do you think about removing the "1" magic if so?  At that point I think
nothing relies on it.

Thanks,
-Eric

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs