Re: [PATCH 2/2] mkfs: unify validation behavior for data, log and rt dev

[Zorro Lang <zorro.lang@gmail.com>]

On Mon, Apr 06, 2026 at 08:37:26AM -0700, Darrick J. Wong wrote:
> On Sun, Apr 05, 2026 at 12:36:40AM +0800, Zorro Lang wrote:
> > The current validation logic in validate_datadev, validate_logdev,
> > and validate_rtdev is inconsistent and confusing when checking device
> > sizes, particularly when handling file images.
> > 
> > This patch unifies the validation flow by categorizing devices into
> > two distinct cases: "regular file" and "block device". Validation is
> > now performed separately for each case across all three subvolumes to
> > ensure consistent behavior.
> > 
> > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > ---
> > 
> > Hi,
> > 
> > validate_datadev, validate_logdev and validate_rtdev, these three functions
> > handle xi->*.size, cfg->*blocks, and cli->*size inconsistently while also
> > juggling xi->*.isfile status. Three functions ideally have similar validation
> > patterns, but instead of following a template, each function has its own
> > custom implementation, which invites bugs, maintenance overhead and inconsistent
> > behavior, especially for file images.
> > 
> > For example, mkfs.xfs works on an empty data file with -d size=xxx:
> > 
> > # mkfs.xfs -f -d name=/home/emptyfile,size=300m
> > meta-data=/home/emptyfile        isize=512    agcount=4, agsize=19200 blks
> >          =                       sectsz=512   attr=2, projid32bit=1
> >          =                       crc=1        finobt=1, sparse=1, rmapbt=1
> >          =                       reflink=1    bigtime=1 inobtcount=1 nrext64=1
> >          =                       exchange=1   metadir=0
> > data     =                       bsize=4096   blocks=76800, imaxpct=25
> >          =                       sunit=0      swidth=0 blks
> > naming   =version 2              bsize=4096   ascii-ci=0, ftype=1, parent=1
> > log      =internal log           bsize=4096   blocks=16384, version=2
> >          =                       sectsz=512   sunit=0 blks, lazy-count=1
> > realtime =none                   extsz=4096   blocks=0, rtextents=0
> >          =                       rgcount=0    rgsize=0 extents
> >          =                       zoned=0      start=0 reserved=0
> > 
> > But for log or rt, we got below weird errors:
> > 
> > # mkfs.xfs -f -l logdev=/home/emptyfile,size=128m /dev/pmem1
> > size 128m specified for log subvolume is too large, maximum is 0 blocks
> > ...
> > # mkfs.xfs -f -r rtdev=/home/emptyfile,size=128m /dev/pmem1
> > Invalid zero length rt subvolume found
> > ...
> > 
> > One said the "size=128m" is too large, maximum is 0 (??? due to the file
> > size is 0). The other one ignored the "size=128m", just complained the empty
> > file.
> > 
> > Thanks,
> > Zorro
> > 
> > 
> >  mkfs/xfs_mkfs.c | 115 ++++++++++++++++++++++++++++++------------------
> >  1 file changed, 72 insertions(+), 43 deletions(-)
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 9a93330f..5a2274ed 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3839,34 +3839,37 @@ validate_datadev(
> >  {
> >  	struct libxfs_init	*xi = cli->xi;
> >  
> > -	if (!xi->data.size) {
> > +	if (!xi->data.isfile) {
> >  		/*
> >  		 * if the device is a file, we can't validate the size here.
> >  		 * Instead, the file will be truncated to the correct length
> >  		 * later on. if it's not a file, we've got a dud device.
> >  		 */
> > -		if (!xi->data.isfile) {
> > +		if (!xi->data.size) {
> >  			fprintf(stderr, _("can't get size of data subvolume\n"));
> >  			usage();
> > -		} else {
> > -			if (!cli->dsize) {
> > +		}
> > +		if (cfg->dblocks) {
> > +			/* check the size fits into the underlying device */
> > +			if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
> >  				fprintf(stderr,
> > -_("Warning: Empty file needs a data subvolume size by -d size=<value> option\n"));
> > +_("size %s specified for data subvolume is too large, maximum is %lld blocks\n"),
> > +				        cli->dsize,
> > +				        (long long)DTOBT(xi->data.size, cfg->blocklog));
> >  				usage();
> >  			}
> > +		} else {
> > +			/* no user size, so use the full block device */
> > +			cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
> >  		}
> > -	} else if (cfg->dblocks) {
> > -		/* check the size fits into the underlying device */
> > -		if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
> > +	} else {
> > +		if (!cfg->dblocks && !xi->data.size) {
> >  			fprintf(stderr,
> > -_("size %s specified for data subvolume is too large, maximum is %lld blocks\n"),
> > -				cli->dsize,
> > -				(long long)DTOBT(xi->data.size, cfg->blocklog));
> > +_("Warning: Empty data file needs a data subvolume size by -d size=<value> option\n"));
> >  			usage();
> > +		} else if (xi->data.size && !cfg->dblocks) {
> > +			cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
> >  		}
> > -	} else {
> > -		/* no user size, so use the full block device */
> > -		cfg->dblocks = DTOBT(xi->data.size, cfg->blocklog);
> 
> I think this rearrangement preserves all the datadev validation checks,
> then makes the log/rt validation code look almost the same, except for
> which variables are accessed.  That change looks ok to me, but it's
> disappointing that there isn't a third patch that actually refactors all
> three into a single function, seeing as the commit message talks about
> unifying the implementations.

Thanks Darrick, you're right. I actually considered adding another patch
initially, but I wasn’t entirely confident in the modified logic since we
lack a regression test case for this specific mkfs.xfs behavior. Although
I’ve done some manual testing, I wanted to send this out for review first,
specially the "zt->rt.nr_zones" part, I'm not sure if I have missed
something. If the general approach looks good, I can send a v2 to have the
3rd patch.

Thanks,
Zorro

> 
> --D
> 
> >  	}
> >  
> >  	if (cfg->dblocks < XFS_MIN_DATA_BLOCKS(cfg)) {
> > @@ -3925,19 +3928,31 @@ _("log size %lld too large for internal log\n"),
> >  		usage();
> >  	}
> >  
> > -	if (!cfg->logblocks) {
> > -		if (xi->log.size == 0) {
> > +	if (!xi->log.isfile) {
> > +		if (!xi->log.size) {
> > +			fprintf(stderr, _("can't get size of log subvolume\n"));
> > +			usage();
> > +		} else if (cfg->logblocks) {
> > +			/* check the size fits into the underlying device */
> > +			if (cfg->logblocks > DTOBT(xi->log.size, cfg->blocklog)) {
> > +				fprintf(stderr,
> > +_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> > +				        cli->logsize,
> > +				        (long long)DTOBT(xi->log.size, cfg->blocklog));
> > +				usage();
> > +			}
> > +		} else {
> > +			/* no user size, so use the full block device */
> > +			cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
> > +		}
> > +	} else {
> > +		if (!cfg->logblocks && !xi->log.size) {
> >  			fprintf(stderr,
> > -_("unable to get size of the log subvolume.\n"));
> > +_("Warning: Empty log file needs a log subvolume size by -l size=<value> option\n"));
> >  			usage();
> > +		} else if (xi->log.size && !cfg->logblocks) {
> > +			cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
> >  		}
> > -		cfg->logblocks = DTOBT(xi->log.size, cfg->blocklog);
> > -	} else if (cfg->logblocks > DTOBT(xi->log.size, cfg->blocklog)) {
> > -		fprintf(stderr,
> > -_("size %s specified for log subvolume is too large, maximum is %lld blocks\n"),
> > -			cli->logsize,
> > -			(long long)DTOBT(xi->log.size, cfg->blocklog));
> > -		usage();
> >  	}
> >  
> >  	if (xi->log.bsize > cfg->lsectorsize) {
> > @@ -3968,31 +3983,45 @@ _("size specified for non-existent rt subvolume\n"));
> >  		cfg->rtbmblocks = 0;
> >  		return;
> >  	}
> > -	if (!xi->rt.size) {
> > -		fprintf(stderr, _("Invalid zero length rt subvolume found\n"));
> > -		usage();
> > -	}
> >  
> > -	if (cli->rtsize) {
> > -		if (cfg->rtblocks > DTOBT(xi->rt.size, cfg->blocklog)) {
> > -			fprintf(stderr,
> > +	if (!xi->rt.isfile) {
> > +		if (!xi->rt.size) {
> > +			fprintf(stderr, _("can't get size of realtime subvolume\n"));
> > +			usage();
> > +		}
> > +		if (cfg->rtblocks) {
> > +			/* check the size fits into the underlying device */
> > +			if (cfg->rtblocks > DTOBT(xi->rt.size, cfg->blocklog)) {
> > +				fprintf(stderr,
> >  _("size %s specified for rt subvolume is too large, maximum is %lld blocks\n"),
> > -				cli->rtsize,
> > -				(long long)DTOBT(xi->rt.size, cfg->blocklog));
> > +				        cli->rtsize,
> > +				        (long long)DTOBT(xi->rt.size, cfg->blocklog));
> > +				usage();
> > +			}
> > +		} else {
> > +			/* no user size, so use the full block device */
> > +			if (zt->rt.nr_zones) {
> > +				cfg->rtblocks = DTOBT(zt->rt.nr_zones * zt->rt.zone_capacity,
> > +				                      cfg->blocklog);
> > +			} else {
> > +				cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
> > +			}
> > +		}
> > +	} else {
> > +		if (!cfg->rtblocks && !xi->rt.size) {
> > +			fprintf(stderr,
> > +_("Warning: Empty rt file needs a rt subvolume size by -r size=<value> option\n"));
> >  			usage();
> > +		} else if (xi->rt.size && !cfg->rtblocks) {
> > +			cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
> >  		}
> > -		if (xi->rt.bsize > cfg->sectorsize) {
> > -			fprintf(stderr, _(
> > +	}
> > +
> > +	if (xi->rt.bsize > cfg->sectorsize) {
> > +		fprintf(stderr, _(
> >  "Warning: the realtime subvolume sector size %u is less than the sector size\n\
> >  reported by the device (%u).\n"),
> > -				cfg->sectorsize, xi->rt.bsize);
> > -		}
> > -	} else if (zt->rt.nr_zones) {
> > -		cfg->rtblocks = DTOBT(zt->rt.nr_zones * zt->rt.zone_capacity,
> > -				      cfg->blocklog);
> > -	} else {
> > -		/* grab volume size */
> > -		cfg->rtblocks = DTOBT(xi->rt.size, cfg->blocklog);
> > +		        cfg->sectorsize, xi->rt.bsize);
> >  	}
> >  
> >  	cfg->rtextents = cfg->rtblocks / cfg->rtextblocks;
> > -- 
> > 2.52.0
> > 
> > 
>