[PATCH 0/2] xfsprogs/mkfs: consolidate subvolume validation logic for file images

[PATCH 0/2] xfsprogs/mkfs: consolidate subvolume validation logic for file images

[Zorro Lang]

This patchset contains two patches:
[PATCH 1/2] mkfs: fix assertion failure on empty data file
[PATCH 2/2] mkfs: unify validation behavior for data, log and rt dev

[PATCH 1/2] trys to fix an assertion failure in xfs_mkfs.c . Then [PATCH 2/2]
trys to rewrite the subvolume validation logic of validate_{data,log,rt}dev.
Further details are available in the patches. Any feedback or review points
would be appreciated.

Thanks,
Zorro

[PATCH 1/2] mkfs: fix assertion failure on empty data file

[Zorro Lang]

mkfs.xfs triggers an assertion failure, when it trys to make a xfs
on an empty file:
  # echo > emptyfile
  # mkfs.xfs emptyfile
  mkfs.xfs: xfs_mkfs.c:3852: validate_datadev: Assertion `cfg->dblocks' failed.
  Aborted                    (core dumped) mkfs.xfs emptyfile

This shouldn't be an assertion, but a warning.

Signed-off-by: Zorro Lang <zlang@kernel.org>
---
 mkfs/xfs_mkfs.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 527a662f..9a93330f 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3848,8 +3848,13 @@ validate_datadev(
 		if (!xi->data.isfile) {
 			fprintf(stderr, _("can't get size of data subvolume\n"));
 			usage();
+		} else {
+			if (!cli->dsize) {
+				fprintf(stderr,
+_("Warning: Empty file needs a data subvolume size by -d size=<value> option\n"));
+				usage();
+			}
 		}
-		ASSERT(cfg->dblocks);
 	} else if (cfg->dblocks) {
 		/* check the size fits into the underlying device */
 		if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
-- 
2.52.0

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

[Zorro Lang]

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

Re: [PATCH 1/2] mkfs: fix assertion failure on empty data file

[Darrick J. Wong]

On Sun, Apr 05, 2026 at 12:36:39AM +0800, Zorro Lang wrote:
> mkfs.xfs triggers an assertion failure, when it trys to make a xfs
> on an empty file:
>   # echo > emptyfile
>   # mkfs.xfs emptyfile
>   mkfs.xfs: xfs_mkfs.c:3852: validate_datadev: Assertion `cfg->dblocks' failed.
>   Aborted                    (core dumped) mkfs.xfs emptyfile
> 
> This shouldn't be an assertion, but a warning.
> 
> Signed-off-by: Zorro Lang <zlang@kernel.org>
> ---
>  mkfs/xfs_mkfs.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 527a662f..9a93330f 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3848,8 +3848,13 @@ validate_datadev(
>  		if (!xi->data.isfile) {
>  			fprintf(stderr, _("can't get size of data subvolume\n"));
>  			usage();
> +		} else {
> +			if (!cli->dsize) {
> +				fprintf(stderr,
> +_("Warning: Empty file needs a data subvolume size by -d size=<value> option\n"));

I'm not sure why "Warning" is appropriate here -- this error is fatal to
the program, and most fatal mkfs error messages don't have a prefix.

Other than that, the logic is sound that we need a softer landing for
this scenario.

--D

> +				usage();
> +			}
>  		}
> -		ASSERT(cfg->dblocks);
>  	} else if (cfg->dblocks) {
>  		/* check the size fits into the underlying device */
>  		if (cfg->dblocks > DTOBT(xi->data.size, cfg->blocklog)) {
> -- 
> 2.52.0
> 
> 

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

[Darrick J. Wong]

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

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

[Christoph Hellwig]

On Mon, Apr 06, 2026 at 08:37:26AM -0700, Darrick J. Wong wrote:
> 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.

Agreed, it would be really helpful to share the code here.