Re: [PATCH V4 09/10] xfs: Check for extent overflow when swapping extents

[Chandan Babu R <chandanrlinux@gmail.com>]

On Friday 18 September 2020 9:14:45 PM IST Darrick J. Wong wrote:
> On Fri, Sep 18, 2020 at 03:17:58PM +0530, Chandan Babu R wrote:
> > Removing an initial range of source/donor file's extent and adding a new
> > extent (from donor/source file) in its place will cause extent count to
> > increase by 1.
> > 
> > Signed-off-by: Chandan Babu R <chandanrlinux@gmail.com>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c       | 18 +++++++++---------
> >  fs/xfs/libxfs/xfs_bmap.h       |  1 +
> >  fs/xfs/libxfs/xfs_inode_fork.h |  7 +++++++
> >  fs/xfs/xfs_bmap_util.c         | 17 +++++++++++++++++
> >  4 files changed, 34 insertions(+), 9 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 51c2d2690f05..9c665e379dfc 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -6104,15 +6104,6 @@ xfs_bmap_split_extent(
> >  	return error;
> >  }
> >  
> > -/* Deferred mapping is only for real extents in the data fork. */
> > -static bool
> > -xfs_bmap_is_update_needed(
> > -	struct xfs_bmbt_irec	*bmap)
> > -{
> > -	return  bmap->br_startblock != HOLESTARTBLOCK &&
> > -		bmap->br_startblock != DELAYSTARTBLOCK;
> > -}
> > -
> >  /* Record a bmap intent. */
> >  static int
> >  __xfs_bmap_add(
> > @@ -6144,6 +6135,15 @@ __xfs_bmap_add(
> >  	return 0;
> >  }
> >  
> > +/* Deferred mapping is only for real extents in the data fork. */
> > +bool
> > +xfs_bmap_is_update_needed(
> > +	struct xfs_bmbt_irec	*bmap)
> > +{
> > +	return  bmap->br_startblock != HOLESTARTBLOCK &&
> > +		bmap->br_startblock != DELAYSTARTBLOCK;
> > +}
> 
> I think the predicate you want below is xfs_bmap_is_real_extent().

Yes, that is indeed correct. I will fix this one too.

> 
> (I think that mostly because I'm going to kill this predicate entirely
> in a patch for the next cycle, because it is redundant and
> _is_real_extent is a better name.)
> 
> --D
> 
> > +
> >  /* Map an extent into a file. */
> >  void
> >  xfs_bmap_map_extent(
> > diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> > index e1bd484e5548..60fbe184d5f4 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.h
> > +++ b/fs/xfs/libxfs/xfs_bmap.h
> > @@ -263,6 +263,7 @@ struct xfs_bmap_intent {
> >  	struct xfs_bmbt_irec			bi_bmap;
> >  };
> >  
> > +bool	xfs_bmap_is_update_needed(struct xfs_bmbt_irec *bmap);
> >  int	xfs_bmap_finish_one(struct xfs_trans *tp, struct xfs_inode *ip,
> >  		enum xfs_bmap_intent_type type, int whichfork,
> >  		xfs_fileoff_t startoff, xfs_fsblock_t startblock,
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index ded3c1b56c94..837c01595439 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -102,6 +102,13 @@ struct xfs_ifork {
> >  #define XFS_IEXT_REFLINK_REMAP_CNT(smap_real, dmap_written) \
> >  	(((smap_real) ? 1 : 0) + ((dmap_written) ? 1 : 0))
> >  
> > +/*
> > + * Removing an initial range of source/donor file's extent and adding a new
> > + * extent (from donor/source file) in its place will cause extent count to
> > + * increase by 1.
> > + */
> > +#define XFS_IEXT_SWAP_RMAP_CNT		(1)
> > +
> >  /*
> >   * Fork handling.
> >   */
> > diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> > index 0776abd0103c..542f990247c4 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -28,6 +28,7 @@
> >  #include "xfs_icache.h"
> >  #include "xfs_iomap.h"
> >  #include "xfs_reflink.h"
> > +#include "xfs_bmap.h"
> >  
> >  /* Kernel only BMAP related definitions and functions */
> >  
> > @@ -1407,6 +1408,22 @@ xfs_swap_extent_rmap(
> >  					irec.br_blockcount);
> >  			trace_xfs_swap_extent_rmap_remap_piece(tip, &uirec);
> >  
> > +			if (xfs_bmap_is_update_needed(&uirec)) {
> > +				error = xfs_iext_count_may_overflow(ip,
> > +						XFS_DATA_FORK,
> > +						XFS_IEXT_SWAP_RMAP_CNT);
> > +				if (error)
> > +					goto out;
> > +			}
> > +
> > +			if (xfs_bmap_is_update_needed(&irec)) {
> > +				error = xfs_iext_count_may_overflow(tip,
> > +						XFS_DATA_FORK,
> > +						XFS_IEXT_SWAP_RMAP_CNT);
> > +				if (error)
> > +					goto out;
> > +			}
> > +
> >  			/* Remove the mapping from the donor file. */
> >  			xfs_bmap_unmap_extent(tp, tip, &uirec);
> >  
> 


-- 
chandan