| Message ID | 20240304130428.13026-3-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block atomic writes for XFS | expand |
On Mon, Mar 04, 2024 at 01:04:16PM +0000, John Garry wrote: > The low-space allocator doesn't honour the alignment requirement, so don't > attempt to even use it (when we have an alignment requirement). > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > fs/xfs/libxfs/xfs_bmap.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c > index f362345467fa..60d100134280 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3584,6 +3584,10 @@ xfs_bmap_btalloc_low_space( > { > int error; > > + /* The allocator doesn't honour args->alignment */ > + if (args->alignment > 1) > + return 0; I think that's wrong. The alignment argument here is purely a best effort consideration - we ignore it several different allocation situations, not just low space. e.g. xfs_bmap_btalloc_at_eof() will try exact block allocation regardless of whether an alignment parameter is set. It will then fall back to stripe alignment if exact block fails. If stripe aligned allocation fails, it will then set args->alignment = 1 and try a full filesystem allocation scan without alignment. And if that fails, then we finally get to the low space allocator with args->alignment = 1 even though we might be trying to allocate an aligned extent for an atomic IO.... IOWs, I think this indicates deeper surgery is needed to ensure aligned allocations fail immediately and don't fall back to unaligned allocations and set XFS_TRANS_LOW_MODE... -Dave.
On 04/03/2024 22:15, Dave Chinner wrote: > On Mon, Mar 04, 2024 at 01:04:16PM +0000, John Garry wrote: >> The low-space allocator doesn't honour the alignment requirement, so don't >> attempt to even use it (when we have an alignment requirement). >> >> Signed-off-by: John Garry <john.g.garry@oracle.com> >> --- >> fs/xfs/libxfs/xfs_bmap.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c >> index f362345467fa..60d100134280 100644 >> --- a/fs/xfs/libxfs/xfs_bmap.c >> +++ b/fs/xfs/libxfs/xfs_bmap.c >> @@ -3584,6 +3584,10 @@ xfs_bmap_btalloc_low_space( >> { >> int error; >> >> + /* The allocator doesn't honour args->alignment */ >> + if (args->alignment > 1) >> + return 0; > > I think that's wrong. > > The alignment argument here is purely a best effort consideration - > we ignore it several different allocation situations, not just low > space. Sure, but I am simply addressing the low-space allocator here. In this series I am /we are effectively trying to conflate args->alignment > 1 with forcealign. I thought that args->alignment was guaranteed to be honoured, with some caveats. For forcealign, we obviously require a guarantee. > > e.g. xfs_bmap_btalloc_at_eof() will try exact block > allocation regardless of whether an alignment parameter is set. For this specific issue, I think that we are ok, as: - in xfs_bmap_compute_alignments(), stripe_align is aligned with args->alignment for forcealign - xfs_bmap_btalloc_at_eof() has the optimisation to alloc according to stripe alignment But obviously we should not be relying on optimisations. Please also note that I have a modification later in this series to always have EOF aligned for forcealign. > It > will then fall back to stripe alignment if exact block fails. > > If stripe aligned allocation fails, it will then set args->alignment > = 1 and try a full filesystem allocation scan without alignment. > > And if that fails, then we finally get to the low space allocator > with args->alignment = 1 even though we might be trying to allocate > an aligned extent for an atomic IO.... > > IOWs, I think this indicates deeper surgery is needed to ensure > aligned allocations fail immediately and don't fall back to > unaligned allocations and set XFS_TRANS_LOW_MODE... > ok, I'll look at what you write about all of this in the later patch review. Thanks, John
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index f362345467fa..60d100134280 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3584,6 +3584,10 @@ xfs_bmap_btalloc_low_space( { int error; + /* The allocator doesn't honour args->alignment */ + if (args->alignment > 1) + return 0; + if (args->minlen > ap->minlen) { args->minlen = ap->minlen; error = xfs_alloc_vextent_start_ag(args, ap->blkno);
The low-space allocator doesn't honour the alignment requirement, so don't attempt to even use it (when we have an alignment requirement). Signed-off-by: John Garry <john.g.garry@oracle.com> --- fs/xfs/libxfs/xfs_bmap.c | 4 ++++ 1 file changed, 4 insertions(+)