Re: [PATCH 2/3] xfs: rework insert range into an atomic operation

Allison Collins
2019-12-17T17:05:01Z

On 12/13/19 10:12 AM, Brian Foster wrote:
> The insert range operation uses a unique transaction and ilock cycle
> for the extent split and each extent shift iteration of the overall
> operation. While this works, it is risks racing with other
> operations in subtle ways such as COW writeback modifying an extent
> tree in the middle of a shift operation.
>
> To avoid this problem, make insert range atomic with respect to
> ilock. Hold the ilock across the entire operation, replace the
> individual transactions with a single rolling transaction sequence
> and relog the inode to keep it moving in the log. This guarantees
> that nothing else can change the extent mapping of an inode while
> an insert range operation is in progress.
>
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Ok, this looks good to me. Thanks!

Reviewed by: Allison Collins <allison.henderson@oracle.com>
> ---
> fs/xfs/xfs_bmap_util.c | 32 +++++++++++++-------------------
> 1 file changed, 13 insertions(+), 19 deletions(-)
>
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 829ab1a804c9..555c8b49a223 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1134,47 +1134,41 @@ xfs_insert_file_space(
> if (error)
> return error;
>
> - /*
> - * The extent shifting code works on extent granularity. So, if stop_fsb
> - * is not the starting block of extent, we need to split the extent at
> - * 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);
> + xfs_trans_ijoin(tp, ip, 0);
>
> + /*
> + * The extent shifting code works on extent granularity. So, if stop_fsb
> + * is not the starting block of extent, we need to split the extent at
> + * stop_fsb.
> + */
> error = xfs_bmap_split_extent(tp, ip, stop_fsb);
> if (error)
> goto out_trans_cancel;
>
> - error = xfs_trans_commit(tp);
> - if (error)
> - return error;
> -
> - while (!error && !done) {
> - error = xfs_trans_alloc(mp, &M_RES(mp)->tr_write, 0, 0, 0,
> - &tp);
> + do {
> + error = xfs_trans_roll_inode(&tp, ip);
> if (error)
> - break;
> + goto out_trans_cancel;
>
> - xfs_ilock(ip, XFS_ILOCK_EXCL);
> - xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> error = xfs_bmap_insert_extents(tp, ip, &next_fsb, shift_fsb,
> &done, stop_fsb);
> if (error)
> goto out_trans_cancel;
> + } while (!done);
>
> - error = xfs_trans_commit(tp);
> - }
> -
> + error = xfs_trans_commit(tp);
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> return error;
>
> out_trans_cancel:
> xfs_trans_cancel(tp);
> + xfs_iunlock(ip, XFS_ILOCK_EXCL);
> return error;
> }
>
>