On Fri, Dec 13, 2019 at 12:12:57PM -0500, 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@xxxxxxxxxx>
> ---
> 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);
I'm a little concerned about the livelock potential here, if there are a lot of
other threads that have eaten all the transaction reservation and are trying to
get our ILOCK, while at the same time this thread has the ILOCK and is trying
to roll the transaction to move another extent, having already rolled the
transaction more than logcount times.
I think the extent shifting loop starts with the highest offset mapping and
shifts it up and continues in order of decreasing offset until it gets to
@stop_fsb, correct?
Can we use "alloc trans; ilock; move; commit" for every extent higher than the
one that crosses @stop_fsb, and use "alloc trans; ilock; split; roll;
insert_extents; commit" to deal with that one extent that crosses @stop_fsb?
tr_write pre-reserves enough space to that the roll won't need to get more,
which would eliminate that potential problem, I think.
--D
> + 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;
> }
>
> --
> 2.20.1
>
