On 03/10/2023 02:42, Dave Chinner wrote:
> On Fri, Sep 29, 2023 at 10:27:18AM +0000, John Garry wrote:
>> From: "Darrick J. Wong" <djwong at kernel.org>
>>
>> The existing extsize hint code already did the work of expanding file
>> range mapping requests so that the range is aligned to the hint value.
>> Now add the code we need to guarantee that the space allocations are
>> also always aligned.
>>
>> XXX: still need to check all this with reflink
>>
>> Signed-off-by: Darrick J. Wong <djwong at kernel.org>
>> Co-developed-by: John Garry <john.g.garry at oracle.com>
>> Signed-off-by: John Garry <john.g.garry at oracle.com>
>> ---
>> fs/xfs/libxfs/xfs_bmap.c | 22 +++++++++++++++++-----
>> fs/xfs/xfs_iomap.c | 4 +++-
>> 2 files changed, 20 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
>> index 328134c22104..6c864dc0a6ff 100644
>> --- a/fs/xfs/libxfs/xfs_bmap.c
>> +++ b/fs/xfs/libxfs/xfs_bmap.c
>> @@ -3328,6 +3328,19 @@ 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);
>> +
>> + /*
>> + * xfs_get_cowextsz_hint() returns extsz_hint for when forcealign is
>> + * set as forcealign and cowextsz_hint are mutually exclusive
>> + */
>> + if (xfs_inode_forcealign(ap->ip) && align) {
>> + args->alignment = align;
>> + if (stripe_align % align)
>> + stripe_align = align;
>> + } else {
>> + args->alignment = 1;
>> + }
>
> This smells wrong.
>
> If a filesystem has a stripe unit set (hence stripe_align is
> non-zero) then any IO that crosses stripe unit boundaries will not
> be atomic - they will require multiple IOs to different devices.
>
> Hence if the filesystem has a stripe unit set, then all forced
> alignment hints for atomic IO *must* be an exact integer divider
> of the stripe unit. hence when an atomic IO bundle is aligned, the
> atomic boundaries within the bundle always fall on a stripe unit
> boundary and never cross devices.
>
> IOWs, for a striped filesystem, the maximum size/alignment for a
> single atomic IO unit is the stripe unit.
>
ok, when I added this I was looking at being robust against wacky
scenarios when that is not true, like forcealign = stripe alignment * 2.
Please note that this forcealign feature is being added with the view
that it can be useful for other scenarios, and not just atomic writes.
> This should be enforced when the forced align flag is set on the
> inode (i.e. from the ioctl)
ok, fine.
>
>
>> +
>> if (align) {
>> if (xfs_bmap_extsize_align(mp, &ap->got, &ap->prev, align, 0,
>> ap->eof, 0, ap->conv, &ap->offset,
>> @@ -3423,7 +3436,6 @@ xfs_bmap_exact_minlen_extent_alloc(
>> args.minlen = args.maxlen = ap->minlen;
>> args.total = ap->total;
>>
>> - args.alignment = 1;
>> args.minalignslop = 0;
>>
>> args.minleft = ap->minleft;
>> @@ -3469,6 +3481,7 @@ xfs_bmap_btalloc_at_eof(
>> {
>> struct xfs_mount *mp = args->mp;
>> struct xfs_perag *caller_pag = args->pag;
>> + int orig_alignment = args->alignment;
>> int error;
>>
>> /*
>> @@ -3543,10 +3556,10 @@ xfs_bmap_btalloc_at_eof(
>>
>> /*
>> * Allocation failed, so turn return the allocation args to their
>> - * original non-aligned state so the caller can proceed on allocation
>> - * failure as if this function was never called.
>> + * original state so the caller can proceed on allocation failure as
>> + * if this function was never called.
>> */
>> - args->alignment = 1;
>> + args->alignment = orig_alignment;
>> return 0;
>> }
>
> Urk. Not sure that is right, it's certainly a change of behaviour.
Is it really a change in behaviour? We just restore the args->alignment
value, which was originally always 1.
As described in the comment, above, args->alignment is temporarily set
to the stripe align to try to align a new alloc on a stripe boundary.
>
>> @@ -3694,7 +3707,6 @@ xfs_bmap_btalloc(
>> .wasdel = ap->wasdel,
>> .resv = XFS_AG_RESV_NONE,
>> .datatype = ap->datatype,
>> - .alignment = 1,
>> .minalignslop = 0,
>> };
>> xfs_fileoff_t orig_offset;
>> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
>> index 18c8f168b153..70fe873951f3 100644
>> --- a/fs/xfs/xfs_iomap.c
>> +++ b/fs/xfs/xfs_iomap.c
>> @@ -181,7 +181,9 @@ xfs_eof_alignment(
>> * If mounted with the "-o swalloc" option the alignment is
>> * increased from the strip unit size to the stripe width.
>> */
>> - if (mp->m_swidth && xfs_has_swalloc(mp))
>> + if (xfs_inode_forcealign(ip))
>> + align = xfs_get_extsz_hint(ip);
>> + else if (mp->m_swidth && xfs_has_swalloc(mp))
>> align = mp->m_swidth;
>> else if (mp->m_dalign)
>> align = mp->m_dalign;
>
> Ah. Now I see. This abuses the stripe alignment code to try to
> implement this new inode allocation alignment restriction, rather
> than just making the extent size hint alignment mandatory....
>
> Yeah, this can be done better... :)
>
> As it is, I have been working on a series that reworks all this
> allocator code to separate out the aligned IO from the exact EOF
> allocation case to help clean this up for better perag selection
> during allocation. I think that needs to be done first before we go
> making the alignment code more intricate like this....
>
> -Dave.
ok, fine. I think that we'll just keep this code as is until that code
you mention appears, apart from enforcing stripe alignment % forcealign
== 0.
Thanks,
John