mkfs: handle 4k sector devices more cleanly

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 <[email protected]>

---

libxfs/linux.c +0 -0

index bc49903..2e07d54 100644

Quoted 24 lines · 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 <[email protected]>

---

libxfs/linux.c +0 -0

index bc49903..2e07d54 100644

libxfs/linux.c platform_set_blocksize(int fd, char *path, dev_t device, int blocksize, int fata · line 117

Quoted 8 lines

libxfs/linux.c +2 -2

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

libxfs/linux.c static void blkid_get_topology(const char *device, int *sunit, int *swidth) · line 354

Quoted 13 lines

libxfs/linux.c +1 -1

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

libxfs/linux.c +3 -1

 	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.

libxfs/linux.c static void blkid_get_topology(const char *device, int *sunit, int *swidth) · line 355

Quoted 1 line

libxfs/linux.c +1 -0

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

libxfs/linux.c static void blkid_get_topology(const char *device, int *sunit, int *swidth)

Quoted 11 lines

libxfs/linux.c +9 -2

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

libxfs/linux.c

Quoted 6 lines

libxfs/linux.c +5 -0

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

Looks good.

Quoted 7 lines

libxfs/linux.c +0 -0

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

libxfs/linux.c main( · line 1767

Quoted 2 lines

libxfs/linux.c +1 -1

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

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

Quoted 1 line · Eric Sandeen

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

Quoted 4 lines

mkfs/xfs_mkfs.c +4 -0

+	/*
+	 * MD wants sector size set == block size to avoid switching.
+	 * Otherwise, if not specfied via command, use device sectorsize
+	 */

Review

Quoted 5 lines

(quoted diff) +5 -0

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

Review

Quoted 4 lines

(quoted diff) +0 -2

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

mkfs/xfs_mkfs.c +18 -8

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

Quoted 1 line · Eric Sandeen

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

Review

Quoted 9 lines

(quoted diff) +9 -0

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

Review

Quoted 4 lines

(quoted diff) +0 -2

 	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?

Quoted 3 lines · Christoph Hellwig

Quoted 1 line · Eric Sandeen

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

Quoted 2 lines

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*

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

Quoted 1 line

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.