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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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@oracle.com>
> > ---
> >  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@oss.sgi.com
> > http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs