Re: [PATCH v4.2 3/9] xfs: redesign the reflink remap loop to fix blkres depletion crash

Christoph Hellwig
2020-07-01T08:21:02Z

On Thu, Jun 25, 2020 at 10:23:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
>
> The existing reflink remapping loop has some structural problems that
> need addressing:
>
> The biggest problem is that we create one transaction for each extent in
> the source file without accounting for the number of mappings there are
> for the same range in the destination file. In other words, we don't
> know the number of remap operations that will be necessary and we
> therefore cannot guess the block reservation required. On highly
> fragmented filesystems (e.g. ones with active dedupe) we guess wrong,
> run out of block reservation, and fail.
>
> The second problem is that we don't actually use the bmap intents to
> their full potential -- instead of calling bunmapi directly and having
> to deal with its backwards operation, we could call the deferred ops
> xfs_bmap_unmap_extent and xfs_refcount_decrease_extent instead. This
> makes the frontend loop much simpler.
>
> Solve all of these problems by refactoring the remapping loops so that
> we only perform one remapping operation per transaction, and each
> operation only tries to remap a single extent from source to dest.
>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v4.2: move qres conditional to the next patch, rename bmap helper, try
> to clear up some of the smap/dmap confusion
> ---
> fs/xfs/libxfs/xfs_bmap.h | 13 ++
> fs/xfs/xfs_reflink.c | 240 +++++++++++++++++++++++++---------------------
> fs/xfs/xfs_trace.h | 52 +---------
> 3 files changed, 142 insertions(+), 163 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 2b18338d0643..e1bd484e5548 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -158,6 +158,13 @@ static inline int xfs_bmapi_whichfork(int bmapi_flags)
> { BMAP_ATTRFORK, "ATTR" }, \
> { BMAP_COWFORK, "COW" }
>
> +/* Return true if the extent is an allocated extent, written or not. */
> +static inline bool xfs_bmap_is_real_extent(struct xfs_bmbt_irec *irec)
> +{
> + return irec->br_startblock != HOLESTARTBLOCK &&
> + irec->br_startblock != DELAYSTARTBLOCK &&
> + !isnullstartblock(irec->br_startblock);
> +}

I think reusing the previous name is a little dangerous. Maybe rename
this to ..._is_allocated_extent_ ?

>
> /*
> * Return true if the extent is a real, allocated extent, or false if it is a

And not for the previous one: if the real goes away in the name, it
should probably be updated here as well.

The rest looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>