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

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

[Eric Sandeen]

Trying to mkfs a 4k sector device today fails w/o manually specifying
sector size:

# modprobe scsi_debug sector_size=4096 dev_size_mb=32
# mkfs.xfs -f /dev/sdc
mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid argument
Warning: the data subvolume sector size 512 is less than the sector size 
reported by the device (4096).
... <fail>

add sectorsize to the device topology info, and use that if present.

Also check that explicitly requested sector sizes are not smaller
than the hardware size.  This already fails today, but with the more
cryptic "cannot set blocksize" ioctl error above.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libxfs/linux.c b/libxfs/linux.c
index bc49903..2e07d54 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
 	if (major(device) != RAMDISK_MAJOR) {
 		if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
 			fprintf(stderr, _("%s: %s - cannot set blocksize "
-					"on block device %s: %s\n"),
+					"%d on block device %s: %s\n"),
 				progname, fatal ? "error": "warning",
-				path, strerror(errno));
+				blocksize, path, strerror(errno));
 		}
 	}
 	return error;
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index bd92bc0..be8271e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -33,6 +33,7 @@ struct fs_topology {
 	int	dsunit;		/* stripe unit - data subvolume */
 	int	dswidth;	/* stripe width - data subvolume */
 	int	rtswidth;	/* stripe width - rt subvolume */
+	int	sectorsize;
 	int	sectoralign;
 };
 
@@ -320,7 +321,7 @@ out_free_probe:
 	return ret;
 }
 
-static void blkid_get_topology(const char *device, int *sunit, int *swidth)
+static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
 {
 	blkid_topology tp;
 	blkid_probe pr;
@@ -348,7 +349,9 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth)
 	val = blkid_topology_get_optimal_io_size(tp) >> 9;
 	if (val > 1)
 		*swidth = val;
-
+	val = blkid_probe_get_sectorsize(pr);
+	if (val > 1)
+		*sectorsize = val;
 	if (blkid_topology_get_alignment_offset(tp) != 0) {
 		fprintf(stderr,
 			_("warning: device is not properly aligned %s\n"),
@@ -370,13 +373,13 @@ static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
 	if (!xi->disfile) {
 		const char *dfile = xi->volname ? xi->volname : xi->dname;
 
-		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth);
+		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth, &ft->sectorsize);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int dummy;
 
-		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth);
+		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
 	}
 }
 #else /* ENABLE_BLKID */
@@ -1543,8 +1546,15 @@ main(
 	memset(&ft, 0, sizeof(ft));
 	get_topology(&xi, &ft);
 
-	if (ft.sectoralign) {
-		sectorsize = blocksize;
+	/*
+	 * 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;
 		sectorlog = libxfs_highbit32(sectorsize);
 		if (loginternal) {
 			lsectorsize = sectorsize;
@@ -1556,6 +1566,11 @@ main(
 		fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
 		usage();
 	}
+	if (sectorsize < ft.sectorsize) {
+		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
+			sectorsize, ft.sectorsize);
+		usage();
+	}
 	if (lsectorsize < XFS_MIN_SECTORSIZE ||
 	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
 		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
@@ -1749,10 +1764,10 @@ main(
 	calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
 				&dsunit, &dswidth, &lsunit);
 
-	if (slflag || ssflag)
+	if (slflag || ssflag || ft.setorsize)
 		xi.setblksize = sectorsize;
 	else
-		xi.setblksize = 1;
+		xi.setblksize = 1; /* Use default sector size */
 
 	/*
 	 * Initialize.  This will open the log and rt devices as well.

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

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

[Christoph Hellwig]

On Tue, Dec 08, 2009 at 12:25:41PM -0600, Eric Sandeen wrote:
> Trying to mkfs a 4k sector device today fails w/o manually specifying
> sector size:
> 
> # modprobe scsi_debug sector_size=4096 dev_size_mb=32
> # mkfs.xfs -f /dev/sdc
> mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid argument
> Warning: the data subvolume sector size 512 is less than the sector size 
> reported by the device (4096).
> ... <fail>
> 
> add sectorsize to the device topology info, and use that if present.
> 
> Also check that explicitly requested sector sizes are not smaller
> than the hardware size.  This already fails today, but with the more
> cryptic "cannot set blocksize" ioctl error above.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> diff --git a/libxfs/linux.c b/libxfs/linux.c
> index bc49903..2e07d54 100644
> --- a/libxfs/linux.c
> +++ b/libxfs/linux.c
> @@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
>  	if (major(device) != RAMDISK_MAJOR) {
>  		if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
>  			fprintf(stderr, _("%s: %s - cannot set blocksize "
> -					"on block device %s: %s\n"),
> +					"%d on block device %s: %s\n"),
>  				progname, fatal ? "error": "warning",
> -				path, strerror(errno));
> +				blocksize, path, strerror(errno));

Defintively a more useful error message than before, thanks.

> -static void blkid_get_topology(const char *device, int *sunit, int *swidth)
> +static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
>  {
>  	blkid_topology tp;
>  	blkid_probe pr;
> @@ -348,7 +349,9 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth)
>  	val = blkid_topology_get_optimal_io_size(tp) >> 9;
>  	if (val > 1)
>  		*swidth = val;
> -
> +	val = blkid_probe_get_sectorsize(pr);
> +	if (val > 1)
> +		*sectorsize = val;

I don't think the val > 1 check here makes any sense.

> +		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth, &ft->sectorsize);

The lines is growing a bit too long here..


Also I think you should add the sector size retrival by using the ioctl
directly for the non-blkid case to make sure the code doesn't have too
many corner cases.

> -	if (ft.sectoralign) {
> -		sectorsize = blocksize;
> +	/*
> +	 * 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;

The code looks good, but I don't think the comment helps understanding
what's going on.  And I might get a bit pendantic here, but changing it
to

	if (!ssflag || ft.sectoralign)

might make the intent a bit more clear.


> +	if (sectorsize < ft.sectorsize) {
> +		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
> +			sectorsize, ft.sectorsize);
> +		usage();
> +	}

Looks good.

>  	if (lsectorsize < XFS_MIN_SECTORSIZE ||
>  	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
>  		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
> @@ -1749,10 +1764,10 @@ main(
>  	calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
>  				&dsunit, &dswidth, &lsunit);
>  
> -	if (slflag || ssflag)
> +	if (slflag || ssflag || ft.setorsize)

There's a c missing here, this wouldn't even compile :)

It will also be always true for blkid builds which is at least a bit
confusing.  If you also updated the libdisk case as suggested above we
could also get rid of the xi.setblksize = 1 special case totally and
always pass the proper block size to libxfs_init and libxfs_device_open
(and make libxfs_device_open static in libxfs/init.c while we're at it
:))

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

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

[Eric Sandeen]

Christoph Hellwig wrote:
> On Tue, Dec 08, 2009 at 12:25:41PM -0600, Eric Sandeen wrote:
>> Trying to mkfs a 4k sector device today fails w/o manually specifying
>> sector size:
>>
>> # modprobe scsi_debug sector_size=4096 dev_size_mb=32
>> # mkfs.xfs -f /dev/sdc
>> mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid argument
>> Warning: the data subvolume sector size 512 is less than the sector size 
>> reported by the device (4096).
>> ... <fail>
>>
>> add sectorsize to the device topology info, and use that if present.
>>
>> Also check that explicitly requested sector sizes are not smaller
>> than the hardware size.  This already fails today, but with the more
>> cryptic "cannot set blocksize" ioctl error above.
>>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
>> ---
>>
>> diff --git a/libxfs/linux.c b/libxfs/linux.c
>> index bc49903..2e07d54 100644
>> --- a/libxfs/linux.c
>> +++ b/libxfs/linux.c
>> @@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
>>  	if (major(device) != RAMDISK_MAJOR) {
>>  		if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
>>  			fprintf(stderr, _("%s: %s - cannot set blocksize "
>> -					"on block device %s: %s\n"),
>> +					"%d on block device %s: %s\n"),
>>  				progname, fatal ? "error": "warning",
>> -				path, strerror(errno));
>> +				blocksize, path, strerror(errno));
> 
> Defintively a more useful error message than before, thanks.
> 
>> -static void blkid_get_topology(const char *device, int *sunit, int *swidth)
>> +static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
>>  {
>>  	blkid_topology tp;
>>  	blkid_probe pr;
>> @@ -348,7 +349,9 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth)
>>  	val = blkid_topology_get_optimal_io_size(tp) >> 9;
>>  	if (val > 1)
>>  		*swidth = val;
>> -
>> +	val = blkid_probe_get_sectorsize(pr);
>> +	if (val > 1)
>> +		*sectorsize = val;
> 
> I don't think the val > 1 check here makes any sense.

TBH it was a cut and paste from above, sigh.

I guess I don't know why the other one uses "1" either:

unsigned long blkid_topology_get_optimal_io_size(blkid_topology tp)
{
        return tp ? tp->optimal_io_size : 0;
}

anyway for this one it can be an unconditional assignment I guess.

>> +		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth, &ft->sectorsize);
> 
> The lines is growing a bit too long here..

yeah ....

> 
> Also I think you should add the sector size retrival by using the ioctl
> directly for the non-blkid case to make sure the code doesn't have too
> many corner cases.

Ok.  The problem is platform_get_blocksize or whatnot would
want both an fd and a path, and we've not opened anything yet :(

blkid was so handy :)

>> -	if (ft.sectoralign) {
>> -		sectorsize = blocksize;
>> +	/*
>> +	 * 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;
> 
> The code looks good, but I don't think the comment helps understanding
> what's going on.  And I might get a bit pendantic here, but changing it
> to
> 
> 	if (!ssflag || ft.sectoralign)
> 
> might make the intent a bit more clear.

ok. I can drop the comment, doesn't bother me.

> 
>> +	if (sectorsize < ft.sectorsize) {
>> +		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
>> +			sectorsize, ft.sectorsize);
>> +		usage();  
>> +	}
> 
> Looks good.
> 
>>  	if (lsectorsize < XFS_MIN_SECTORSIZE ||
>>  	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
>>  		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
>> @@ -1749,10 +1764,10 @@ main(
>>  	calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
>>  				&dsunit, &dswidth, &lsunit);
>>  
>> -	if (slflag || ssflag)
>> +	if (slflag || ssflag || ft.setorsize)
> 
> There's a c missing here, this wouldn't even compile :)

boooo for last-minute edits, sorry.

> It will also be always true for blkid builds which is at least a bit
> confusing.  If you also updated the libdisk case as suggested above we
> could also get rid of the xi.setblksize = 1 special case totally and
> always pass the proper block size to libxfs_init and libxfs_device_open
> (and make libxfs_device_open static in libxfs/init.c while we're at it
> :))

hm all good ideas, though need to think about how to do things
cleanly with platform_find_blah.

We already have:

void
platform_findsizes(char *path, int fd, long long *sz, int *bsz)

which would make it easy, but grumble, we don't have an fd yet!

I guess maybe I could make that function cope with a dummy fd,
and do the open/close itself....

Thanks,
-Eric

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

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

[Eric Sandeen]

Trying to mkfs a 4k sector device today fails w/o manually specifying
sector size:

# modprobe scsi_debug sector_size=4096 dev_size_mb=32
# mkfs.xfs -f /dev/sdc
mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid argument
Warning: the data subvolume sector size 512 is less than the sector size 
reported by the device (4096).
... <fail>

add sectorsize to the device topology info, and use that if present.

Also check that explicitly requested sector sizes are not smaller
than the hardware size.  This already fails today, but with the more
cryptic "cannot set blocksize" ioctl error above.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/libxfs/linux.c b/libxfs/linux.c
index bc49903..2e07d54 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
 	if (major(device) != RAMDISK_MAJOR) {
 		if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
 			fprintf(stderr, _("%s: %s - cannot set blocksize "
-					"on block device %s: %s\n"),
+					"%d on block device %s: %s\n"),
 				progname, fatal ? "error": "warning",
-				path, strerror(errno));
+				blocksize, path, strerror(errno));
 		}
 	}
 	return error;
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d3ed00a..356dcb4 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -33,6 +33,7 @@ struct fs_topology {
 	int	dsunit;		/* stripe unit - data subvolume */
 	int	dswidth;	/* stripe width - data subvolume */
 	int	rtswidth;	/* stripe width - rt subvolume */
+	int	sectorsize;
 	int	sectoralign;
 };
 
@@ -320,7 +321,7 @@ out_free_probe:
 	return ret;
 }
 
-static void blkid_get_topology(const char *device, int *sunit, int *swidth)
+static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
 {
 	blkid_topology tp;
 	blkid_probe pr;
@@ -348,6 +349,8 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth)
 	val = blkid_topology_get_optimal_io_size(tp) >> 9;
 	if (val > 1)
 		*swidth = val;
+	val = blkid_probe_get_sectorsize(pr);
+	*sectorsize = val;
 
 	if (blkid_topology_get_alignment_offset(tp) != 0) {
 		fprintf(stderr,
@@ -370,13 +373,14 @@ static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
 	if (!xi->disfile) {
 		const char *dfile = xi->volname ? xi->volname : xi->dname;
 
-		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth);
+		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
+				   &ft->sectorsize);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int dummy;
 
-		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth);
+		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
 	}
 }
 #else /* ENABLE_BLKID */
@@ -403,15 +407,29 @@ check_overwrite(
 	return 0;
 }
 
+extern void platform_findsizes (char *path, int fd, long long *sz, int *bsz);
+
 static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
 {
 	char *dfile = xi->volname ? xi->volname : xi->dname;
+	int bsz = BBSIZE;
 
 	if (!xi->disfile) {
+		int fd;
+		long long dummy;
+
 		get_subvol_stripe_wrapper(dfile, SVTYPE_DATA,
 				&ft->dsunit, &ft->dswidth, &ft->sectoralign);
+		fd = open(dfile, O_RDONLY);
+		/* If this fails we just fall back to BBSIZE */
+		if (fd) {
+			platform_findsizes(dfile, fd, &dummy, &bsz);
+			close(fd);
+		}
 	}
 
+	ft->sectorsize = bsz;
+
 	if (xi->rtname && !xi->risfile) {
 		int dummy1;
 
@@ -1543,8 +1561,15 @@ main(
 	memset(&ft, 0, sizeof(ft));
 	get_topology(&xi, &ft);
 
-	if (ft.sectoralign) {
-		sectorsize = blocksize;
+	/*
+	 * 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;
 		sectorlog = libxfs_highbit32(sectorsize);
 		if (loginternal) {
 			lsectorsize = sectorsize;
@@ -1556,6 +1581,11 @@ main(
 		fprintf(stderr, _("illegal sector size %d\n"), sectorsize);
 		usage();
 	}
+	if (sectorsize < ft.sectorsize) {
+		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
+			sectorsize, ft.sectorsize);
+		usage();
+	}
 	if (lsectorsize < XFS_MIN_SECTORSIZE ||
 	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
 		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
@@ -1751,8 +1781,6 @@ main(
 
 	if (slflag || ssflag)
 		xi.setblksize = sectorsize;
-	else
-		xi.setblksize = 1;
 
 	/*
 	 * Initialize.  This will open the log and rt devices as well.

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

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

[Christoph Hellwig]

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?

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

Maye we should just do the xi.setblksize = sectorsize unconditionally?


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

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

[Eric Sandeen]

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

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

[Eric Sandeen]

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

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

[Christoph Hellwig]

On Fri, Jan 08, 2010 at 08:24:00PM -0600, Eric Sandeen wrote:
> 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*

I'd just move the header around to keep it simple for now.

> 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 ...

Yes, now we do not set any size, while previously we always set
XFS_MIN_SECTORSIZE.  I don't think this actually changes anything - the
kernel doesn't make use of this blocksize information outside of
filesystems, and we already reset it on mount.

> > 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.

repair uses it in a weird way.

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

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

[Eric Sandeen]

Trying to mkfs a 4k sector device today fails w/o manually specifying
sector size:

# modprobe scsi_debug sector_size=4096 dev_size_mb=32
# mkfs.xfs -f /dev/sdc
mkfs.xfs: warning - cannot set blocksize on block device /dev/sdc: Invalid argument
Warning: the data subvolume sector size 512 is less than the sector size 
reported by the device (4096).
... <fail>

add sectorsize to the device topology info, and use that if present.

Also check that explicitly requested sector sizes are not smaller
than the hardware size.  This already fails today, but with the more
cryptic "cannot set blocksize" ioctl error above.

With a few more suggested comments & cleanups from Christoph.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/include/libxfs.h b/include/libxfs.h
index ab37a60..e7199c7 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -115,6 +115,7 @@ extern void	libxfs_device_zero (dev_t, xfs_daddr_t, uint);
 extern void	libxfs_device_close (dev_t);
 extern int	libxfs_device_alignment (void);
 extern void	libxfs_report(FILE *);
+extern void	platform_findsizes(char *path, int fd, long long *sz, int *bsz);
 
 /* check or write log footer: specify device, log size in blocks & uuid */
 typedef xfs_caddr_t (libxfs_get_block_t)(xfs_caddr_t, int, void *);
diff --git a/libxfs/init.h b/libxfs/init.h
index 1f27af6..f0b8cb6 100644
--- a/libxfs/init.h
+++ b/libxfs/init.h
@@ -24,7 +24,6 @@ extern int platform_check_ismounted (char *path, char *block,
 					struct stat64 *sptr, int verbose);
 extern int platform_check_iswritable (char *path, char *block,
 					struct stat64 *sptr, int fatal);
-extern void platform_findsizes (char *path, int fd, long long *sz, int *bsz);
 extern int platform_set_blocksize (int fd, char *path, dev_t device, int bsz, int fatal);
 extern void platform_flush_device (int fd, dev_t device);
 extern char *platform_findrawpath(char *path);
diff --git a/libxfs/linux.c b/libxfs/linux.c
index bc49903..2e07d54 100644
--- a/libxfs/linux.c
+++ b/libxfs/linux.c
@@ -112,9 +112,9 @@ platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata
 	if (major(device) != RAMDISK_MAJOR) {
 		if ((error = ioctl(fd, BLKBSZSET, &blocksize)) < 0) {
 			fprintf(stderr, _("%s: %s - cannot set blocksize "
-					"on block device %s: %s\n"),
+					"%d on block device %s: %s\n"),
 				progname, fatal ? "error": "warning",
-				path, strerror(errno));
+				blocksize, path, strerror(errno));
 		}
 	}
 	return error;
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index d3ed00a..64b324c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -33,6 +33,7 @@ struct fs_topology {
 	int	dsunit;		/* stripe unit - data subvolume */
 	int	dswidth;	/* stripe width - data subvolume */
 	int	rtswidth;	/* stripe width - rt subvolume */
+	int	sectorsize;
 	int	sectoralign;
 };
 
@@ -320,7 +321,7 @@ out_free_probe:
 	return ret;
 }
 
-static void blkid_get_topology(const char *device, int *sunit, int *swidth)
+static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
 {
 	blkid_topology tp;
 	blkid_probe pr;
@@ -348,6 +349,8 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth)
 	val = blkid_topology_get_optimal_io_size(tp) >> 9;
 	if (val > 1)
 		*swidth = val;
+	val = blkid_probe_get_sectorsize(pr);
+	*sectorsize = val;
 
 	if (blkid_topology_get_alignment_offset(tp) != 0) {
 		fprintf(stderr,
@@ -370,13 +373,14 @@ static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
 	if (!xi->disfile) {
 		const char *dfile = xi->volname ? xi->volname : xi->dname;
 
-		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth);
+		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
+				   &ft->sectorsize);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int dummy;
 
-		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth);
+		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
 	}
 }
 #else /* ENABLE_BLKID */
@@ -403,15 +407,29 @@ check_overwrite(
 	return 0;
 }
 
+extern void platform_findsizes (char *path, int fd, long long *sz, int *bsz);
+
 static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
 {
 	char *dfile = xi->volname ? xi->volname : xi->dname;
+	int bsz = BBSIZE;
 
 	if (!xi->disfile) {
+		int fd;
+		long long dummy;
+
 		get_subvol_stripe_wrapper(dfile, SVTYPE_DATA,
 				&ft->dsunit, &ft->dswidth, &ft->sectoralign);
+		fd = open(dfile, O_RDONLY);
+		/* If this fails we just fall back to BBSIZE */
+		if (fd) {
+			platform_findsizes(dfile, fd, &dummy, &bsz);
+			close(fd);
+		}
 	}
 
+	ft->sectorsize = bsz;
+
 	if (xi->rtname && !xi->risfile) {
 		int dummy1;
 
@@ -1544,18 +1562,41 @@ main(
 	get_topology(&xi, &ft);
 
 	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) {
 		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);
 		usage();
 	}
+	if (sectorsize < ft.sectorsize) {
+		fprintf(stderr, _("illegal sector size %d; hw sector is %d\n"),
+			sectorsize, ft.sectorsize);
+		usage();
+	}
 	if (lsectorsize < XFS_MIN_SECTORSIZE ||
 	    lsectorsize > XFS_MAX_SECTORSIZE || lsectorsize > blocksize) {
 		fprintf(stderr, _("illegal log sector size %d\n"), lsectorsize);
@@ -1749,10 +1790,7 @@ main(
 	calc_stripe_factors(dsu, dsw, sectorsize, lsu, lsectorsize,
 				&dsunit, &dswidth, &lsunit);
 
-	if (slflag || ssflag)
-		xi.setblksize = sectorsize;
-	else
-		xi.setblksize = 1;
+	xi.setblksize = sectorsize;
 
 	/*
 	 * Initialize.  This will open the log and rt devices as well.

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

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

[Christoph Hellwig]

Looks good,


Reviewed-by: Christoph Hellwig <hch@lst.de>

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