xfs
[Top] [All Lists]

Re: [PATCH] xfs: trim eofblocks before collapse range

To: xfs@xxxxxxxxxxx
Subject: Re: [PATCH] xfs: trim eofblocks before collapse range
From: Brian Foster <bfoster@xxxxxxxxxx>
Date: Mon, 25 Aug 2014 13:40:20 -0400
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <1408988250-17772-1-git-send-email-bfoster@xxxxxxxxxx>
References: <1408988250-17772-1-git-send-email-bfoster@xxxxxxxxxx>
User-agent: Mutt/1.5.23 (2014-03-12)
On Mon, Aug 25, 2014 at 01:37:30PM -0400, Brian Foster wrote:
> xfs_collapse_file_space() currently writes back the entire file
> undergoing collapse range to settle things down for the extent shift
> algorithm. While this prevents changes to the extent list during the
> collapse operation, the writeback itself is not enough to prevent
> unnecessary collapse failures.
> 
> The current shift algorithm uses the extent index to iterate the in-core
> extent list. If a post-eof delalloc extent persists after the writeback
> (e.g., a prior zero range op where the end of the range aligns with eof
> can separate the post-eof blocks such that they are not written back and
> converted), xfs_bmap_shift_extents() becomes confused over the encoded
> br_startblock value and fails the collapse.
> 
> As with the full writeback, this is a temporary fix until the algorithm
> is improved to cope with a volatile extent list and avoid attempts to
> shift post-eof extents.
> 
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> 
> Hi all,
> 
> This addresses the other fsx failure I've observed related to collapse
> range. It should also be addressed by reworking the algorithm as
> discussed in Dave's full file writeback patch. This patch applies on top
> of that and I think this is more suitable for a near-term -rc drop.
> 
> Brian
> 

Also, appended is a quick reproducer for the fsx sequence that leads to
the failure.

Brian

---8<---

#!/bin/sh

FILE=$1

rm -f $FILE
touch $FILE

# create preallocation
xfs_io -c "pwrite 0 32k" $FILE
xfs_io -c "pwrite 32k 32k" $FILE
xfs_io -c "pwrite 64k 32k" $FILE

# 160k, dangling post-eof extent
xfs_io -c "pwrite 96k 64k" $FILE
xfs_io -c "zero 156k 4k" $FILE

# create more extents
xfs_io -c fsync $FILE
for i in $(seq 0 8192 151152)
do
        xfs_io -c "fpunch $i 4k" $FILE
done

# boom!
xfs_io -c "fcollapse 0 4k" $FILE

---

>  fs/xfs/xfs_bmap_util.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 76b6a29..1707980 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -1471,8 +1471,10 @@ xfs_collapse_file_space(
>       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.
> +      * Writeback the entire file and force remove any post-eof blocks. The
> +      * writeback prevents changes to the extent list via concurrent
> +      * writeback and the eofblocks trim prevents the extent shift algorithm
> +      * from running into a post-eof delalloc extent.
>        *
>        * XXX: This is a temporary fix until the extent shift loop below is
>        * converted to use offsets and lookups within the ILOCK rather than
> @@ -1482,6 +1484,11 @@ xfs_collapse_file_space(
>       error = filemap_write_and_wait(VFS_I(ip)->i_mapping);
>       if (error)
>               return error;
> +     if (xfs_can_free_eofblocks(ip, true)) {
> +             error = xfs_free_eofblocks(mp, ip, false);
> +             if (error)
> +                     return error;
> +     }
>  
>       error = xfs_free_file_space(ip, offset, len);
>       if (error)
> -- 
> 1.8.3.1
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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