FSTests Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] generic/527: add additional test including a file with a hardlink
@ 2020-01-15 13:22 fdmanana
  2020-01-16 16:05 ` Josef Bacik
  0 siblings, 1 reply; 3+ messages in thread
From: fdmanana @ 2020-01-15 13:22 UTC (permalink / raw)
  To: fstests; +Cc: linux-btrfs, Filipe Manana

From: Filipe Manana <fdmanana@suse.com>

Add a similar test to the existing one but with a file that has a
hardlink as well. This is motivated by a bug found in btrfs where
a fsync on a file that has the old name of another file results
in the logging code to hit an infinite loop. The patch that fixes
the bug in btrfs has the following subject:

  "Btrfs: fix infinite loop during fsync after rename operations"

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 tests/generic/527     | 26 ++++++++++++++++++++++++++
 tests/generic/527.out |  4 ++++
 2 files changed, 30 insertions(+)

diff --git a/tests/generic/527 b/tests/generic/527
index aacccd91..61dd4f0b 100755
--- a/tests/generic/527
+++ b/tests/generic/527
@@ -45,6 +45,12 @@ mkdir $SCRATCH_MNT/testdir
 echo -n "foo" > $SCRATCH_MNT/testdir/fname1
 echo -n "hello" > $SCRATCH_MNT/testdir/fname2
 
+# For a different variant of the same test but when files have hardlinks too.
+mkdir $SCRATCH_MNT/testdir2
+echo -n "foo" > $SCRATCH_MNT/testdir2/zz
+ln $SCRATCH_MNT/testdir2/zz $SCRATCH_MNT/testdir2/zz_link
+echo -n "hello" > $SCRATCH_MNT/testdir2/a
+
 # Make sure everything done so far is durably persisted.
 sync
 
@@ -57,6 +63,21 @@ ln $SCRATCH_MNT/testdir/fname3 $SCRATCH_MNT/testdir/fname2
 echo -n "bar" > $SCRATCH_MNT/testdir/fname1
 $XFS_IO_PROG -c "fsync" $SCRATCH_MNT/testdir/fname1
 
+# A second variant, more complex, that involves files with hardlinks too.
+
+# The following 3 renames are equivalent to a rename exchange (zz_link to a), but
+# without the atomicity which isn't required here.
+mv $SCRATCH_MNT/testdir2/a $SCRATCH_MNT/testdir2/tmp
+mv $SCRATCH_MNT/testdir2/zz_link $SCRATCH_MNT/testdir2/a
+mv $SCRATCH_MNT/testdir2/tmp $SCRATCH_MNT/testdir2/zz_link
+
+# The following rename and file creation are equivalent to a rename whiteout.
+mv $SCRATCH_MNT/testdir2/zz $SCRATCH_MNT/testdir2/a2
+echo -n "bar" > $SCRATCH_MNT/testdir2/zz
+
+# Fsync of zz should work and produce correct results after a power failure.
+$XFS_IO_PROG -c "fsync" $SCRATCH_MNT/testdir2/zz
+
 # Simulate a power failure and mount the filesystem to check that all file names
 # exist and correspond to the correct inodes.
 _flakey_drop_and_remount
@@ -65,6 +86,11 @@ echo "File fname1 data after power failure: $(cat $SCRATCH_MNT/testdir/fname1)"
 echo "File fname2 data after power failure: $(cat $SCRATCH_MNT/testdir/fname2)"
 echo "File fname3 data after power failure: $(cat $SCRATCH_MNT/testdir/fname3)"
 
+echo "File a data after power failure: $(cat $SCRATCH_MNT/testdir2/a)"
+echo "File a2 data after power failure: $(cat $SCRATCH_MNT/testdir2/a2)"
+echo "File zz data after power failure: $(cat $SCRATCH_MNT/testdir2/zz)"
+echo "File zz_link data after power failure: $(cat $SCRATCH_MNT/testdir2/zz_link)"
+
 _unmount_flakey
 
 status=0
diff --git a/tests/generic/527.out b/tests/generic/527.out
index 3c34e1ff..7397fe88 100644
--- a/tests/generic/527.out
+++ b/tests/generic/527.out
@@ -2,3 +2,7 @@ QA output created by 527
 File fname1 data after power failure: bar
 File fname2 data after power failure: foo
 File fname3 data after power failure: foo
+File a data after power failure: foo
+File a2 data after power failure: foo
+File zz data after power failure: bar
+File zz_link data after power failure: hello
-- 
2.11.0


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] generic/527: add additional test including a file with a hardlink
  2020-01-15 13:22 [PATCH] generic/527: add additional test including a file with a hardlink fdmanana
@ 2020-01-16 16:05 ` Josef Bacik
  2020-01-16 16:16   ` Filipe Manana
  0 siblings, 1 reply; 3+ messages in thread
From: Josef Bacik @ 2020-01-16 16:05 UTC (permalink / raw)
  To: fdmanana, fstests; +Cc: linux-btrfs, Filipe Manana

On 1/15/20 8:22 AM, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> Add a similar test to the existing one but with a file that has a
> hardlink as well. This is motivated by a bug found in btrfs where
> a fsync on a file that has the old name of another file results
> in the logging code to hit an infinite loop. The patch that fixes
> the bug in btrfs has the following subject:
> 
>    "Btrfs: fix infinite loop during fsync after rename operations"
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

What's our policy on adding a new variant to an existing test?  I thought we 
preferred to create a new test for these sort of things?  If not then you can add

Reviewed-by: Josef Bacik <josef@toxicpanda.com>

I have no strong opinions either way, I just know it's come up in the past.  Thanks,

Josef

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] generic/527: add additional test including a file with a hardlink
  2020-01-16 16:05 ` Josef Bacik
@ 2020-01-16 16:16   ` Filipe Manana
  0 siblings, 0 replies; 3+ messages in thread
From: Filipe Manana @ 2020-01-16 16:16 UTC (permalink / raw)
  To: Josef Bacik; +Cc: fstests, linux-btrfs, Filipe Manana

On Thu, Jan 16, 2020 at 4:05 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> On 1/15/20 8:22 AM, fdmanana@kernel.org wrote:
> > From: Filipe Manana <fdmanana@suse.com>
> >
> > Add a similar test to the existing one but with a file that has a
> > hardlink as well. This is motivated by a bug found in btrfs where
> > a fsync on a file that has the old name of another file results
> > in the logging code to hit an infinite loop. The patch that fixes
> > the bug in btrfs has the following subject:
> >
> >    "Btrfs: fix infinite loop during fsync after rename operations"
> >
> > Signed-off-by: Filipe Manana <fdmanana@suse.com>
>
> What's our policy on adding a new variant to an existing test?  I thought we
> preferred to create a new test for these sort of things?  If not then you can add

Usually yes, but since over time there started to be too many fsync
related tests, we had a discussion with Dave (Chinner)
and others about considering consolidating more tests into the same
files. It's not just the number of test files, but more time to run
(even if each one adds only 1 or 2 seconds).

That's why I opted here to update generic/527 instead of adding a new test file.
Thanks.

>
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
>
> I have no strong opinions either way, I just know it's come up in the past.  Thanks,
>
> Josef

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15 13:22 [PATCH] generic/527: add additional test including a file with a hardlink fdmanana
2020-01-16 16:05 ` Josef Bacik
2020-01-16 16:16   ` Filipe Manana

FSTests Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/fstests/0 fstests/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 fstests fstests/ https://lore.kernel.org/fstests \
		fstests@vger.kernel.org
	public-inbox-index fstests

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.fstests


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git