[PATCH] xfs: convert inode to extent format after extent merge due to shift

[PATCH] xfs: convert inode to extent format after extent merge due to shift

[Brian Foster]

The collapse range operation can merge extents if two newly adjacent
extents are physically contiguous. If the extent count is reduced on
a btree format inode, a change to extent format might be necessary.
This format change currently occurs as a side effect of the file
size update after extents have been shifted for the collapse. This
codepath ultimately calls xfs_bunmapi(), which happens to check for
and execute the format conversion even if there were no blocks
removed from the mapping.

While this ultimately puts the inode into the correct state, the
fact the format conversion occurs in a separate transaction from the
change that called for it is a problem. If an extent shift
transaction commits and the filesystem happens to crash before the
format conversion, the inode fork is left in a corrupted state after
log recovery. The inode fork verifier fails and xfs_repair
ultimately nukes the inode. This problem was originally reproduced
by generic/388.

Similar to how the insert range extent split code handles extent to
btree conversion, update the collapse range extent merge code to
handle btree to extent format conversion in the same transaction
that merges the extents. This ensures that the inode fork format
remains consistent if the filesystem happens to crash in the middle
of a collapse range operation that changes the inode fork format.

Signed-off-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/libxfs/xfs_bmap.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
index 054b4ce30033..eaf2d4250a26 100644
--- a/fs/xfs/libxfs/xfs_bmap.c
+++ b/fs/xfs/libxfs/xfs_bmap.c
@@ -5621,6 +5621,11 @@ xfs_bmse_merge(
 	if (error)
 		return error;
 
+	/* change to extent format if required after extent removal */
+	error = xfs_bmap_btree_to_extents(tp, ip, cur, logflags, whichfork);
+	if (error)
+		return error;
+
 done:
 	xfs_iext_remove(ip, icur, 0);
 	xfs_iext_prev(XFS_IFORK_PTR(ip, whichfork), icur);
-- 
2.20.1

Re: [PATCH] xfs: convert inode to extent format after extent merge due to shift

[Christoph Hellwig]

This looks sensible:

Reviewed-by: Christoph Hellwig <hch@lst.de>

For a while I've been wondering if we can find some good way to take
care of conversion to btree or extent format automatically without all
that boilerplate code, but I haven't come up with a good idea yet.

Re: [PATCH] xfs: convert inode to extent format after extent merge due to shift

[Darrick J. Wong]

On Wed, Sep 18, 2019 at 10:55:38AM -0400, Brian Foster wrote:
> The collapse range operation can merge extents if two newly adjacent
> extents are physically contiguous. If the extent count is reduced on
> a btree format inode, a change to extent format might be necessary.
> This format change currently occurs as a side effect of the file
> size update after extents have been shifted for the collapse. This
> codepath ultimately calls xfs_bunmapi(), which happens to check for
> and execute the format conversion even if there were no blocks
> removed from the mapping.
> 
> While this ultimately puts the inode into the correct state, the
> fact the format conversion occurs in a separate transaction from the
> change that called for it is a problem. If an extent shift
> transaction commits and the filesystem happens to crash before the
> format conversion, the inode fork is left in a corrupted state after
> log recovery. The inode fork verifier fails and xfs_repair
> ultimately nukes the inode. This problem was originally reproduced
> by generic/388.
> 
> Similar to how the insert range extent split code handles extent to
> btree conversion, update the collapse range extent merge code to
> handle btree to extent format conversion in the same transaction
> that merges the extents. This ensures that the inode fork format
> remains consistent if the filesystem happens to crash in the middle
> of a collapse range operation that changes the inode fork format.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>

Seems sensible to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/libxfs/xfs_bmap.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index 054b4ce30033..eaf2d4250a26 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -5621,6 +5621,11 @@ xfs_bmse_merge(
>  	if (error)
>  		return error;
>  
> +	/* change to extent format if required after extent removal */
> +	error = xfs_bmap_btree_to_extents(tp, ip, cur, logflags, whichfork);
> +	if (error)
> +		return error;
> +
>  done:
>  	xfs_iext_remove(ip, icur, 0);
>  	xfs_iext_prev(XFS_IFORK_PTR(ip, whichfork), icur);
> -- 
> 2.20.1
>