| Message ID | 20240820-master-v2-1-41703dddcc32@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] generic/755: test that inode's ctime is updated on unlink | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
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> > >
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> > > > > > >
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
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(+) --- base-commit: f5ada754d5838d29fd270257003d0d123a9d1cd2 change-id: 20240813-master-e3b46de630bd Best regards,