Re: [PATCH 1/2] xfs: flush inodegc before swapon

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

On Thu, Feb 06, 2025 at 08:53:59AM +1100, Dave Chinner wrote:
> On Wed, Feb 05, 2025 at 01:16:59PM -0800, Darrick J. Wong wrote:
> > On Thu, Feb 06, 2025 at 08:08:14AM +1100, Dave Chinner wrote:
> > > On Wed, Feb 05, 2025 at 05:28:00PM +0100, Christoph Hellwig wrote:
> > > > Fix the brand new xfstest that tries to swapon on a recently unshared
> > > > file and use the chance to document the other bit of magic in this
> > > > function.
> > > 
> > > You haven't documented the magic at all - I have no clue what the
> > > bug being fixed is nor how adding an inodegc flush fixes anything
> > > to do with swap file activation....
> > > 
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > ---
> > > >  fs/xfs/xfs_aops.c | 18 +++++++++++++++++-
> > > >  1 file changed, 17 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> > > > index 69b8c2d1937d..c792297aa0a3 100644
> > > > --- a/fs/xfs/xfs_aops.c
> > > > +++ b/fs/xfs/xfs_aops.c
> > > > @@ -21,6 +21,7 @@
> > > >  #include "xfs_error.h"
> > > >  #include "xfs_zone_alloc.h"
> > > >  #include "xfs_rtgroup.h"
> > > > +#include "xfs_icache.h"
> > > >  
> > > >  struct xfs_writepage_ctx {
> > > >  	struct iomap_writepage_ctx ctx;
> > > > @@ -685,7 +686,22 @@ xfs_iomap_swapfile_activate(
> > > >  	struct file			*swap_file,
> > > >  	sector_t			*span)
> > > >  {
> > > > -	sis->bdev = xfs_inode_buftarg(XFS_I(file_inode(swap_file)))->bt_bdev;
> > > > +	struct xfs_inode		*ip = XFS_I(file_inode(swap_file));
> > > > +
> > > > +	/*
> > > > +	 * Ensure inode GC has finished to remove unmapped extents, as the
> > > > +	 * reflink bit is only cleared once all previously shared extents
> > > > +	 * are unmapped.  Otherwise swapon could incorrectly fail on a
> > > > +	 * very recently unshare file.
> > > > +	 */
> > > > +	xfs_inodegc_flush(ip->i_mount);
> > > 
> > > The comment doesn't explains what this actually fixes. Inodes that
> > > are processed by inodegc *must* be unreferenced by the VFS, so it
> > > is not clear exactly what this is actually doing.
> > > 
> > > I'm guessing that the test in question is doing something like this:
> > > 
> > > 	file2 = clone(file1)
> > > 	unlink(file1)
> > > 	swapon(file2)
> > > 
> > > and so the swap file activation is racing with the background
> > > inactivation and extent removal of file1?
> > 
> > Yes, I think hch is referring to this:
> > https://lore.kernel.org/fstests/2c9ff99c2bcaec4412b0903e03949d5a3ad0d817.1736783467.git.fdmanana@suse.com/
> > 
> > > But in that case, the extents are being removed from file1, and at
> > > no time does that remove the reflink bit on file2. i.e. even if the
> > > inactivation of file1 results in all the extents in file2 no longer
> > > being shared, that only results in refcountbt updates and it does
> > > not get propagated back to file2's inode. i.e. file2 will still be
> > > marked as a reflink file containing shared extents.
> > 
> > Right, but the (iomap) swapfile activation code only errors out if the
> > filesystem gives it a mapping that is marked as shared.  So the reflink
> > flag isn't relevant here.
> > 
> > How about this for a better comment:
> > 
> > "Ensure inode GC has finished so that unlinked clones of this file have
> > been truncated and inactivated fully.  This is to ensure that walking
> > the swap file does not find any shared extents."
> 
> Even talking about it in terms on "inodegc" seems like
> misdirection to me. Now I understand what this flush is working
> around, it is clear to me that swapon could race the same way with
> any other operation that removes extents from cloned files (e.g.
> hole punch, truncate, etc).
> 
> however, from a user perspective, the only one that matters -right
> now- is unlink because of the deferred processing of extent removal.
> 
> But even that isn't a guarantee - if something else has that cloned
> file open, then the unlinked inode won't be queued for inodegc
> and so swapon will still fail regardless of the inodegc flush.
> 
> Hence I think this needs to explain the race with extent removal and
> cloned files, then explain that the inodegc flush is a workaround
> that applies only to a specific corner case w.r.t. unlinking clones
> before swapon is run. 
> 
> Something like:
> 
> /*
>  * Swap file activation is can race against concurrent shared extent

"..can race..."

>  * removal in files that have been cloned. If this happens,
>  * iomap_swapfile_iter() can fail because it encountered a shared
>  * extent even though an operation is in progress to remove those
>  * shared extents.
>  *
>  * This race becomes problematic when we defer extent removal
>  * operations beyond the end of a syscall (i.e. use async background
>  * processing algorithms). Users think the extents are no longer
>  * shared, but iomap_swapfile_iter() still sees them as shared
>  * because the refcountbt entries for the extents being removed have
>  * not yet been updated. Hence the swapon call fails unexpectedly.
>  *
>  * The race condition is currently most obvious from the unlink()
>  * operation as extent removal is deferred until after the last
>  * reference to the inode goes away. We then process the extent
>  * removal asynchronously, hence triggers the "syscall completed but
>  * work not done" condition mentioned above. To close this race
>  * window, we need to flush any pending inodegc operations to ensure
>  * they have updated the refcountbt records before we try to map the
>  * swapfile.

Yes, this is a good explanation.

>  */
> 
> This explains the race condition we are working around, and it gives
> enough information to document that any other refcountbt updates we
> defer to background processing (either removals or inserts!) are
> going to need to be synchronised here.

There shouldn't be any refcount increments involving the swapfile
because the mm already took IOLOCK_EXCL for us.  But yes, there could
someday be more asynchronous decrements elsewhere in the filesystem.

--D

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