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

["Darrick J. Wong" <djwong@kernel.org>]

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.

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