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?

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);
> 

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