Re: [PATCH v3 1/2] xfs: set a mount flag when perag reservation is active

Brian Foster
2021-03-19T14:55:13Z

On Fri, Mar 19, 2021 at 12:43:03PM +1100, Dave Chinner wrote:
> On Fri, Mar 19, 2021 at 12:05:06PM +1100, Dave Chinner wrote:
> > On Thu, Mar 18, 2021 at 03:19:01PM -0700, Darrick J. Wong wrote:
> > > TBH I think the COW recovery and the AG block reservation pieces are
> > > prime candidates for throwing at an xfs_pwork workqueue so we can
> > > perform those scans in parallel.
> >
> > As I mentioned on #xfs, I think we only need to do the AG read if we
> > are near enospc. i.e. we can take the entire reservation at mount
> > time (which is fixed per-ag) and only take away the used from the
> > reservation (i.e. return to the free space pool) when we actually
> > access the AGF/AGI the first time. Or when we get a ENOSPC
> > event, which might occur when we try to take the fixed reservation
> > at mount time...
>
> Which leaves the question about when we need to actually do the
> accounting needed to fix the bug Brian is trying to fix. Can that be
> delayed until we read the AGFs or have an ENOSPC event occur? Or
> maybe some other "we are near ENOSPC and haven't read all AGFs yet"
> threshold/trigger?
>

Technically there isn't a hard requirement to read in any AGFs at mount
time. The tradeoff is that leaves a gap in effectiveness until at least
the majority of allocbt blocks have been accounted for (via perag agf
initialization). The in-core counter simply folds into the reservation
set aside value, so it would just remain at 0 at reservation time and
behave as if the mechanism didn't exist in the first place. The obvious
risk is a user can mount the fs and immediately acquire reservation
without having populated the counter from enough AGs to prevent the
reservation overrun problem. For that reason, I didn't really consider
the "lazy" init approach a suitable fix and hooked onto the (mostly)
preexisting perag res behavior to initialize the appropriate structures
at mount time.

If that underlying mount time behavior changes, it's not totally clear
to me how that impacts this patch. If the perag res change relies on an
overestimated mount time reservation and a fallback to a hard scan on
-ENOSPC, then I wonder whether the overestimated reservation might
effectively subsume whatever the allocbt set aside might be for that AG.
If so, and the perag init effectively transfers excess reservation back
to free space at the same time allocbt blocks are accounted for (and set
aside from subsequent reservations), perhaps that has a similar net
effect as the current behavior (of initializing the allocbt count at
mount time)..?

One problem is that might be hard to reason about even with code in
place, let alone right now when the targeted behavior is still
vaporware. OTOH, I suppose that if we do know right now that the perag
res scan will still fall back to mount time scans beyond some low free
space threshold, perhaps it's just a matter of factoring allocbt set
aside into the threshold somehow so that we know the counter will always
be initialized before a user can over reserve blocks. As it is, I don't
really have a strong opinion on whether we should try to make this fix
now and preserve it, or otherwise table it and revisit once we know what
the resulting perag res code will look like. Thoughts?

Brian

> If that's the case, then I'm happy to have this patchset proceed as
> it stands under the understanding that there will be follow up to
> make the clean, lots of space free mount case avoid reading the the
> AG headers.
>
> If it can't be made constrained, then I think we probably need to
> come up with a different approach that doesn't require reading every
> AG header on every mount...
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@fromorbit.com
>