xfs
[Top] [All Lists]

Re: [PATCH 6/6] xfs: xfs_file_collapse_range is delalloc challenged

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 6/6] xfs: xfs_file_collapse_range is delalloc challenged
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Thu, 21 Aug 2014 09:09:25 -0400
Cc: xfs@xxxxxxxxxxx, clm@xxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1408597754-13526-7-git-send-email-david@xxxxxxxxxxxxx>
References: <1408597754-13526-1-git-send-email-david@xxxxxxxxxxxxx> <1408597754-13526-7-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Thu, Aug 21, 2014 at 03:09:14PM +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> If we have delalloc extents on a file before we run a collapse range
> opertaion, we sync the range that we are going to collapse to
> convert delalloc extents in that region to real extents to simplify
> the shift operation.
> 
> However, the shift operation then assumes that the extent list is
> not going to change as it iterates over the extent list moving
> things about. Unfortunately, this isn't true because we can't hold
> the ILOCK over all the operations. We can prevent new IO from
> modifying the extent list by holding the IOLOCK, but that doesn't
> prevent writeback from running....
> 
> And when writeback runs, it can convert delalloc extents is the
> range of the file prior to the region being collapsed, and this
> changes the indexes of all the extents in the file. That causes the
> collapse range operation to Go Bad.
> 
> The right fix is to rewrite the extent shift operation not to be
> dependent on the extent list not changing across the entire
> operation, but this is a fairly significant piece of work to do.
> Hence, as a short-term workaround for the problem, sync the entire
> file before starting a collapse operation to remove all delalloc
> ranges from the file and so avoid the problem of concurrent
> writeback changing the extent list.
> 
> Diagnosed-and-Reported-by: Brian Foster <bfoster@xxxxxxxxxx>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---

Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>

>  fs/xfs/xfs_bmap_util.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index c53cc03..035041d 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1460,6 +1460,19 @@ xfs_collapse_file_space(
>       start_fsb = XFS_B_TO_FSB(mp, offset + len);
>       shift_fsb = XFS_B_TO_FSB(mp, len);
>  
> +     /*
> +      * writeback the entire file to prevent concurrent writeback of ranges
> +      * outside the collapsing region from changing the extent list.
> +      *
> +      * XXX: This is a temporary fix until the extent shift loop below is
> +      * converted to use offsets and lookups within the ILOCK rather than
> +      * carrying around the index into the extent list for the next
> +      * iteration.
> +      */
> +     error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
> +     if (error)
> +             return error;
> +
>       error = xfs_free_file_space(ip, offset, len);
>       if (error)
>               return error;
> -- 
> 2.0.0
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

<Prev in Thread] Current Thread [Next in Thread>