| Message ID | 20230929102726.2985188-12-john.g.garry@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block atomic writes | expand |
On Fri, Sep 29, 2023 at 10:27:16AM +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 30c931b38853..328134c22104 100644 > --- a/fs/xfs/libxfs/xfs_bmap.c > +++ b/fs/xfs/libxfs/xfs_bmap.c > @@ -3569,6 +3569,10 @@ xfs_bmap_btalloc_low_space( > { > int error; > > + /* The allocator doesn't honour args->alignment */ > + if (args->alignment > 1) > + return 0; > + How does this happen? The earlier failing aligned allocations will clear alignment before we get here.... -Dave.
On Tue, Oct 03, 2023 at 12:16:26PM +1100, Dave Chinner wrote: > On Fri, Sep 29, 2023 at 10:27:16AM +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 30c931b38853..328134c22104 100644 > > --- a/fs/xfs/libxfs/xfs_bmap.c > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > @@ -3569,6 +3569,10 @@ xfs_bmap_btalloc_low_space( > > { > > int error; > > > > + /* The allocator doesn't honour args->alignment */ > > + if (args->alignment > 1) > > + return 0; > > + > > How does this happen? > > The earlier failing aligned allocations will clear alignment before > we get here.... I was thinking the predicate should be xfs_inode_force_align(ip) to save me/us from thinking about all the other weird ways args->alignment could end up 1. /* forced-alignment means we don't use low mode */ if (xfs_inode_force_align(ip)) return -ENOSPC; --D > -Dave. > -- > Dave Chinner > david@fromorbit.com
On Mon, Oct 02, 2023 at 08:00:10PM -0700, Darrick J. Wong wrote: > On Tue, Oct 03, 2023 at 12:16:26PM +1100, Dave Chinner wrote: > > On Fri, Sep 29, 2023 at 10:27:16AM +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 30c931b38853..328134c22104 100644 > > > --- a/fs/xfs/libxfs/xfs_bmap.c > > > +++ b/fs/xfs/libxfs/xfs_bmap.c > > > @@ -3569,6 +3569,10 @@ xfs_bmap_btalloc_low_space( > > > { > > > int error; > > > > > > + /* The allocator doesn't honour args->alignment */ > > > + if (args->alignment > 1) > > > + return 0; > > > + > > > > How does this happen? > > > > The earlier failing aligned allocations will clear alignment before > > we get here.... > > I was thinking the predicate should be xfs_inode_force_align(ip) to save > me/us from thinking about all the other weird ways args->alignment could > end up 1. > > /* forced-alignment means we don't use low mode */ > if (xfs_inode_force_align(ip)) > return -ENOSPC; See the email I just wrote about not needing per-inode on-disk state or even extent size hints for doing allocation for atomic IO. Atomic write unit alignment is a device parameter (similar to stripe unit) that applies to context specific allocation requests - it's not an inode property as such.... Cheers, Dave.
On 03/10/2023 04:00, Darrick J. Wong wrote: >> How does this happen? >> >> The earlier failing aligned allocations will clear alignment before >> we get here.... > I was thinking the predicate should be xfs_inode_force_align(ip) to save > me/us from thinking about all the other weird ways args->alignment could > end up 1. > > /* forced-alignment means we don't use low mode */ > if (xfs_inode_force_align(ip)) My idea was that if we add another feature which requires args->alignment > 1 be honoured, then we would need to change this code to cover both features, so better just check args->alignment > 1. > return -ENOSPC; Thanks, John
diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c index 30c931b38853..328134c22104 100644 --- a/fs/xfs/libxfs/xfs_bmap.c +++ b/fs/xfs/libxfs/xfs_bmap.c @@ -3569,6 +3569,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(+)