[PATCH fstests v2] generic/755: test that inode's ctime is updated on unlink

[PATCH fstests v2] generic/755: test that inode's ctime is updated on unlink

[Jeff Layton]

I recently found and fixed a bug in btrfs where it wasn't updating the
citme on the target inode when unlinking [1]. Add a fstest for this.

[1]: https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
HCH suggested I roll a fstest for this problem that I found in btrfs the
other day. This just creates a file and a hardlink to it, statx's it and
then unlinks the hardlink and statx's it again. The ctimes are then
compared.
---
Changes in v2:
- Turn it into a shell script.
- Link to v1: https://lore.kernel.org/r/20240813-master-v1-1-862678cc4000@kernel.org
---
 tests/generic/755     | 38 ++++++++++++++++++++++++++++++++++++++
 tests/generic/755.out |  2 ++
 2 files changed, 40 insertions(+)

diff --git a/tests/generic/755 b/tests/generic/755
new file mode 100755
index 000000000000..68da3b20073f
--- /dev/null
+++ b/tests/generic/755
@@ -0,0 +1,38 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2024, Jeff Layton <jlayton@kernel.org>
+#
+# FS QA Test No. 755
+#
+# Create a file, stat it and then unlink it. Does the ctime of the
+# target inode change?
+#
+. ./common/preamble
+_begin_fstest auto quick
+
+_require_test
+_require_test_program unlink-ctime
+
+testfile="$TEST_DIR/unlink-ctime1.$$"
+testlink="$TEST_DIR/unlink-ctime2.$$"
+
+rm -f $testfile $testlink
+touch $testfile
+ln $testfile $testlink
+
+time1=$(stat -c "%Z" $testfile)
+
+sleep 2
+unlink $testlink
+
+time2=$(stat -c "%Z" $testfile)
+
+unlink $testfile
+
+if [ $time1 -eq $time2 ]; then
+	echo "Target's ctime did not change after unlink!"
+fi
+
+echo Silence is golden
+status=0
+exit
diff --git a/tests/generic/755.out b/tests/generic/755.out
new file mode 100644
index 000000000000..7c9ea51cd298
--- /dev/null
+++ b/tests/generic/755.out
@@ -0,0 +1,2 @@
+QA output created by 755
+Silence is golden

---
base-commit: f5ada754d5838d29fd270257003d0d123a9d1cd2
change-id: 20240813-master-e3b46de630bd

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH fstests v2] generic/755: test that inode's ctime is updated on unlink

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH fstests v2] generic/755: test that inode's ctime is updated on unlink

[Zorro Lang]

On Tue, Aug 20, 2024 at 03:48:25PM -0400, Jeff Layton wrote:
> I recently found and fixed a bug in btrfs where it wasn't updating the
> citme on the target inode when unlinking [1]. Add a fstest for this.
> 
> [1]: https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> HCH suggested I roll a fstest for this problem that I found in btrfs the
> other day. This just creates a file and a hardlink to it, statx's it and
> then unlinks the hardlink and statx's it again. The ctimes are then
> compared.
> ---
> Changes in v2:
> - Turn it into a shell script.
> - Link to v1: https://lore.kernel.org/r/20240813-master-v1-1-862678cc4000@kernel.org
> ---
>  tests/generic/755     | 38 ++++++++++++++++++++++++++++++++++++++
>  tests/generic/755.out |  2 ++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tests/generic/755 b/tests/generic/755
> new file mode 100755
> index 000000000000..68da3b20073f
> --- /dev/null
> +++ b/tests/generic/755
> @@ -0,0 +1,38 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2024, Jeff Layton <jlayton@kernel.org>
> +#
> +# FS QA Test No. 755
> +#
> +# Create a file, stat it and then unlink it. Does the ctime of the
> +# target inode change?
> +#
> +. ./common/preamble
> +_begin_fstest auto quick
> +
> +_require_test
> +_require_test_program unlink-ctime
                         ^^^^
This V2 shouldn't require this program, it's cause this case _notrun directly.
The 3bc2ac2f8f0b ("btrfs: update target inode's ctime on unlink") has been merged,

I'll remove this "_require_test_program" line, and add:
[ "$FSTYP = btrfs"] && _fixed_by_kernel_commit 3bc2ac2f8f0b \
	"btrfs: update target inode's ctime on unlink"

when I merge this patch. Others looks good to me.

Reviewed-by: Zorro Lang <zlang@redhat.com>

Thanks,
Zorro

> +
> +testfile="$TEST_DIR/unlink-ctime1.$$"
> +testlink="$TEST_DIR/unlink-ctime2.$$"
> +
> +rm -f $testfile $testlink
> +touch $testfile
> +ln $testfile $testlink
> +
> +time1=$(stat -c "%Z" $testfile)
> +
> +sleep 2
> +unlink $testlink
> +
> +time2=$(stat -c "%Z" $testfile)
> +
> +unlink $testfile
> +
> +if [ $time1 -eq $time2 ]; then
> +	echo "Target's ctime did not change after unlink!"
> +fi
> +
> +echo Silence is golden
> +status=0
> +exit
> diff --git a/tests/generic/755.out b/tests/generic/755.out
> new file mode 100644
> index 000000000000..7c9ea51cd298
> --- /dev/null
> +++ b/tests/generic/755.out
> @@ -0,0 +1,2 @@
> +QA output created by 755
> +Silence is golden
> 
> ---
> base-commit: f5ada754d5838d29fd270257003d0d123a9d1cd2
> change-id: 20240813-master-e3b46de630bd
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
> 

Re: [PATCH fstests v2] generic/755: test that inode's ctime is updated on unlink

[Jeff Layton]

On Thu, 2024-08-22 at 02:46 +0800, Zorro Lang wrote:
> On Tue, Aug 20, 2024 at 03:48:25PM -0400, Jeff Layton wrote:
> > I recently found and fixed a bug in btrfs where it wasn't updating
> > the
> > citme on the target inode when unlinking [1]. Add a fstest for
> > this.
> > 
> > [1]:
> > https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > HCH suggested I roll a fstest for this problem that I found in
> > btrfs the
> > other day. This just creates a file and a hardlink to it, statx's
> > it and
> > then unlinks the hardlink and statx's it again. The ctimes are then
> > compared.
> > ---
> > Changes in v2:
> > - Turn it into a shell script.
> > - Link to v1:
> > https://lore.kernel.org/r/20240813-master-v1-1-862678cc4000@kernel.org
> > ---
> >  tests/generic/755     | 38 ++++++++++++++++++++++++++++++++++++++
> >  tests/generic/755.out |  2 ++
> >  2 files changed, 40 insertions(+)
> > 
> > diff --git a/tests/generic/755 b/tests/generic/755
> > new file mode 100755
> > index 000000000000..68da3b20073f
> > --- /dev/null
> > +++ b/tests/generic/755
> > @@ -0,0 +1,38 @@
> > +#! /bin/bash
> > +# SPDX-License-Identifier: GPL-2.0
> > +# Copyright (c) 2024, Jeff Layton <jlayton@kernel.org>
> > +#
> > +# FS QA Test No. 755
> > +#
> > +# Create a file, stat it and then unlink it. Does the ctime of the
> > +# target inode change?
> > +#
> > +. ./common/preamble
> > +_begin_fstest auto quick
> > +
> > +_require_test
> > +_require_test_program unlink-ctime
>                          ^^^^
> This V2 shouldn't require this program, it's cause this case _notrun
> directly.

Doh! I removed that line on my test box, but forgot to fix it in tree.

> The 3bc2ac2f8f0b ("btrfs: update target inode's ctime on unlink") has
> been merged,
> 
> I'll remove this "_require_test_program" line, and add:
> [ "$FSTYP = btrfs"] && _fixed_by_kernel_commit 3bc2ac2f8f0b \
> 	"btrfs: update target inode's ctime on unlink"
> 
> when I merge this patch. Others looks good to me.
> 
> Reviewed-by: Zorro Lang <zlang@redhat.com>
> 

Thanks, that sounds great.

> > +
> > +testfile="$TEST_DIR/unlink-ctime1.$$"
> > +testlink="$TEST_DIR/unlink-ctime2.$$"
> > +
> > +rm -f $testfile $testlink
> > +touch $testfile
> > +ln $testfile $testlink
> > +
> > +time1=$(stat -c "%Z" $testfile)
> > +
> > +sleep 2
> > +unlink $testlink
> > +
> > +time2=$(stat -c "%Z" $testfile)
> > +
> > +unlink $testfile
> > +
> > +if [ $time1 -eq $time2 ]; then
> > +	echo "Target's ctime did not change after unlink!"
> > +fi
> > +
> > +echo Silence is golden
> > +status=0
> > +exit
> > diff --git a/tests/generic/755.out b/tests/generic/755.out
> > new file mode 100644
> > index 000000000000..7c9ea51cd298
> > --- /dev/null
> > +++ b/tests/generic/755.out
> > @@ -0,0 +1,2 @@
> > +QA output created by 755
> > +Silence is golden
> > 
> > ---
> > base-commit: f5ada754d5838d29fd270257003d0d123a9d1cd2
> > change-id: 20240813-master-e3b46de630bd
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
> 
> 

-- 
Jeff Layton <jlayton@kernel.org>