Re: [PATCH 1/2] xfs: remove all COW fork extents when remounting readonly

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Dec 08, 2021 at 08:33:16AM +1100, Dave Chinner wrote:
> On Tue, Dec 07, 2021 at 10:35:45AM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > 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.
> > 
> > Fixes: 10ddf64e420f ("xfs: remove leftover CoW reservations when remounting ro")
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_super.c |   14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> Looks good, I went through the analysis yesterday when you mentioned
> it on #xfs. Minor nit below, otherwise:
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks for the review!

> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index e21459f9923a..0c07a4aef3b9 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);
> > +	/*
> > +	 * Clean out all remaining COW staging extents.  This extra step is
> > +	 * done synchronously because the background blockgc worker could have
> > +	 * raced with a reader thread and failed to grab an IOLOCK.  In that
> > +	 * case, the inode could still have post-eof and COW blocks.
> > +	 */
> 
> Rather than describe how inodes might be skipped here, the
> constraint we are operating under should be described. That is:
> 
> 	/*
> 	 * We need to clear out all remaining COW staging extents so
> 	 * that we don't leave inodes requiring modifications during
> 	 * inactivation and reclaim on a read-only mount. We must
> 	 * check and process every inode currently in memory, hence
> 	 * this requires a synchronous inode cache scan to be
> 	 * executed.
> 	 */

I will shorten this to:

	/*
	 * 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.
	 */

--D

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com