Re: [PATCH 1/3] xfs: open code insert range extent split helper

Allison Collins
2019-12-17T17:02:50Z

On 12/13/19 10:12 AM, Brian Foster wrote:
> The insert range operation currently splits the extent at the target
> offset in a separate transaction and lock cycle from the one that
> shifts extents. In preparation for reworking insert range into an
> atomic operation, lift the code into the caller so it can be easily
> condensed to a single rolling transaction and lock cycle and
> eliminate the helper. No functional changes.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Looks ok to me.

Reviewed by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 32 ++------------------------------
> fs/xfs/libxfs/xfs_bmap.h | 3 ++-
> fs/xfs/xfs_bmap_util.c | 14 +++++++++++++-
> 3 files changed, 17 insertions(+), 32 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index a9ad1f991ba3..2bba0f983e4f 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -6021,8 +6021,8 @@ xfs_bmap_insert_extents(
> * @split_fsb is a block where the extents is split. If split_fsb lies in a
> * hole or the first block of extents, just return 0.
> */
> -STATIC int
> -xfs_bmap_split_extent_at(
> +int
> +xfs_bmap_split_extent(
> struct xfs_trans *tp,
> struct xfs_inode *ip,
> xfs_fileoff_t split_fsb)
> @@ -6138,34 +6138,6 @@ xfs_bmap_split_extent_at(
> return error;
> }
>
> -int
> -xfs_bmap_split_extent(
> - struct xfs_inode *ip,
> - xfs_fileoff_t split_fsb)
> -{
> - struct xfs_mount *mp = ip->i_mount;
> - struct xfs_trans *tp;
> - int error;
> -
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write,
> - XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
> - if (error)
> - return error;
> -
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> -
> - error = xfs_bmap_split_extent_at(tp, ip, split_fsb);
> - if (error)
> - goto out;
> -
> - return xfs_trans_commit(tp);
> -
> -out:
> - xfs_trans_cancel(tp);
> - return error;
> -}
> -
> /* Deferred mapping is only for real extents in the data fork. */
> static bool
> xfs_bmap_is_update_needed(
> diff --git a/fs/xfs/libxfs/xfs_bmap.h b/fs/xfs/libxfs/xfs_bmap.h
> index 14d25e0b7d9c..f3259ad5c22c 100644
> --- a/fs/xfs/libxfs/xfs_bmap.h
> +++ b/fs/xfs/libxfs/xfs_bmap.h
> @@ -222,7 +222,8 @@ int xfs_bmap_can_insert_extents(struct xfs_inode *ip, xfs_fileoff_t off,
> int xfs_bmap_insert_extents(struct xfs_trans *tp, struct xfs_inode *ip,
> xfs_fileoff_t *next_fsb, xfs_fileoff_t offset_shift_fsb,
> bool *done, xfs_fileoff_t stop_fsb);
> -int xfs_bmap_split_extent(struct xfs_inode *ip, xfs_fileoff_t split_offset);
> +int xfs_bmap_split_extent(struct xfs_trans *tp, struct xfs_inode *ip,
> + xfs_fileoff_t split_offset);
> int xfs_bmapi_reserve_delalloc(struct xfs_inode *ip, int whichfork,
> xfs_fileoff_t off, xfs_filblks_t len, xfs_filblks_t prealloc,
> struct xfs_bmbt_irec *got, struct xfs_iext_cursor *cur,
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 2efd78a9719e..829ab1a804c9 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1139,7 +1139,19 @@ xfs_insert_file_space(
> * is not the starting block of extent, we need to split the extent at
> * stop_fsb.
> */
> - error = xfs_bmap_split_extent(ip, stop_fsb);
> + error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write,
> + XFS_DIOSTRAT_SPACE_RES(mp, 0), 0, 0, &tp);
> + if (error)
> + return error;
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +
> + error = xfs_bmap_split_extent(tp, ip, stop_fsb);
> + if (error)
> + goto out_trans_cancel;
> +
> + error = xfs_trans_commit(tp);
> if (error)
> return error;
>
>