Re: [PATCH 6/7] mkfs: try to align AG size based on atomic write capabilities

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

On Wed, Jul 02, 2025 at 10:03:54AM +0100, John Garry wrote:
> On 01/07/2025 19:08, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Try to align the AG size to the maximum hardware atomic write unit so
> > that we can give users maximum flexibility in choosing an RWF_ATOMIC
> > write size.
> 
> 
> Regardless of comments below, FWIW:
> 
> Reviewed-by: John Garry <john.g.garry@oracle.com>
> 
> 
> > 
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> >   libxfs/topology.h |    6 ++++--
> >   libxfs/topology.c |   36 ++++++++++++++++++++++++++++++++++++
> >   mkfs/xfs_mkfs.c   |   48 +++++++++++++++++++++++++++++++++++++++++++-----
> >   3 files changed, 83 insertions(+), 7 deletions(-)
> > 
> > 
> > diff --git a/libxfs/topology.h b/libxfs/topology.h
> > index 207a8a7f150556..f0ca65f3576e92 100644
> > --- a/libxfs/topology.h
> > +++ b/libxfs/topology.h
> > @@ -13,8 +13,10 @@
> >   struct device_topology {
> >   	int	logical_sector_size;	/* logical sector size */
> >   	int	physical_sector_size;	/* physical sector size */
> > -	int	sunit;		/* stripe unit */
> > -	int	swidth;		/* stripe width  */
> > +	int	sunit;			/* stripe unit */
> > +	int	swidth;			/* stripe width  */
> > +	int	awu_min;		/* min atomic write unit in bbcounts */
> 
> awu_min is not really used, but, like the kernel code does, I suppose useful
> to store it
> 
> > +	int	awu_max;		/* max atomic write unit in bbcounts */
> >   };
> >   struct fs_topology {
> > diff --git a/libxfs/topology.c b/libxfs/topology.c
> > index 96ee74b61b30f5..7764687beac000 100644
> > --- a/libxfs/topology.c
> > +++ b/libxfs/topology.c
> > @@ -4,11 +4,18 @@
> >    * All Rights Reserved.
> >    */
> > +#ifdef OVERRIDE_SYSTEM_STATX
> > +#define statx sys_statx
> > +#endif
> > +#include <fcntl.h>
> > +#include <sys/stat.h>
> > +
> >   #include "libxfs_priv.h"
> >   #include "libxcmd.h"
> >   #include <blkid/blkid.h>
> >   #include "xfs_multidisk.h"
> >   #include "libfrog/platform.h"
> > +#include "libfrog/statx.h"
> >   #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
> >   #define GIGABYTES(count, blog)	((uint64_t)(count) << (30 - (blog)))
> > @@ -278,6 +285,34 @@ blkid_get_topology(
> >   		device);
> >   }
> > +static void
> > +get_hw_atomic_writes_topology(
> > +	struct libxfs_dev	*dev,
> > +	struct device_topology	*dt)
> > +{
> > +	struct statx		sx;
> > +	int			fd;
> > +	int			ret;
> > +
> > +	fd = open(dev->name, O_RDONLY);
> > +	if (fd < 0)
> > +		return;
> > +
> > +	ret = statx(fd, "", AT_EMPTY_PATH, STATX_WRITE_ATOMIC, &sx);
> > +	if (ret)
> > +		goto out_close;
> > +
> > +	if (!(sx.stx_mask & STATX_WRITE_ATOMIC))
> > +		goto out_close;
> > +
> > +	dt->awu_min = sx.stx_atomic_write_unit_min >> 9;
> > +	dt->awu_max = max(sx.stx_atomic_write_unit_max_opt,
> > +			  sx.stx_atomic_write_unit_max) >> 9;
> 
> for a bdev, stx_atomic_write_unit_max_opt should be zero
> 
> However, I suppose some bdev could have hybrid atomic write support, just
> like xfs, so what you are doing looks good

Yeah, it's unlikely ever to happen but if (say) you had a loop device
backed by an xfs file then maybe it'd useful to pass through both atomic
write maxima.

Thanks for the review.

--D