On Thu, Feb 18, 2021 at 10:38:17AM -0600, Eric Sandeen wrote:

On 2/17/21 11:24 PM, Gao Xiang wrote:

...

since we have this check already in xfs_validate_stripe_geometry, it seems best to
keep using it there, and not copy it ... which I think you accomplish below.

quoted

quoted

btw, do we have some range test about these variables? I could rearrange the code
snippet, but I'm not sure if it could introduce some new potential regression as well...

Thanks,
Gao Xiang

Or how about applying the following incremental patch, although the maximum dswidth
would be smaller I think, but considering libxfs_validate_stripe_geometry() accepts
dswidth in 64-bit bytes as well. I think that would be fine. Does that make sense?

I've confirmed "# mkfs/mkfs.xfs -f -d su=4097,sw=1 /dev/loop0" now report:
stripe unit (4097) must be a multiple of the sector size (512)

and xfs/191-input-validation passes now...

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f152d5c7..80405790 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c

@@ -2361,20 +2361,24 @@ _("both data su and data sw options must be specified\n"));
 			usage();
 		}

Just thinking through this... I think this is the right idea.

quoted

-		dsunit  = (int)BTOBBT(dsu);
-		big_dswidth = (long long int)dsunit * dsw;
+		big_dswidth = (long long int)dsu * dsw;

dsu is in bytes; this would mean big_dswidth is now also in bytes...
the original goal here, I think, is to not overflow the 32-bit superblock value
for dswidth.

Yeah, agreed. Thanks for catching this.

quoted

 		if (big_dswidth > INT_MAX) {
 			fprintf(stderr,
 _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
 				big_dswidth, dsunit);

so this used to test big_dswidth in BB (sectors); but now it tests in bytes.

Perhaps this should change to check and report sectors again:

  		if (BTOBBT(big_dswidth) > INT_MAX) {
  			fprintf(stderr,
  _("data stripe width (%lld) is too large of a multiple of the data stripe unit (%d)\n"),
  				BTOBBT(big_dswidth), dsunit);

I think the goal is to not overflow the 32-bit on-disk values, which would be
easy to do with "dsw" specified as a /multiplier/ of "dsu"

So I think that if we keep range checking the value in BB units, it will be
OK.

quoted

 			usage();
 		}
-		dswidth = big_dswidth;
-	}
 
-	if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit), BBTOB(dswidth),
-					     cfg->sectorsize, false))
+		if (!libxfs_validate_stripe_geometry(NULL, dsu, big_dswidth,
+						     cfg->sectorsize, false))
+			usage();
+
+		dsunit = BTOBBT(dsu);
+		dswidth = BTOBBT(big_dswidth);
+	} else if (!libxfs_validate_stripe_geometry(NULL, BBTOB(dsunit),
+			BBTOB(dswidth), cfg->sectorsize, false)) {
 		usage();
+	}

Otherwise this looks reasonable to me; now it's basically:

1) If we got geometry in bytes, validate them directly
2) If we got geometry in BB, convert to bytes, and validate
3) If we got no geometry, validate the device-reported defaults

Ok, let me send the next version.

Thanks,
Gao Xiang

Thanks,
-Eric

quoted

 	/* If sunit & swidth were manually specified as 0, same as noalign */
 	if ((cli_opt_set(&dopts, D_SUNIT) || cli_opt_set(&dopts, D_SU)) &&