| Message ID | 20190918145538.30376-1-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | xfs: convert inode to extent format after extent merge due to shift | expand |
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.
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 >
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);
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(+)