Re: [PATCH 05/21] xfs: add BMAPI_NORMAP flag to perform block remapping without updating rmapbt

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

On Fri, Apr 06, 2018 at 09:07:22AM +1000, Dave Chinner wrote:
> On Mon, Apr 02, 2018 at 12:56:47PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Add a new flag, XFS_BMAPI_NORMAP, which will perform file block
> > remapping without updating the rmapbt.  This will be used by the repair
> > code to reconstruct bmbts from the rmapbt, in which case we don't want
> > the rmapbt update.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c |   25 ++++++++++++++++---------
> >  fs/xfs/libxfs/xfs_bmap.h |    6 +++++-
> >  2 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 3b03d88..519ef9c 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1998,9 +1998,12 @@ xfs_bmap_add_extent_delay_real(
> >  	}
> >  
> >  	/* add reverse mapping */
> > -	error = xfs_rmap_map_extent(mp, bma->dfops, bma->ip, whichfork, new);
> > -	if (error)
> > -		goto done;
> > +	if (!(bma->flags & XFS_BMAPI_NORMAP)) {
> > +		error = xfs_rmap_map_extent(mp, bma->dfops, bma->ip,
> > +				whichfork, new);
> > +		if (error)
> > +			goto done;
> > +	}
> 
> Double negatives are a bit hard to parse (not NORMAP) but I don't
> have a better idea for this.

Yeah.  I'll update the comment to read:

/* add reverse mapping unless caller opted out */

to hammer on the point that updating rmap is the default, and callers
have to explicitly turn that off.

> 
> ....
> 
> > @@ -4119,7 +4125,8 @@ xfs_bmapi_allocate(
> >  	else
> >  		error = xfs_bmap_add_extent_hole_real(bma->tp, bma->ip,
> >  				whichfork, &bma->icur, &bma->cur, &bma->got,
> > -				bma->firstblock, bma->dfops, &bma->logflags);
> > +				bma->firstblock, bma->dfops, &bma->logflags,
> > +				bma->flags);
> 
> Ob: The number of function parameters is starting to get out of hand
> again :/

I'd noticed.  TBH I've wondered why not just move whichfork into struct
xfs_bmalloca and then we can get rid of a lot of parameters and
whichfork passing?  Oh, right, because xfs_bmalloca.flags already has
XFS_BMAPI_{ATTR,COW}FORK so even that's unnecessary.

On the other hand bmapi_remap also calls _add_extent_hole_real, and it
(currently) doesn't create a xfs_bmalloca and maybe it's silly to make
it do that?

But that can be a cleanup patch for 4.18...

--D

> Regardless,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html