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

Darrick J. Wong
2020-08-31T16:20:45Z

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?

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.)

--D

> + if (error)
> + goto out;
> +
> trace_xfs_swap_extent_rmap_remap_piece(tip, &tirec);
>
> /* Read extent from the source file */
> --
> 2.28.0
>