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@xxxxxx>
> > ---
> > 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@xxxxxxxx/
> 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@xxxxxxxxxxxxx
>
