On Wed, Feb 09, 2022 at 03:36:55PM +0800, Gao Xiang wrote:
> Previously, xfs_reflink_end_cow_extent() will unconditionally unmap
> the corresponding old extent and remap an extent from COW fork.
> However, it seems somewhat ineffective since the old bmbt records can
> be directly updated for many cases instead.
>
> This patch uses introduced xfs_bmap_update_extent_real() in the
> previous patch for most extent inclusive cases or it will fall back
> to the old way if such replacement is not possible.
>
> Actually, we're planing to use a modified alway-cow like atomic write
> approach internally, therefore it'd be nice to do some optimization
> to reduce some metadata overhead.
>
> Signed-off-by: Gao Xiang <hsiangkao@xxxxxxxxxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 117 ++++++++++++++++++++++++++++++++++++---
> fs/xfs/libxfs/xfs_bmap.h | 3 +
> fs/xfs/xfs_reflink.c | 19 +------
> 3 files changed, 112 insertions(+), 27 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a10476dee701..0e132f811f7a 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5880,6 +5880,114 @@ xfs_bmap_collapse_extents(
> 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;
> +}
> +
> +/* del is an extent from COW fork */
> +int
> +xfs_bremapi_from_cowfork(
> + struct xfs_trans *tp,
> + struct xfs_inode *ip,
> + struct xfs_bmbt_irec *icow)
> +{
> + int error;
> + xfs_filblks_t rlen;
> +
> + /* Use the old (unmap-remap) way for real-time inodes instead */
> + if (!XFS_IS_REALTIME_INODE(ip) && xfs_bmap_is_update_needed(icow)) {
When would be be remapping a realtime file with a COW fork?
> + xfs_fileoff_t start, end, max_len;
> + struct xfs_bmbt_irec got;
> + struct xfs_iext_cursor icur;
> + struct xfs_btree_cur *cur = NULL;
> + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> + int logflags = 0;
> +
> + error = xfs_iread_extents(tp, ip, XFS_DATA_FORK);
> + if (error)
> + return error;
> +
> + max_len = xfs_refcount_max_unmap(tp->t_log_res);
> + if (max_len < icow->br_blockcount) {
> + icow->br_startoff += icow->br_blockcount - max_len;
> + icow->br_startblock += icow->br_blockcount - max_len;
> + icow->br_blockcount = max_len;
> + }
> +
> + end = icow->br_startoff + icow->br_blockcount;
> + if (!xfs_iext_count(ifp) || !xfs_iext_lookup_extent_before(ip,
> + ifp, &end, &icur, &got) ||
> + isnullstartblock(got.br_startblock) ||
> + icow->br_startoff + icow->br_blockcount > got.br_startoff +
> + got.br_blockcount) {
> + error = -EAGAIN;
> + } else {
> + end = icow->br_startoff + icow->br_blockcount;
> + start = XFS_FILEOFF_MAX(icow->br_startoff,
> + got.br_startoff);
> + ASSERT(start < end);
> +
> + /* Trim the extent to what we need */
> + xfs_trim_extent(icow, start, end - start);
> + xfs_trim_extent(&got, start, end - start);
> +
> + if (ifp->if_format == XFS_DINODE_FMT_BTREE) {
> + cur = xfs_bmbt_init_cursor(tp->t_mountp, tp, ip,
> + XFS_DATA_FORK);
> + cur->bc_ino.flags = 0;
> + }
> +
> + /*
> + * Free the CoW orphan record (it should be done here
> + * before updating extent due to rmapbt update)
> + */
> + xfs_refcount_free_cow_extent(tp, icow->br_startblock,
> + icow->br_blockcount);
> +
> + xfs_bmap_update_extent_real(tp, ip, XFS_DATA_FORK,
> + &icur, &cur, icow, &logflags, false);
Hmm... are you directly updating the data fork mapping record from the
COW fork mapping record? Is the performance advantage you mentioned
earlier a result of this code no longer logging a bmap map intent item
and reducing the transaction roll count by one?
--D
> +
> + /* Free previous referenced space */
> + xfs_refcount_decrease_extent(tp, &got);
> +
> + trace_xfs_reflink_cow_remap(ip, icow);
> + error = 0;
> + }
> + if (cur)
> + xfs_btree_del_cursor(cur, 0);
> + if (logflags)
> + xfs_trans_log_inode(tp, ip, logflags);
> + if (!error)
> + return 0;
> + }
> +
> + rlen = icow->br_blockcount;
> + error = __xfs_bunmapi(tp, ip, icow->br_startoff, &rlen, 0, 1);
> + if (error)
> + return error;
> +
> + /* Trim the extent to whatever got unmapped. */
> + xfs_trim_extent(icow, icow->br_startoff + rlen,
> + icow->br_blockcount - rlen);
> + /* Free the CoW orphan record. */
> + xfs_refcount_free_cow_extent(tp, icow->br_startblock,
> + icow->br_blockcount);
> +
> + /* Map the new blocks into the data fork. */
> + xfs_bmap_map_extent(tp, ip, icow);
> +
> + /* Charge this new data fork mapping to the on-disk quota. */
> + xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
> + (long)icow->br_blockcount);
> + trace_xfs_reflink_cow_remap(ip, icow);
> + return 0;
> +}
> +
> /* Make sure we won't be right-shifting an extent past the maximum bound. */
> int
> xfs_bmap_can_insert_extents(
> @@ -6123,15 +6231,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(
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index c52ff94786e2..9da1cff41c1c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -220,6 +220,9 @@ int xfs_bmap_update_extent_real(struct xfs_trans *tp,
> struct xfs_inode *ip, int whichfork,
> struct xfs_iext_cursor *icur, struct xfs_btree_cur **curp,
> struct xfs_bmbt_irec *new, int *logflagsp, bool convert);
> +int
> +xfs_bremapi_from_cowfork(struct xfs_trans *tp, struct xfs_inode *ip,
> + struct xfs_bmbt_irec *icow);
>
> enum xfs_bmap_intent_type {
> XFS_BMAP_MAP = 1,
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index 276387a6a85d..75bd2e03cd5b 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -590,7 +590,6 @@ xfs_reflink_end_cow_extent(
> struct xfs_mount *mp = ip->i_mount;
> struct xfs_trans *tp;
> struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, XFS_COW_FORK);
> - xfs_filblks_t rlen;
> unsigned int resblks;
> int error;
>
> @@ -651,26 +650,10 @@ xfs_reflink_end_cow_extent(
> goto out_cancel;
> }
>
> - /* Unmap the old blocks in the data fork. */
> - rlen = del.br_blockcount;
> - error = __xfs_bunmapi(tp, ip, del.br_startoff, &rlen, 0, 1);
> + error = xfs_bremapi_from_cowfork(tp, ip, &del);
> if (error)
> goto out_cancel;
>
> - /* Trim the extent to whatever got unmapped. */
> - xfs_trim_extent(&del, del.br_startoff + rlen, del.br_blockcount - rlen);
> - trace_xfs_reflink_cow_remap(ip, &del);
> -
> - /* Free the CoW orphan record. */
> - xfs_refcount_free_cow_extent(tp, del.br_startblock, del.br_blockcount);
> -
> - /* Map the new blocks into the data fork. */
> - xfs_bmap_map_extent(tp, ip, &del);
> -
> - /* Charge this new data fork mapping to the on-disk quota. */
> - xfs_trans_mod_dquot_byino(tp, ip, XFS_TRANS_DQ_DELBCOUNT,
> - (long)del.br_blockcount);
> -
> /* Remove the mapping from the CoW fork. */
> xfs_bmap_del_extent_cow(ip, &icur, &got, &del);
>
> --
> 2.24.4
>
