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

[Dave Chinner <david@fromorbit.com>]

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?

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.

So I'm kinda clueless as to what this change is actually
doing/fixing because the comments and commit message do not describe
the bug that is being fixed, nor how the change fixes that bug.

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