xfs
[Top] [All Lists]

Re: [PATCH 048/119] xfs: don't update rmapbt when fixing agfl

To: Brian Foster <bfoster@xxxxxxxxxx>
Subject: Re: [PATCH 048/119] xfs: don't update rmapbt when fixing agfl
From: "Darrick J. Wong" <darrick.wong@xxxxxxxxxx>
Date: Mon, 18 Jul 2016 08:53:49 -0700
Cc: david@xxxxxxxxxxxxx, linux-fsdevel@xxxxxxxxxxxxxxx, vishal.l.verma@xxxxxxxxx, xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20160718133434.GG27380@xxxxxxxxxxxxxxx>
References: <146612627129.12839.3827886950949809165.stgit@xxxxxxxxxxxxxxxx> <146612657955.12839.6406507864344687918.stgit@xxxxxxxxxxxxxxxx> <20160718133434.GG27380@xxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.24 (2015-08-30)
On Mon, Jul 18, 2016 at 09:34:34AM -0400, Brian Foster wrote:
> On Thu, Jun 16, 2016 at 06:22:59PM -0700, Darrick J. Wong wrote:
> > Allow a caller of xfs_alloc_fix_freelist to disable rmapbt updates
> > when fixing the AG freelist.  xfs_repair needs this during phase 5
> > to be able to adjust the freelist while it's reconstructing the rmap
> > btree; the missing entries will be added back at the very end of
> > phase 5 once the AGFL contents settle down.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
> >  fs/xfs/libxfs/xfs_alloc.c |   40 ++++++++++++++++++++++++++--------------
> >  fs/xfs/libxfs/xfs_alloc.h |    3 +++
> >  2 files changed, 29 insertions(+), 14 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> > index 4c8ffd4..6eabab1 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.c
> > +++ b/fs/xfs/libxfs/xfs_alloc.c
> > @@ -2092,26 +2092,38 @@ xfs_alloc_fix_freelist(
> >      * anything other than extra overhead when we need to put more blocks
> >      * back on the free list? Maybe we should only do this when space is
> >      * getting low or the AGFL is more than half full?
> > +    *
> > +    * The NOSHRINK flag prevents the AGFL from being shrunk if it's too
> > +    * big; the NORMAP flag prevents AGFL expand/shrink operations from
> > +    * updating the rmapbt.  Both flags are used in xfs_repair while we're
> > +    * rebuilding the rmapbt, and neither are used by the kernel.  They're
> > +    * both required to ensure that rmaps are correctly recorded for the
> > +    * regenerated AGFL, bnobt, and cntbt.  See repair/phase5.c and
> > +    * repair/rmap.c in xfsprogs for details.
> >      */
> > -   xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
> > -   while (pag->pagf_flcount > need) {
> > -           struct xfs_buf  *bp;
> > +   memset(&targs, 0, sizeof(targs));
> > +   if (!(flags & XFS_ALLOC_FLAG_NOSHRINK)) {
> > +           if (!(flags & XFS_ALLOC_FLAG_NORMAP))
> > +                   xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
> 
> Can we get away with setting targs.oinfo once rather than here and
> below? If so, I think something like the following might clean this up a
> bit and save some indentation:
> 
>       memset(targs, 0, ...);
>       if (!(flags & NORMAP))
>               xfs_rmap_ag_owner(...);
>       while (!(flags & NOSHRINK) &&
>              flcount > need) {
>               ...
>       }
>       ...
> 
> Hm?

Yeah, I think that is the case.  In the end it'll look like:

memset(targs, 0...);
if (flags & NORMAP)
        xfs_rmap_skip_update(&targs.oinfo);
else
        xfs_rmap_ag_owner(&targs.oinfo...);
while (!(flags & NOSHRINK) && flcount > need) {
        ...
}

--D

> 
> Brian
> 
> > +           while (pag->pagf_flcount > need) {
> > +                   struct xfs_buf  *bp;
> >  
> > -           error = xfs_alloc_get_freelist(tp, agbp, &bno, 0);
> > -           if (error)
> > -                   goto out_agbp_relse;
> > -           error = xfs_free_ag_extent(tp, agbp, args->agno, bno, 1,
> > -                                      &targs.oinfo, 1);
> > -           if (error)
> > -                   goto out_agbp_relse;
> > -           bp = xfs_btree_get_bufs(mp, tp, args->agno, bno, 0);
> > -           xfs_trans_binval(tp, bp);
> > +                   error = xfs_alloc_get_freelist(tp, agbp, &bno, 0);
> > +                   if (error)
> > +                           goto out_agbp_relse;
> > +                   error = xfs_free_ag_extent(tp, agbp, args->agno, bno, 1,
> > +                                              &targs.oinfo, 1);
> > +                   if (error)
> > +                           goto out_agbp_relse;
> > +                   bp = xfs_btree_get_bufs(mp, tp, args->agno, bno, 0);
> > +                   xfs_trans_binval(tp, bp);
> > +           }
> >     }
> >  
> > -   memset(&targs, 0, sizeof(targs));
> >     targs.tp = tp;
> >     targs.mp = mp;
> > -   xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
> > +   if (!(flags & XFS_ALLOC_FLAG_NORMAP))
> > +           xfs_rmap_ag_owner(&targs.oinfo, XFS_RMAP_OWN_AG);
> >     targs.agbp = agbp;
> >     targs.agno = args->agno;
> >     targs.alignment = targs.minlen = targs.prod = targs.isfl = 1;
> > diff --git a/fs/xfs/libxfs/xfs_alloc.h b/fs/xfs/libxfs/xfs_alloc.h
> > index 7b6c66b..7b9e67e 100644
> > --- a/fs/xfs/libxfs/xfs_alloc.h
> > +++ b/fs/xfs/libxfs/xfs_alloc.h
> > @@ -54,6 +54,9 @@ typedef unsigned int xfs_alloctype_t;
> >   */
> >  #define    XFS_ALLOC_FLAG_TRYLOCK  0x00000001  /* use trylock for buffer 
> > locking */
> >  #define    XFS_ALLOC_FLAG_FREEING  0x00000002  /* indicate caller is 
> > freeing extents*/
> > +#define    XFS_ALLOC_FLAG_NORMAP   0x00000004  /* don't modify the rmapbt 
> > */
> > +#define    XFS_ALLOC_FLAG_NOSHRINK 0x00000008  /* don't shrink the 
> > freelist */
> > +
> >  
> >  /*
> >   * Argument structure for xfs_alloc routines.
> > 
> > _______________________________________________
> > xfs mailing list
> > xfs@xxxxxxxxxxx
> > http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>