fix swapon for recently unshared blocks

fix swapon for recently unshared blocks

[Christoph Hellwig]

Hi all,

the first patch fixes the recently added generic/370, and the
second one does a bit of naming cleanup in the area.

Diffstat:
 xfs_aops.c |   24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

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

[Christoph Hellwig]

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.

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);
+
+	/*
+	 * Direct the swap code to the correct block device when this file
+	 * sits on the RT device.
+	 */
+	sis->bdev = xfs_inode_buftarg(ip)->bt_bdev;
+
 	return iomap_swapfile_activate(sis, swap_file, span,
 			&xfs_read_iomap_ops);
 }
-- 
2.45.2

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

[Darrick J. Wong]

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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Cc: <stable@vger.kernel.org> # v5.19
Fixes: 5e672cd69f0a53 ("xfs: introduce xfs_inodegc_push()")

Perhaps?  This isn't really a hard brokenness, but it could surprise a
user somewhere.

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

                         unshared

> +	 */
> +	xfs_inodegc_flush(ip->i_mount);
> +
> +	/*
> +	 * Direct the swap code to the correct block device when this file
> +	 * sits on the RT device.
> +	 */
> +	sis->bdev = xfs_inode_buftarg(ip)->bt_bdev;

With all that amended,
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> +
>  	return iomap_swapfile_activate(sis, swap_file, span,
>  			&xfs_read_iomap_ops);
>  }
> -- 
> 2.45.2
> 
> 

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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."

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

I wasn't too clear on how the xfs_inode_buftarg() call was magic, it
just had a lot of parentheses.

--D

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

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

[Dave Chinner]

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

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.

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

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

[Darrick J. Wong]

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
> 

[PATCH 2/2] xfs: rename xfs_iomap_swapfile_activate to xfs_vm_swap_activate

[Christoph Hellwig]

Match the method name and the naming convention or address_space
operations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_aops.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index c792297aa0a3..fc492a1724c3 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -681,7 +681,7 @@ xfs_vm_readahead(
 }
 
 static int
-xfs_iomap_swapfile_activate(
+xfs_vm_swap_activate(
 	struct swap_info_struct		*sis,
 	struct file			*swap_file,
 	sector_t			*span)
@@ -717,11 +717,11 @@ const struct address_space_operations xfs_address_space_operations = {
 	.migrate_folio		= filemap_migrate_folio,
 	.is_partially_uptodate  = iomap_is_partially_uptodate,
 	.error_remove_folio	= generic_error_remove_folio,
-	.swap_activate		= xfs_iomap_swapfile_activate,
+	.swap_activate		= xfs_vm_swap_activate,
 };
 
 const struct address_space_operations xfs_dax_aops = {
 	.writepages		= xfs_dax_writepages,
 	.dirty_folio		= noop_dirty_folio,
-	.swap_activate		= xfs_iomap_swapfile_activate,
+	.swap_activate		= xfs_vm_swap_activate,
 };
-- 
2.45.2

Re: [PATCH 2/2] xfs: rename xfs_iomap_swapfile_activate to xfs_vm_swap_activate

[Darrick J. Wong]

On Wed, Feb 05, 2025 at 05:28:01PM +0100, Christoph Hellwig wrote:
> Match the method name and the naming convention or address_space
> operations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_aops.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index c792297aa0a3..fc492a1724c3 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -681,7 +681,7 @@ xfs_vm_readahead(
>  }
>  
>  static int
> -xfs_iomap_swapfile_activate(
> +xfs_vm_swap_activate(

Hee hee.
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

>  	struct swap_info_struct		*sis,
>  	struct file			*swap_file,
>  	sector_t			*span)
> @@ -717,11 +717,11 @@ const struct address_space_operations xfs_address_space_operations = {
>  	.migrate_folio		= filemap_migrate_folio,
>  	.is_partially_uptodate  = iomap_is_partially_uptodate,
>  	.error_remove_folio	= generic_error_remove_folio,
> -	.swap_activate		= xfs_iomap_swapfile_activate,
> +	.swap_activate		= xfs_vm_swap_activate,
>  };
>  
>  const struct address_space_operations xfs_dax_aops = {
>  	.writepages		= xfs_dax_writepages,
>  	.dirty_folio		= noop_dirty_folio,
> -	.swap_activate		= xfs_iomap_swapfile_activate,
> +	.swap_activate		= xfs_vm_swap_activate,
>  };
> -- 
> 2.45.2
> 
> 

fix swapon for recently unshared blocks v2

[Christoph Hellwig]

Hi all,

the first patch fixes the recently added generic/370, and the
second one does a bit of naming cleanup in the area.

Changes since v1:
 - expand the comment based on text from Dave Chinner
 - add a Fixes tag

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

[Christoph Hellwig]

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.

The big comment is taken from a mailinglist post by Dave Chinner.

Fixes: 5e672cd69f0a53 ("xfs: introduce xfs_inodegc_push()")
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
---
 fs/xfs/xfs_aops.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 69b8c2d1937d..9ff1a0b07303 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,39 @@ 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));
+
+	/*
+	 * Swap file activation can race against concurrent shared extent
+	 * 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.
+	 */
+	xfs_inodegc_flush(ip->i_mount);
+
+	/*
+	 * Direct the swap code to the correct block device when this file
+	 * sits on the RT device.
+	 */
+	sis->bdev = xfs_inode_buftarg(ip)->bt_bdev;
+
 	return iomap_swapfile_activate(sis, swap_file, span,
 			&xfs_read_iomap_ops);
 }
-- 
2.45.2

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

[Carlos Maiolino]

On Thu, 06 Feb 2025 07:15:00 +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.
> 
> The big comment is taken from a mailinglist post by Dave Chinner.
> 
> 
> [...]

Applied to for-next, thanks!

[1/2] xfs: flush inodegc before swapon
      commit: 35010cc72acc468c98962f1056480a0a363eb1c3
[2/2] xfs: rename xfs_iomap_swapfile_activate to xfs_vm_swap_activate
      commit: 6f7ce473cca4952e4ac673f0fdf6dad2fac40324

Best regards,
-- 
Carlos Maiolino <cem@kernel.org>