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

From: Chandan Babu R <chandanrlinux@gmail.com>
To: "Darrick J. Wong" <darrick.wong@oracle.com>
Cc: linux-xfs@vger.kernel.org, david@fromorbit.com, hch@infradead.org
Subject: Re: [PATCH V3 10/10] xfs: Check for extent overflow when swapping extents
Date: Tue, 01 Sep 2020 15:15:16 +0530	[thread overview]
Message-ID: <799478968.03WXDaQWZF@garuda> (raw)
In-Reply-To: <20200831162039.GI6096@magnolia>

On Monday 31 August 2020 9:50:39 PM IST Darrick J. Wong wrote:
> On Thu, Aug 20, 2020 at 11:13:49AM +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_inode_fork.h |  6 ++++++
> >  fs/xfs/xfs_bmap_util.c         | 11 +++++++++++
> >  2 files changed, 17 insertions(+)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_inode_fork.h b/fs/xfs/libxfs/xfs_inode_fork.h
> > index d1c675cf803a..4219b01f1034 100644
> > --- a/fs/xfs/libxfs/xfs_inode_fork.h
> > +++ b/fs/xfs/libxfs/xfs_inode_fork.h
> > @@ -100,6 +100,12 @@ 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 e682eecebb1f..7105525dadd5 100644
> > --- a/fs/xfs/xfs_bmap_util.c
> > +++ b/fs/xfs/xfs_bmap_util.c
> > @@ -1375,6 +1375,17 @@ xfs_swap_extent_rmap(
> >  		/* Unmap the old blocks in the source file. */
> >  		while (tirec.br_blockcount) {
> >  			ASSERT(tp->t_firstblock == NULLFSBLOCK);
> > +
> > +			error = xfs_iext_count_may_overflow(ip, XFS_DATA_FORK,
> > +					XFS_IEXT_SWAP_RMAP_CNT);
> > +			if (error)
> > +				goto out;
> > +
> > +			error = xfs_iext_count_may_overflow(tip, XFS_DATA_FORK,
> > +					XFS_IEXT_SWAP_RMAP_CNT);
> 
> Heh, the old swapext code is very gritty.  Two questions--
> 
> If either of irec and uirec describe a hole, why do we need to check for
> an extent count overflow?

Thanks for pointing this out. I missed this. The check for overflow should be
moved later in the code i.e. after reading up the extent mappings. Also, the
overflow check should be made on source file only if the donor file extent
has a valid on-disk mapping and vice versa.

> 
> Second, is the transaction clean at the point where we could goto out?
> I'm pretty sure it is, but if there's a chance we could end up bailing
> out with a dirty transaction, then we need to do this check elsewhere
> such that we don't shut down the filesystem.
> 
> (I'm pretty sure the answer to #2 is "yes", but I thought I'd better
> ask.)

In xfs_swap_extents(), the code between allocating a new transaction and
invoking xfs_swap_extent_rmap() does not peform any operation that can dirty
the transaction.

Inside xfs_swap_extent_rmap(), we invoke xfs_defer_finish() every time we
register deferred operations to exchange extents between two inodes'
forks. xfs_defer_finish() will always return with a clean transaction. So, we
can safely return an error code on detecting an overflow.
> 
> --D
> 
> > +			if (error)
> > +				goto out;
> > +
> >  			trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
> >  
> >  			/* Read extent from the source file */
> 

-- 
chandan