On Mon, Mar 03, 2025 at 05:11:20PM +0000, John Garry wrote:
> When issuing an atomic write by the CoW method, give the block allocator a
> hint to align to the extszhint.
>
> This means that we have a better chance to issuing the atomic write via
> HW offload next time.
>
> It does mean that the inode extszhint should be set appropriately for the
> expected atomic write size.
>
> Reviewed-by: "Darrick J. Wong" <djwong@xxxxxxxxxx>
> Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 7 ++++++-
> fs/xfs/libxfs/xfs_bmap.h | 6 +++++-
> fs/xfs/xfs_reflink.c | 8 ++++++--
> 3 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 0ef19f1469ec..9bfdfb7cdcae 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -3454,6 +3454,12 @@ xfs_bmap_compute_alignments(
> align = xfs_get_cowextsz_hint(ap->ip);
> else if (ap->datatype & XFS_ALLOC_USERDATA)
> align = xfs_get_extsz_hint(ap->ip);
> +
> + if (align > 1 && ap->flags & XFS_BMAPI_EXTSZALIGN)
needs () around the & logic.
if (align > 1 && (ap->flags & XFS_BMAPI_EXTSZALIGN))
> + args->alignment = align;
> + else
> + args->alignment = 1;
When is args->alignment not already initialised to 1?
> +
> if (align) {
> if (xfs_bmap_extsize_align(mp, &ap->got, &ap->prev, align, 0,
> ap->eof, 0, ap->conv, &ap->offset,
> @@ -3782,7 +3788,6 @@ xfs_bmap_btalloc(
> .wasdel = ap->wasdel,
> .resv = XFS_AG_RESV_NONE,
> .datatype = ap->datatype,
> - .alignment = 1,
> .minalignslop = 0,
> };
Oh, you removed the initialisation to 1, so now we have the
possibility of getting args->alignment = 0 anywhere in the
allocation stack?
FWIW, we've been trying to get rid of that case - args->alignment should
always be 1 if no alignment is necessary so we don't ahve to special
case alignment of 0 (meaning no alignemnt) anywhere. This seems
like a step backwards from that perspective...
> xfs_fileoff_t orig_offset;
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 4b721d935994..e6baa81e20d8 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -87,6 +87,9 @@ struct xfs_bmalloca {
> /* Do not update the rmap btree. Used for reconstructing bmbt from rmapbt. */
> #define XFS_BMAPI_NORMAP (1u << 10)
>
> +/* Try to align allocations to the extent size hint */
> +#define XFS_BMAPI_EXTSZALIGN (1u << 11)
Don't we already do that?
Or is this doing something subtle and non-obvious like overriding
stripe width alignment for large atomic writes?
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
