Re: [PATCH] mkfs.xfs fix sunit size on 512e and 4kN disks.

[lukas@herbolt.com]

On 2025-09-29 08:54, Christoph Hellwig wrote:
> On Fri, Sep 26, 2025 at 02:38:30PM +0200, Lukas Herbolt wrote:
>> Creating of XFS on 4kN or 512e disk result in suboptimal LSU/LSUNIT.
>> As of now we check if the sectorsize is bigger than XLOG_HEADER_SIZE
>> and so we set lsu to blocksize. But we do not check the the size if
>> lsunit can be bigger to fit the disk geometry.
> 
> As I had to walk the code to understand (again for the nth time :))
> what the lsunit actually does:  it pads every log write up to that
> size.  I.e. if you set a log stripe unit, that effectively becomes the
> minimum I/O size for the log.  So yes, setting it to the minimum I/O
> size of the device makes sense.  But maybe the commit log should be
> a bit more clear about that?  (and of course our terminology should
> be as well, ast least outside the user interface that we can't touch).
> 
>> Before:
> 
> You Before/after also contain changes for metadir/zoned, looks like you
> upgraded to a new xfsprogs for your patch, but not the baseline.
> 
Yeah it was fedora rawhide, 6.12, did not notice the metadir/zoned.

>> index 8cd4ccd7..05268cd9 100644
>> --- a/mkfs/xfs_mkfs.c
>> +++ b/mkfs/xfs_mkfs.c
>> @@ -3643,6 +3643,10 @@ check_lsunit:
>>  		lsu = getnum(cli->lsu, &lopts, L_SU);
>>  	else if (cfg->lsectorsize > XLOG_HEADER_SIZE)
>>  		lsu = cfg->blocksize; /* lsunit matches filesystem block size */
>> +		if (cfg->dsunit){
>> +			cfg->lsunit = cfg->dsunit;
>> +			lsu = 0;
>> +		}
> 
> I don't think just picking the data stripe unit is correct here, given
> that the log can also be external and on a separate device.  Instead
> we'll need to duplicate the calculation based on ft.log, preferably by
> factoring it into a helper.
Hmmm, aren't all the <data|rt|log>.sunit" set by blkid_get_topology()?
So as log is internal lsunit should be equal to dsunit and it can only
differ if the log is external?

Based on comment:

     /*
      * check that log sunit is modulo fsblksize; default it to dsunit 
for
      * an internal log; or the log device stripe unit if it's external.
      */

> The lsu = 0 also drop the multiple of block size check.  If that is not
> a hard requirement (and I'd have to do some research where it is coming
> from) we should relax the check instead of silently disabling it like
> this.

My understanding was the LSU check is there mostly if cli->lsu is set.
Actually if that's assumption is correct it can be done just like this.

---
  mkfs/xfs_mkfs.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 8cd4ccd7..3aecacd3 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3644,7 +3644,7 @@ check_lsunit:
  	else if (cfg->lsectorsize > XLOG_HEADER_SIZE)
  		lsu = cfg->blocksize; /* lsunit matches filesystem block size */

-	if (lsu) {
+	if (cli->lsu) {
  		/* verify if lsu is a multiple block size */
  		if (lsu % cfg->blocksize != 0) {
  			fprintf(stderr,
-- 
2.51.0