Re: [PATCH v2 04/14] fs: xfs: Make file data allocations observe the 'forcealign' flag

[Dave Chinner <david@fromorbit.com>]

On Tue, Mar 05, 2024 at 03:22:52PM +0000, John Garry wrote:
> On 05/03/2024 00:44, Dave Chinner wrote:
> > On Mon, Mar 04, 2024 at 01:04:18PM +0000, John Garry wrote:
....
> > IOWs, these are static geometry constraints and so should be checked
> > and rejected at the point where alignments are specified (i.e.
> > mkfs, mount and ioctls). Then the allocator can simply assume that
> > forced inode alignments are always stripe alignment compatible and
> > we don't need separate handling of two possibly incompatible
> > alignments.
> 
> ok, makes sense.
> 
> Please note in case missed, I am mandating extsize hint for forcealign needs
> to be a power-of-2. It just makes life easier for all the sub-extent
> zero'ing later on.

That's fine - that will need to be documented in the xfsctl man
page...

> Also we need to enforce that the AG count to be compliant with the extsize
                                      ^^^^^ size?

> hint for forcealign; but since the extsize hint for forcealign needs to be
> compliant with stripe unit, above, and stripe unit would be compliant wth AG
> count (right?), then this would be a given.

We already align AG size to stripe unit when a stripe unit is set,
and ensure that we don't place all the AG headers on the same stripe
unit.

However, if there is no stripe unit we don't align the AG to
anything. So, yes, AG sizing by mkfs will need to ensure that all
AGs are correctly aligned to the underlying storage (integer
multiple of the max atomic write size, right?)...

> > More below....
> > 
> > > +	} else {
> > > +		args->alignment = 1;
> > > +	}
> > 
> > Just initialise the allocation args structure with a value of 1 like
> > we already do?
> 
> It was being done in this way to have just a single place where the value is
> initialised. It can easily be kept as is.

I'd prefer it as is, because then the value is always initialised
correctly and we only override in the special cases....

> > >   	args.minleft = ap->minleft;
> > > @@ -3484,6 +3496,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;
> > >   	/*
> > > @@ -3558,10 +3571,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;
> > >   }
> > 
> > As I said above, we can't set an alignment of > 1 here if we haven't
> > accounted for that alignment in args->minalignslop above. This leads
> > to unexpected ENOSPC conditions and filesystem shutdowns.
> > 
> > I suspect what we need to do is get rid of the separate stripe_align
> > variable altogether and always just set args->alignment to what we
> > need the extent start alignment to be, regardless of whether it is
> > from stripe alignment or forced alignment.
> 
> ok, it sounds a bit simpler at least
> 
> > 
> > Then the code in xfs_bmap_btalloc_at_eof() doesn't need to know what
> > 'stripe_align' is - the exact EOF block allocation can simply save
> > and restore the args->alignment value and use it for minalignslop
> > calculations for the initial exact block allocation.
> > 
> > Then, if xfs_bmap_btalloc_at_eof() fails and xfs_inode_forcealign()
> > is true, we can abort allocation immediately, and not bother to fall
> > back on further aligned/unaligned attempts that will also fail or do
> > the wrong them.
> 
> ok
> 
> > 
> > Similarly, if we aren't doing EOF allocation, having args->alignment
> > set means it will do the right thing for the first allocation
> > attempt. Again, if that fails, we can check if
> > xfs_inode_forcealign() is true and fail the aligned allocation
> > instead of running the low space algorithm. This now makes it clear
> > that we're failing the allocation because of the forced alignment
> > requirement, and now the low space allocation code can explicitly
> > turn off start alignment as it isn't required...
> 
> are you saying that low-space allocator can set args->alignment = 1 to be
> explicit?

Yes.

-Dave.

-- 
Dave Chinner
david@fromorbit.com