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

[Allison Collins <allison.henderson@oracle.com>]

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;
>   }
>   
>