On 09 Dec 2021 at 04:45, Darrick J. Wong wrote: > From: Darrick J. Wong <djwong@xxxxxxxxxx> > > As part of multiple customer escalations due to file data corruption > after copy on write operations, I wrote some fstests that use fsstress > to hammer on COW to shake things loose. Regrettably, I caught some > filesystem shutdowns due to incorrect rmap operations with the following > loop: > > mount <filesystem> # (0) > fsstress <run only readonly ops> & # (1) > while true; do > fsstress <run all ops> > mount -o remount,ro # (2) > fsstress <run only readonly ops> > mount -o remount,rw # (3) > done > > When (2) happens, notice that (1) is still running. xfs_remount_ro will > call xfs_blockgc_stop to walk the inode cache to free all the COW > extents, but the blockgc mechanism races with (1)'s reader threads to > take IOLOCKs and loses, which means that it doesn't clean them all out. > Call such a file (A). > > When (3) happens, xfs_remount_rw calls xfs_reflink_recover_cow, which > walks the ondisk refcount btree and frees any COW extent that it finds. > This function does not check the inode cache, which means that incore > COW forks of inode (A) is now inconsistent with the ondisk metadata. If > one of those former COW extents are allocated and mapped into another > file (B) and someone triggers a COW to the stale reservation in (A), A's > dirty data will be written into (B) and once that's done, those blocks > will be transferred to (A)'s data fork without bumping the refcount. > > The results are catastrophic -- file (B) and the refcount btree are now > corrupt. Solve this race by forcing the xfs_blockgc_free_space to run > synchronously, which causes xfs_icwalk to return to inodes that were > skipped because the blockgc code couldn't take the IOLOCK. This is safe > to do here because the VFS has already prohibited new writer threads. > Looks good. Reviewed-by: Chandan Babu R <chandan.babu@xxxxxxxxxx> > Fixes: 10ddf64e420f ("xfs: remove leftover CoW reservations when remounting ro") > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx> > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> > --- > fs/xfs/xfs_super.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index e21459f9923a..778b57b1f020 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1765,7 +1765,10 @@ static int > xfs_remount_ro( > struct xfs_mount *mp) > { > - int error; > + struct xfs_icwalk icw = { > + .icw_flags = XFS_ICWALK_FLAG_SYNC, > + }; > + int error; > > /* > * Cancel background eofb scanning so it cannot race with the final > @@ -1773,8 +1776,13 @@ xfs_remount_ro( > */ > xfs_blockgc_stop(mp); > > - /* Get rid of any leftover CoW reservations... */ > - error = xfs_blockgc_free_space(mp, NULL); > + /* > + * Clear out all remaining COW staging extents and speculative post-EOF > + * preallocations so that we don't leave inodes requiring inactivation > + * cleanups during reclaim on a read-only mount. We must process every > + * cached inode, so this requires a synchronous cache scan. > + */ > + error = xfs_blockgc_free_space(mp, &icw); > if (error) { > xfs_force_shutdown(mp, SHUTDOWN_CORRUPT_INCORE); > return error; -- chandan