On Thu, Jun 25, 2020 at 10:23:10AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> ---
> 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@xxxxxx>
