[PATCH] generic: test swap activation on file that used to have clones

[PATCH] generic: test swap activation on file that used to have clones

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

Test that we are able to activate a swap file on a file that used to have
its extents shared multiple times.

This exercises a bug on btrfs' extent sharedness detection during swap
file activation, which is fixed by the following patch:

  "btrfs: fix swap file activation failure due to extents that used to be shared"

The test also fails sporadically on xfs and the bug was already reported
to the xfs mailing list:

   https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 tests/generic/368     | 101 ++++++++++++++++++++++++++++++++++++++++++
 tests/generic/368.out |  25 +++++++++++
 2 files changed, 126 insertions(+)
 create mode 100755 tests/generic/368
 create mode 100644 tests/generic/368.out

diff --git a/tests/generic/368 b/tests/generic/368
new file mode 100755
index 00000000..b2bf2d2c
--- /dev/null
+++ b/tests/generic/368
@@ -0,0 +1,101 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (C) 2024 SUSE Linux Products GmbH. All Rights Reserved.
+#
+# FS QA Test 368
+#
+# Test that we are able to create and activate a swap file on a file that used
+# to have its extents shared multiple times.
+#
+. ./common/preamble
+_begin_fstest auto quick clone swap
+
+_cleanup()
+{
+	cd /
+	rm -r -f $tmp.*
+	test -n "$swap_file" && swapoff $swap_file &> /dev/null
+}
+
+. ./common/reflink
+
+[ "$FSTYP" = "btrfs" ] && _fixed_by_kernel_commit xxxxxxxxxxxx \
+    "btrfs: fix swap file activation failure due to extents that used to be shared"
+
+_require_scratch_swapfile
+_require_scratch_reflink
+_require_cp_reflink
+
+run_test()
+{
+	local sync_after_add_reflinks=$1
+	local sync_after_remove_reflinks=$2
+	local first_swap_file="$SCRATCH_MNT/swap"
+	local swap_size=$(($(_get_page_size) * 32))
+	local num_clones=50
+	local swap_file="$SCRATCH_MNT/clone_${num_clones}"
+
+	_scratch_mkfs >> $seqres.full 2>&1 || _fail "failed to mkfs"
+	_scratch_mount
+
+	echo "Creating swap file..."
+	_format_swapfile $first_swap_file $swap_size >> $seqres.full
+
+	echo "Cloning swap file..."
+	# Create a large number of clones so that on btrfs we get external ref
+	# items in the extent tree and not just inline refs (33 is currently the
+	# treshold after which external refs are created).
+	for ((i = 1; i <= $num_clones; i++)); do
+		# Create the destination file and set +C (NOCOW) on it before
+		# copying into it with reflink. This is because when cp needs to
+		# create the destination file, it first copies/clones the data
+		# and then sets the +C attribute, and on btrfs we can't clone a
+		# NOCOW file into a COW file, both must be NOCOW or both COW.
+		touch $SCRATCH_MNT/clone_$i
+		# 0600 is required for swap files, do the same as _format_swapfile.
+		chmod 0600 $SCRATCH_MNT/clone_$i
+		$CHATTR_PROG +C $SCRATCH_MNT/clone_$i > /dev/null 2>&1
+		_cp_reflink $first_swap_file $SCRATCH_MNT/clone_$i
+	done
+
+	if [ $sync_after_add_reflinks -ne 0 ]; then
+		# Force a transaction commit on btrfs to flush all delayed
+		# references and commit the current transaction.
+		_scratch_sync
+	fi
+
+	echo "Deleting original file and all clones except the last..."
+	rm -f $first_swap_file
+	for ((i = 1; i < $num_clones; i++)); do
+		rm -f $SCRATCH_MNT/clone_$i
+	done
+
+	if [ $sync_after_remove_reflinks -ne 0 ]; then
+		# Force a transaction commit on btrfs to flush all delayed
+		# references and commit the current transaction.
+		_scratch_sync
+	fi
+
+	# Now use the last clone as a swap file.
+	echo "Activating swap file..."
+	_swapon_file $swap_file
+	swapoff $swap_file
+
+	_scratch_unmount
+}
+
+echo -e "\nTest without sync after creating and removing clones"
+run_test 0 0
+
+echo -e "\nTest with sync after creating clones"
+run_test 1 0
+
+echo -e "\nTest with sync after removing clones"
+run_test 0 1
+
+echo -e "\nTest with sync after creating and removing clones"
+run_test 1 1
+
+# success, all done
+status=0
+exit
diff --git a/tests/generic/368.out b/tests/generic/368.out
new file mode 100644
index 00000000..14a561e1
--- /dev/null
+++ b/tests/generic/368.out
@@ -0,0 +1,25 @@
+QA output created by 368
+
+Test without sync after creating and removing clones
+Creating swap file...
+Cloning swap file...
+Deleting original file and all clones except the last...
+Activating swap file...
+
+Test with sync after creating clones
+Creating swap file...
+Cloning swap file...
+Deleting original file and all clones except the last...
+Activating swap file...
+
+Test with sync after removing clones
+Creating swap file...
+Cloning swap file...
+Deleting original file and all clones except the last...
+Activating swap file...
+
+Test with sync after creating and removing clones
+Creating swap file...
+Cloning swap file...
+Deleting original file and all clones except the last...
+Activating swap file...
-- 
2.45.2

Re: [PATCH] generic: test swap activation on file that used to have clones

[Christoph Hellwig]

On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> The test also fails sporadically on xfs and the bug was already reported
> to the xfs mailing list:
> 
>    https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> 

This version still doesn't seem to have the fs freeze/unfreeze that Darrick
asked for in that thread.

Re: [PATCH] generic: test swap activation on file that used to have clones

[Filipe Manana]

On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> > The test also fails sporadically on xfs and the bug was already reported
> > to the xfs mailing list:
> >
> >    https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> >
>
> This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> asked for in that thread.

I don't get it, what's the freeze/unfreeze for? Where should they be placed?
Is it some way to get around the bug on xfs?

>

Re: [PATCH] generic: test swap activation on file that used to have clones

[Darrick J. Wong]

On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> > > The test also fails sporadically on xfs and the bug was already reported
> > > to the xfs mailing list:
> > >
> > >    https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> > >
> >
> > This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> > asked for in that thread.
> 
> I don't get it, what's the freeze/unfreeze for? Where should they be placed?
> Is it some way to get around the bug on xfs?

freeze kicks the background inode gc thread so that the unlinked clones
actually get freed before the swapon call.  A less bighammer idea might
be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
collectors.

--D

Re: [PATCH] generic: test swap activation on file that used to have clones

[Filipe Manana]

On Tue, Dec 17, 2024 at 5:22 PM Darrick J. Wong <djwong@kernel.org> wrote:
>
> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
> > On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >
> > > On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> > > > The test also fails sporadically on xfs and the bug was already reported
> > > > to the xfs mailing list:
> > > >
> > > >    https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> > > >
> > >
> > > This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> > > asked for in that thread.
> >
> > I don't get it, what's the freeze/unfreeze for? Where should they be placed?
> > Is it some way to get around the bug on xfs?
>
> freeze kicks the background inode gc thread so that the unlinked clones
> actually get freed before the swapon call.  A less bighammer idea might
> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
> collectors.

No matter the technical details that make the bug not so easy to fix
on xfs, adding calls to freeze/unfreeze, XFS_IOC_FREE_EOFBLOCKS, or
whatever else, is just a way to hide the bug on xfs, isn't it?
If the file has no more shared extents, swap activation should work.

Thanks.

>
> --D

Re: [PATCH] generic: test swap activation on file that used to have clones

[Qu Wenruo]

在 2024/12/18 03:52, Darrick J. Wong 写道:
> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
>> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
>>>
>>> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
>>>> The test also fails sporadically on xfs and the bug was already reported
>>>> to the xfs mailing list:
>>>>
>>>>     https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
>>>>
>>>
>>> This version still doesn't seem to have the fs freeze/unfreeze that Darrick
>>> asked for in that thread.
>>
>> I don't get it, what's the freeze/unfreeze for? Where should they be placed?
>> Is it some way to get around the bug on xfs?
>
> freeze kicks the background inode gc thread so that the unlinked clones
> actually get freed before the swapon call.  A less bighammer idea might
> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
> collectors.

I'm wondering why this GC things can not be done inside XFS' swapon call?

So that we don't need some per-fs workaround in a generic test case.

Thanks,
Qu
>
> --D
>

Re: [PATCH] generic: test swap activation on file that used to have clones

[Darrick J. Wong]

On Wed, Dec 18, 2024 at 09:07:26AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/12/18 03:52, Darrick J. Wong 写道:
> > On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
> > > On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > > 
> > > > On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> > > > > The test also fails sporadically on xfs and the bug was already reported
> > > > > to the xfs mailing list:
> > > > > 
> > > > >     https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> > > > > 
> > > > 
> > > > This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> > > > asked for in that thread.
> > > 
> > > I don't get it, what's the freeze/unfreeze for? Where should they be placed?
> > > Is it some way to get around the bug on xfs?
> > 
> > freeze kicks the background inode gc thread so that the unlinked clones
> > actually get freed before the swapon call.  A less bighammer idea might
> > be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
> > collectors.
> 
> I'm wondering why this GC things can not be done inside XFS' swapon call?
> 
> So that we don't need some per-fs workaround in a generic test case.

I suppose one could call xfs_inodegc_flush from within swapon with the
swap file's i_rwsem held, but now we're blocking swapon while we wait
for some unbounded number of probably unrelated unlinked inodes to be
freed on the off chance that one of them shared blocks.

A better answer might be to run FALLOC_FL_UNSHARE on the file, but now
we're making swapon more complex and potentially issuing a lot of IO to
make that happen.  If you can convince the fsdevel/mm folks that swapon
is supposed to try to correct things it doesn't like in the file mapping
(instead of returning EINVAL or whatever it does now) then we could add
that to the syscall definition.

--D

> Thanks,
> Qu
> > 
> > --D
> > 
> 
> 

Re: [PATCH] generic: test swap activation on file that used to have clones

[Qu Wenruo]

在 2024/12/19 06:39, Darrick J. Wong 写道:
> On Wed, Dec 18, 2024 at 09:07:26AM +1030, Qu Wenruo wrote:
>>
>>
>> 在 2024/12/18 03:52, Darrick J. Wong 写道:
>>> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
>>>> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
>>>>>
>>>>> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
>>>>>> The test also fails sporadically on xfs and the bug was already reported
>>>>>> to the xfs mailing list:
>>>>>>
>>>>>>      https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
>>>>>>
>>>>>
>>>>> This version still doesn't seem to have the fs freeze/unfreeze that Darrick
>>>>> asked for in that thread.
>>>>
>>>> I don't get it, what's the freeze/unfreeze for? Where should they be placed?
>>>> Is it some way to get around the bug on xfs?
>>>
>>> freeze kicks the background inode gc thread so that the unlinked clones
>>> actually get freed before the swapon call.  A less bighammer idea might
>>> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
>>> collectors.
>>
>> I'm wondering why this GC things can not be done inside XFS' swapon call?
>>
>> So that we don't need some per-fs workaround in a generic test case.
>
> I suppose one could call xfs_inodegc_flush from within swapon with the
> swap file's i_rwsem held, but now we're blocking swapon while we wait
> for some unbounded number of probably unrelated unlinked inodes to be
> freed on the off chance that one of them shared blocks.
>
> A better answer might be to run FALLOC_FL_UNSHARE on the file, but now
> we're making swapon more complex and potentially issuing a lot of IO to
> make that happen.  If you can convince the fsdevel/mm folks that swapon
> is supposed to try to correct things it doesn't like in the file mapping
> (instead of returning EINVAL or whatever it does now) then we could add
> that to the syscall definition.

Sorry that I'm no familiar with XFS to provide any help, but the swapon
call on btrfs is already very complex.

It needs to verify every extent of that file is not shared, and block
the subvolume from being snapshotted.
(The extent shareness check iteslf may already cause quite some IO)

So at least to me, a little more extra logic and IO shouldn't be a huge
blockage, since we're already doing exactly that since the btrfs
swapfile support.

Thanks,
Qu
>
> --D
>
>> Thanks,
>> Qu
>>>
>>> --D
>>>
>>
>>
>

Re: [PATCH] generic: test swap activation on file that used to have clones

[Filipe Manana]

On Wed, Dec 18, 2024 at 8:19 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
>
>
>
> 在 2024/12/19 06:39, Darrick J. Wong 写道:
> > On Wed, Dec 18, 2024 at 09:07:26AM +1030, Qu Wenruo wrote:
> >>
> >>
> >> 在 2024/12/18 03:52, Darrick J. Wong 写道:
> >>> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
> >>>> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
> >>>>>
> >>>>> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> >>>>>> The test also fails sporadically on xfs and the bug was already reported
> >>>>>> to the xfs mailing list:
> >>>>>>
> >>>>>>      https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> >>>>>>
> >>>>>
> >>>>> This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> >>>>> asked for in that thread.
> >>>>
> >>>> I don't get it, what's the freeze/unfreeze for? Where should they be placed?
> >>>> Is it some way to get around the bug on xfs?
> >>>
> >>> freeze kicks the background inode gc thread so that the unlinked clones
> >>> actually get freed before the swapon call.  A less bighammer idea might
> >>> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
> >>> collectors.
> >>
> >> I'm wondering why this GC things can not be done inside XFS' swapon call?
> >>
> >> So that we don't need some per-fs workaround in a generic test case.
> >
> > I suppose one could call xfs_inodegc_flush from within swapon with the
> > swap file's i_rwsem held, but now we're blocking swapon while we wait
> > for some unbounded number of probably unrelated unlinked inodes to be
> > freed on the off chance that one of them shared blocks.
> >
> > A better answer might be to run FALLOC_FL_UNSHARE on the file, but now
> > we're making swapon more complex and potentially issuing a lot of IO to
> > make that happen.  If you can convince the fsdevel/mm folks that swapon
> > is supposed to try to correct things it doesn't like in the file mapping
> > (instead of returning EINVAL or whatever it does now) then we could add
> > that to the syscall definition.

So how do we proceed from here?

The btrfs fix was merged into Linus' tree sometime ago (v6.13-rc5),
and I would hate to lose test coverage in fstests.

Since the bug seems to be hard to fix on XFS, shall I make the test
_notun on xfs, or move it tests/btrfs?
Zorro?

Thanks.

>
> Sorry that I'm no familiar with XFS to provide any help, but the swapon
> call on btrfs is already very complex.
>
> It needs to verify every extent of that file is not shared, and block
> the subvolume from being snapshotted.
> (The extent shareness check iteslf may already cause quite some IO)
>
> So at least to me, a little more extra logic and IO shouldn't be a huge
> blockage, since we're already doing exactly that since the btrfs
> swapfile support.
>
> Thanks,
> Qu
> >
> > --D
> >
> >> Thanks,
> >> Qu
> >>>
> >>> --D
> >>>
> >>
> >>
> >
>

Re: [PATCH] generic: test swap activation on file that used to have clones

[Zorro Lang]

On Mon, Jan 13, 2025 at 12:00:24PM +0000, Filipe Manana wrote:
> On Wed, Dec 18, 2024 at 8:19 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
> >
> >
> >
> > 在 2024/12/19 06:39, Darrick J. Wong 写道:
> > > On Wed, Dec 18, 2024 at 09:07:26AM +1030, Qu Wenruo wrote:
> > >>
> > >>
> > >> 在 2024/12/18 03:52, Darrick J. Wong 写道:
> > >>> On Tue, Dec 17, 2024 at 08:26:33AM +0000, Filipe Manana wrote:
> > >>>> On Tue, Dec 17, 2024 at 8:14 AM Christoph Hellwig <hch@infradead.org> wrote:
> > >>>>>
> > >>>>> On Wed, Dec 11, 2024 at 03:09:40PM +0000, fdmanana@kernel.org wrote:
> > >>>>>> The test also fails sporadically on xfs and the bug was already reported
> > >>>>>> to the xfs mailing list:
> > >>>>>>
> > >>>>>>      https://lore.kernel.org/linux-xfs/CAL3q7H7cURmnkJfUUx44HM3q=xKmqHb80eRdisErD_x8rU4+0Q@mail.gmail.com/
> > >>>>>>
> > >>>>>
> > >>>>> This version still doesn't seem to have the fs freeze/unfreeze that Darrick
> > >>>>> asked for in that thread.
> > >>>>
> > >>>> I don't get it, what's the freeze/unfreeze for? Where should they be placed?
> > >>>> Is it some way to get around the bug on xfs?
> > >>>
> > >>> freeze kicks the background inode gc thread so that the unlinked clones
> > >>> actually get freed before the swapon call.  A less bighammer idea might
> > >>> be to call XFS_IOC_FREE_EOFBLOCKS which also kicks the garbage
> > >>> collectors.
> > >>
> > >> I'm wondering why this GC things can not be done inside XFS' swapon call?
> > >>
> > >> So that we don't need some per-fs workaround in a generic test case.
> > >
> > > I suppose one could call xfs_inodegc_flush from within swapon with the
> > > swap file's i_rwsem held, but now we're blocking swapon while we wait
> > > for some unbounded number of probably unrelated unlinked inodes to be
> > > freed on the off chance that one of them shared blocks.
> > >
> > > A better answer might be to run FALLOC_FL_UNSHARE on the file, but now
> > > we're making swapon more complex and potentially issuing a lot of IO to
> > > make that happen.  If you can convince the fsdevel/mm folks that swapon
> > > is supposed to try to correct things it doesn't like in the file mapping
> > > (instead of returning EINVAL or whatever it does now) then we could add
> > > that to the syscall definition.
> 
> So how do we proceed from here?
> 
> The btrfs fix was merged into Linus' tree sometime ago (v6.13-rc5),
> and I would hate to lose test coverage in fstests.

Sure, let's move on :)

> 
> Since the bug seems to be hard to fix on XFS, shall I make the test
> _notun on xfs, or move it tests/btrfs?

Add "_supported_fs ^xfs" and some comments to explain why skiping XFS. Let's
cover the original btrfs bug at first (if there's not a good solution from
xfs for now), then deal with the xfs problem later.

Thanks,
Zorro

> Zorro?
> 
> Thanks.
> 
> >
> > Sorry that I'm no familiar with XFS to provide any help, but the swapon
> > call on btrfs is already very complex.
> >
> > It needs to verify every extent of that file is not shared, and block
> > the subvolume from being snapshotted.
> > (The extent shareness check iteslf may already cause quite some IO)
> >
> > So at least to me, a little more extra logic and IO shouldn't be a huge
> > blockage, since we're already doing exactly that since the btrfs
> > swapfile support.
> >
> > Thanks,
> > Qu
> > >
> > > --D
> > >
> > >> Thanks,
> > >> Qu
> > >>>
> > >>> --D
> > >>>
> > >>
> > >>
> > >
> >
>