On Wed, Feb 01, 2023 at 04:01:24PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 19, 2023 at 09:45:04AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > Now that the filestreams AG selection tracks active perags, we need
> > to return an active perag to the core allocator code. This is
> > because the file allocation the filestreams code will run are AG
> > specific allocations and so need to pin the AG until the allocations
> > complete.
> >
> > We cannot rely on the filestreams item reference to do this - the
> > filestreams association can be torn down at any time, hence we
> > need to have a separate reference for the allocation process to pin
> > the AG after it has been selected.
> >
> > This means there is some perag juggling in allocation failure
> > fallback paths as they will do all AG scans in the case the AG
> > specific allocation fails. Hence we need to track the perag
> > reference that the filestream allocator returned to make sure we
> > don't leak it on repeated allocation failure.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_bmap.c | 38 +++++++++++-----
> > fs/xfs/xfs_filestream.c | 93 ++++++++++++++++++++++++----------------
> > 2 files changed, 84 insertions(+), 47 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 098b46f3f3e3..7f56002b545d 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3427,6 +3427,7 @@ xfs_bmap_btalloc_at_eof(
> > bool ag_only)
> > {
> > struct xfs_mount *mp = args->mp;
> > + struct xfs_perag *caller_pag = args->pag;
> > int error;
> >
> > /*
> > @@ -3454,9 +3455,11 @@ xfs_bmap_btalloc_at_eof(
> > else
> > args->minalignslop = 0;
> >
> > - args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno));
> > + if (!caller_pag)
> > + args->pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, ap->blkno));
> > error = xfs_alloc_vextent_exact_bno(args, ap->blkno);
> > - xfs_perag_put(args->pag);
> > + if (!caller_pag)
> > + xfs_perag_put(args->pag);
> > if (error)
> > return error;
> >
> > @@ -3482,10 +3485,13 @@ xfs_bmap_btalloc_at_eof(
> > args->minalignslop = 0;
> > }
> >
> > - if (ag_only)
> > + if (ag_only) {
> > error = xfs_alloc_vextent_near_bno(args, ap->blkno);
> > - else
> > + } else {
> > + args->pag = NULL;
> > error = xfs_alloc_vextent_start_ag(args, ap->blkno);
> > + args->pag = caller_pag;
>
> At first glance I wondered if we end up leaking any args->pag set by the
> _iterate_ags function, but I think it's the case that _finish will
> release args->pag and set it back to NULL?
*nod*
> So in effect we're
> preserving the caller's args->pag here, and nothing leaks. In that
> case, I think we should check that assumption:
>
> ASSERT(args->pag == NULL);
> args->pag = caller_pag;
Sure. I'm going to try to remove this conditional caller_pag
situation as we get further down the "per-ags everywhere" hole, but
for the moment this is a necessary quirk...
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
