| Message ID | 20240813-master-v1-1-862678cc4000@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [fstests] generic/755: test that inode's ctime is updated on unlink | expand |
On Tue, Aug 13, 2024 at 02:21:08PM -0400, Jeff Layton wrote: Hi Jeff :) Any description about this case test for? > 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. In principle, we probably could expand this to other dir > operations and to check the parent timestamps, but having to do all that > in C is a pain. I didn't see a good way to use xfs_io for this, > however. Is there a kernel commit or patch link about the bug which you found? > --- > src/Makefile | 2 +- > src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > tests/generic/755 | 26 ++++++++++++++++++++++++++ > tests/generic/755.out | 2 ++ > 4 files changed, 79 insertions(+), 1 deletion(-) > > diff --git a/src/Makefile b/src/Makefile > index 9979613711c9..c71fa41e4668 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault > + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime The .gitignore need updating too. > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c > new file mode 100644 > index 000000000000..7661e340eaba > --- /dev/null > +++ b/src/unlink-ctime.c > @@ -0,0 +1,50 @@ > +#define _GNU_SOURCE 1 > +#include <stdio.h> > +#include <fcntl.h> > +#include <unistd.h> > +#include <errno.h> > +#include <sys/stat.h> > + > +int main(int argc, char **argv) > +{ > + int fd, ret; > + struct statx before, after; > + > + if (argc < 2) { > + fprintf(stderr, "Must specify filename!\n"); > + return 1; > + } > + > + fd = open(argv[1], O_RDWR|O_CREAT, 0600); > + if (fd < 0) { > + perror("open"); > + return 1; > + } > + > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); > + if (ret) { > + perror("statx"); > + return 1; > + } > + > + sleep(1); > + > + ret = unlink(argv[1]); > + if (ret) { > + perror("unlink"); > + return 1; > + } > + > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); So you need to keep the "fd" after unlink. If so, there might not be a way through xfs_io to do that. > + if (ret) { > + perror("statx"); > + return 1; > + } > + > + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && > + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { > + printf("No change to ctime after unlink!\n"); > + return 1; > + } > + return 0; > +} > diff --git a/tests/generic/755 b/tests/generic/755 > new file mode 100755 > index 000000000000..500c51f99630 > --- /dev/null > +++ b/tests/generic/755 > @@ -0,0 +1,26 @@ > +#! /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 ^^^^^^ unlink > + > +# Import common functions. > +. ./common/filter > +. ./common/dmerror Why dmerror and filter are needed? If not necessary, remove these 3 lines. Others looks good to me. Thanks, Zorro > + > +_require_test > +_require_test_program unlink-ctime > + > +testfile="$TEST_DIR/unlink-ctime.$$" > + > +$here/src/unlink-ctime $testfile > + > +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 Fri, 2024-08-16 at 20:55 +0800, Zorro Lang wrote: > On Tue, Aug 13, 2024 at 02:21:08PM -0400, Jeff Layton wrote: > > Hi Jeff :) > > Any description about this case test for? > Yes. I should have put that info in the commit. Can you add it if the patch otherwise looks OK? https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/ Thanks, > > 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. In principle, we probably could expand this to other dir > > operations and to check the parent timestamps, but having to do all that > > in C is a pain. I didn't see a good way to use xfs_io for this, > > however. > > Is there a kernel commit or patch link about the bug which you found? > > > --- > > src/Makefile | 2 +- > > src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/755 | 26 ++++++++++++++++++++++++++ > > tests/generic/755.out | 2 ++ > > 4 files changed, 79 insertions(+), 1 deletion(-) > > > > diff --git a/src/Makefile b/src/Makefile > > index 9979613711c9..c71fa41e4668 100644 > > --- a/src/Makefile > > +++ b/src/Makefile > > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > > - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault > > + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime > > The .gitignore need updating too. > > > > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > > diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c > > new file mode 100644 > > index 000000000000..7661e340eaba > > --- /dev/null > > +++ b/src/unlink-ctime.c > > @@ -0,0 +1,50 @@ > > +#define _GNU_SOURCE 1 > > +#include <stdio.h> > > +#include <fcntl.h> > > +#include <unistd.h> > > +#include <errno.h> > > +#include <sys/stat.h> > > + > > +int main(int argc, char **argv) > > +{ > > + int fd, ret; > > + struct statx before, after; > > + > > + if (argc < 2) { > > + fprintf(stderr, "Must specify filename!\n"); > > + return 1; > > + } > > + > > + fd = open(argv[1], O_RDWR|O_CREAT, 0600); > > + if (fd < 0) { > > + perror("open"); > > + return 1; > > + } > > + > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); > > + if (ret) { > > + perror("statx"); > > + return 1; > > + } > > + > > + sleep(1); > > + > > + ret = unlink(argv[1]); > > + if (ret) { > > + perror("unlink"); > > + return 1; > > + } > > + > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); > > So you need to keep the "fd" after unlink. If so, there might not be a > way through xfs_io to do that. > > > + if (ret) { > > + perror("statx"); > > + return 1; > > + } > > + > > + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && > > + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { > > + printf("No change to ctime after unlink!\n"); > > + return 1; > > + } > > + return 0; > > +} > > diff --git a/tests/generic/755 b/tests/generic/755 > > new file mode 100755 > > index 000000000000..500c51f99630 > > --- /dev/null > > +++ b/tests/generic/755 > > @@ -0,0 +1,26 @@ > > +#! /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 > ^^^^^^ > unlink > > > + > > +# Import common functions. > > +. ./common/filter > > +. ./common/dmerror > > Why dmerror and filter are needed? If not necessary, remove these > 3 lines. > > Others looks good to me. > > Thanks, > Zorro > > > + > > +_require_test > > +_require_test_program unlink-ctime > > + > > +testfile="$TEST_DIR/unlink-ctime.$$" > > + > > +$here/src/unlink-ctime $testfile > > + > > +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 Fri, Aug 16, 2024 at 08:58:28AM -0400, Jeff Layton wrote: > On Fri, 2024-08-16 at 20:55 +0800, Zorro Lang wrote: > > On Tue, Aug 13, 2024 at 02:21:08PM -0400, Jeff Layton wrote: > > > > Hi Jeff :) > > > > Any description about this case test for? > > > > Yes. I should have put that info in the commit. Can you add it if the > patch otherwise looks OK? > > https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/ Hi Jeff, I saw this patch has gotten 3 RVBs, it's going to be in btrfs tree. I think it's good enough. BTW, you can add "_fixed_by_kernel_commit" to the test case, see below tests/generic/755 ... By reading above link, I think this issue doesn't need a C program (to reproduce), it can be done in bash script. e.g. # touch file # link file linkfile # ctime1=$(stat -c %Z file) # sleep 2 # ctime2=$(stat -c %Z file) # if [ "$ctime1" == "$ctime2" ];then .... Does that make sense to you? > > Thanks, > > > > 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. In principle, we probably could expand this to other dir > > > operations and to check the parent timestamps, but having to do all that > > > in C is a pain. I didn't see a good way to use xfs_io for this, > > > however. > > > > Is there a kernel commit or patch link about the bug which you found? > > > > > --- > > > src/Makefile | 2 +- > > > src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > tests/generic/755 | 26 ++++++++++++++++++++++++++ > > > tests/generic/755.out | 2 ++ > > > 4 files changed, 79 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/Makefile b/src/Makefile > > > index 9979613711c9..c71fa41e4668 100644 > > > --- a/src/Makefile > > > +++ b/src/Makefile > > > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > > > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > > > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > > > - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault > > > + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime > > > > The .gitignore need updating too. [need changing] > > > > > > > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > > > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > > > diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c > > > new file mode 100644 > > > index 000000000000..7661e340eaba > > > --- /dev/null > > > +++ b/src/unlink-ctime.c > > > @@ -0,0 +1,50 @@ > > > +#define _GNU_SOURCE 1 > > > +#include <stdio.h> > > > +#include <fcntl.h> > > > +#include <unistd.h> > > > +#include <errno.h> > > > +#include <sys/stat.h> > > > + > > > +int main(int argc, char **argv) > > > +{ > > > + int fd, ret; > > > + struct statx before, after; > > > + > > > + if (argc < 2) { > > > + fprintf(stderr, "Must specify filename!\n"); > > > + return 1; > > > + } > > > + > > > + fd = open(argv[1], O_RDWR|O_CREAT, 0600); > > > + if (fd < 0) { > > > + perror("open"); > > > + return 1; > > > + } > > > + > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); > > > + if (ret) { > > > + perror("statx"); > > > + return 1; > > > + } > > > + > > > + sleep(1); > > > + > > > + ret = unlink(argv[1]); > > > + if (ret) { > > > + perror("unlink"); > > > + return 1; > > > + } > > > + > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); > > > > So you need to keep the "fd" after unlink. If so, there might not be a > > way through xfs_io to do that. > > > > > + if (ret) { > > > + perror("statx"); > > > + return 1; > > > + } > > > + > > > + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && > > > + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { > > > + printf("No change to ctime after unlink!\n"); > > > + return 1; > > > + } > > > + return 0; > > > +} > > > diff --git a/tests/generic/755 b/tests/generic/755 > > > new file mode 100755 > > > index 000000000000..500c51f99630 > > > --- /dev/null > > > +++ b/tests/generic/755 > > > @@ -0,0 +1,26 @@ > > > +#! /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 > > ^^^^^^ > > unlink [need changing] > > > > > + > > > +# Import common functions. > > > +. ./common/filter > > > +. ./common/dmerror > > > > Why dmerror and filter are needed? If not necessary, remove these > > 3 lines. [need changing] > > > > Others looks good to me. > > > > Thanks, > > Zorro > > > > > + > > > +_require_test > > > +_require_test_program unlink-ctime _fixed_by_kernel_commit XXXXXXXXXXXX btrfs: update target inode's ctime on unlink > > > + > > > +testfile="$TEST_DIR/unlink-ctime.$$" > > > + > > > +$here/src/unlink-ctime $testfile > > > + > > > +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> >
On Sat, 2024-08-17 at 13:53 +0800, Zorro Lang wrote: > On Fri, Aug 16, 2024 at 08:58:28AM -0400, Jeff Layton wrote: > > On Fri, 2024-08-16 at 20:55 +0800, Zorro Lang wrote: > > > On Tue, Aug 13, 2024 at 02:21:08PM -0400, Jeff Layton wrote: > > > > > > Hi Jeff :) > > > > > > Any description about this case test for? > > > > > > > Yes. I should have put that info in the commit. Can you add it if the > > patch otherwise looks OK? > > > > https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/ > > Hi Jeff, > > I saw this patch has gotten 3 RVBs, it's going to be in btrfs tree. > I think it's good enough. BTW, you can add "_fixed_by_kernel_commit" > to the test case, see below tests/generic/755 ... > > By reading above link, I think this issue doesn't need a C program (to > reproduce), it can be done in bash script. e.g. > > # touch file > # link file linkfile > # ctime1=$(stat -c %Z file) > # sleep 2 > # ctime2=$(stat -c %Z file) > # if [ "$ctime1" == "$ctime2" ];then .... > > Does that make sense to you? > It does. I was trying to replicate the original report which showed that we didn't update the ctime when unlinking the last dentry on an inode, but this should replicate the btrfs bug just as well. I'm fine with going this route if it's what you'd prefer. Let me know. > > > > Thanks, > > > > > > 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. In principle, we probably could expand this to other dir > > > > operations and to check the parent timestamps, but having to do all that > > > > in C is a pain. I didn't see a good way to use xfs_io for this, > > > > however. > > > > > > Is there a kernel commit or patch link about the bug which you found? > > > > > > > --- > > > > src/Makefile | 2 +- > > > > src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > tests/generic/755 | 26 ++++++++++++++++++++++++++ > > > > tests/generic/755.out | 2 ++ > > > > 4 files changed, 79 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > index 9979613711c9..c71fa41e4668 100644 > > > > --- a/src/Makefile > > > > +++ b/src/Makefile > > > > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > > > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > > > > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > > > > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > > > > - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault > > > > + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime > > > > > > The .gitignore need updating too. > > [need changing] > > > > > > > > > > > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > > > > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > > > > diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c > > > > new file mode 100644 > > > > index 000000000000..7661e340eaba > > > > --- /dev/null > > > > +++ b/src/unlink-ctime.c > > > > @@ -0,0 +1,50 @@ > > > > +#define _GNU_SOURCE 1 > > > > +#include <stdio.h> > > > > +#include <fcntl.h> > > > > +#include <unistd.h> > > > > +#include <errno.h> > > > > +#include <sys/stat.h> > > > > + > > > > +int main(int argc, char **argv) > > > > +{ > > > > + int fd, ret; > > > > + struct statx before, after; > > > > + > > > > + if (argc < 2) { > > > > + fprintf(stderr, "Must specify filename!\n"); > > > > + return 1; > > > > + } > > > > + > > > > + fd = open(argv[1], O_RDWR|O_CREAT, 0600); > > > > + if (fd < 0) { > > > > + perror("open"); > > > > + return 1; > > > > + } > > > > + > > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); > > > > + if (ret) { > > > > + perror("statx"); > > > > + return 1; > > > > + } > > > > + > > > > + sleep(1); > > > > + > > > > + ret = unlink(argv[1]); > > > > + if (ret) { > > > > + perror("unlink"); > > > > + return 1; > > > > + } > > > > + > > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); > > > > > > So you need to keep the "fd" after unlink. If so, there might not be a > > > way through xfs_io to do that. > > > > > > > + if (ret) { > > > > + perror("statx"); > > > > + return 1; > > > > + } > > > > + > > > > + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && > > > > + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { > > > > + printf("No change to ctime after unlink!\n"); > > > > + return 1; > > > > + } > > > > + return 0; > > > > +} > > > > diff --git a/tests/generic/755 b/tests/generic/755 > > > > new file mode 100755 > > > > index 000000000000..500c51f99630 > > > > --- /dev/null > > > > +++ b/tests/generic/755 > > > > @@ -0,0 +1,26 @@ > > > > +#! /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 > > > ^^^^^^ > > > unlink > > [need changing] > > > > > > > > + > > > > +# Import common functions. > > > > +. ./common/filter > > > > +. ./common/dmerror > > > > > > Why dmerror and filter are needed? If not necessary, remove these > > > 3 lines. > > [need changing] > > > > > > > Others looks good to me. > > > > > > Thanks, > > > Zorro > > > > > > > + > > > > +_require_test > > > > +_require_test_program unlink-ctime > > _fixed_by_kernel_commit XXXXXXXXXXXX btrfs: update target inode's ctime on unlink > > > > > + > > > > +testfile="$TEST_DIR/unlink-ctime.$$" > > > > + > > > > +$here/src/unlink-ctime $testfile > > > > + > > > > +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> > > >
On Mon, Aug 19, 2024 at 07:39:56AM -0400, Jeff Layton wrote: > On Sat, 2024-08-17 at 13:53 +0800, Zorro Lang wrote: > > On Fri, Aug 16, 2024 at 08:58:28AM -0400, Jeff Layton wrote: > > > On Fri, 2024-08-16 at 20:55 +0800, Zorro Lang wrote: > > > > On Tue, Aug 13, 2024 at 02:21:08PM -0400, Jeff Layton wrote: > > > > > > > > Hi Jeff :) > > > > > > > > Any description about this case test for? > > > > > > > > > > Yes. I should have put that info in the commit. Can you add it if the > > > patch otherwise looks OK? > > > > > > https://lore.kernel.org/linux-btrfs/20240812-btrfs-unlink-v1-1-ee5c2ef538eb@kernel.org/ > > > > Hi Jeff, > > > > I saw this patch has gotten 3 RVBs, it's going to be in btrfs tree. > > I think it's good enough. BTW, you can add "_fixed_by_kernel_commit" > > to the test case, see below tests/generic/755 ... > > > > By reading above link, I think this issue doesn't need a C program (to > > reproduce), it can be done in bash script. e.g. > > > > # touch file > > # link file linkfile > > # ctime1=$(stat -c %Z file) > > # sleep 2 > > # ctime2=$(stat -c %Z file) > > # if [ "$ctime1" == "$ctime2" ];then .... > > > > Does that make sense to you? > > > > It does. I was trying to replicate the original report which showed > that we didn't update the ctime when unlinking the last dentry on an > inode, but this should replicate the btrfs bug just as well. > > I'm fine with going this route if it's what you'd prefer. Let me know. Thanks! If it can be a pure bash script test case, that would be better. I tried my above test steps, it can reproduce this bug too. Thanks, Zorro > > > > > > > Thanks, > > > > > > > > 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. In principle, we probably could expand this to other dir > > > > > operations and to check the parent timestamps, but having to do all that > > > > > in C is a pain. I didn't see a good way to use xfs_io for this, > > > > > however. > > > > > > > > Is there a kernel commit or patch link about the bug which you found? > > > > > > > > > --- > > > > > src/Makefile | 2 +- > > > > > src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > > tests/generic/755 | 26 ++++++++++++++++++++++++++ > > > > > tests/generic/755.out | 2 ++ > > > > > 4 files changed, 79 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/src/Makefile b/src/Makefile > > > > > index 9979613711c9..c71fa41e4668 100644 > > > > > --- a/src/Makefile > > > > > +++ b/src/Makefile > > > > > @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ > > > > > attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ > > > > > fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ > > > > > detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ > > > > > - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault > > > > > + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime > > > > > > > > The .gitignore need updating too. > > > > [need changing] > > > > > > > > > > > > > > > > EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ > > > > > btrfs_crc32c_forged_name.py popdir.pl popattr.py \ > > > > > diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c > > > > > new file mode 100644 > > > > > index 000000000000..7661e340eaba > > > > > --- /dev/null > > > > > +++ b/src/unlink-ctime.c > > > > > @@ -0,0 +1,50 @@ > > > > > +#define _GNU_SOURCE 1 > > > > > +#include <stdio.h> > > > > > +#include <fcntl.h> > > > > > +#include <unistd.h> > > > > > +#include <errno.h> > > > > > +#include <sys/stat.h> > > > > > + > > > > > +int main(int argc, char **argv) > > > > > +{ > > > > > + int fd, ret; > > > > > + struct statx before, after; > > > > > + > > > > > + if (argc < 2) { > > > > > + fprintf(stderr, "Must specify filename!\n"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + fd = open(argv[1], O_RDWR|O_CREAT, 0600); > > > > > + if (fd < 0) { > > > > > + perror("open"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); > > > > > + if (ret) { > > > > > + perror("statx"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + sleep(1); > > > > > + > > > > > + ret = unlink(argv[1]); > > > > > + if (ret) { > > > > > + perror("unlink"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); > > > > > > > > So you need to keep the "fd" after unlink. If so, there might not be a > > > > way through xfs_io to do that. > > > > > > > > > + if (ret) { > > > > > + perror("statx"); > > > > > + return 1; > > > > > + } > > > > > + > > > > > + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && > > > > > + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { > > > > > + printf("No change to ctime after unlink!\n"); > > > > > + return 1; > > > > > + } > > > > > + return 0; > > > > > +} > > > > > diff --git a/tests/generic/755 b/tests/generic/755 > > > > > new file mode 100755 > > > > > index 000000000000..500c51f99630 > > > > > --- /dev/null > > > > > +++ b/tests/generic/755 > > > > > @@ -0,0 +1,26 @@ > > > > > +#! /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 > > > > ^^^^^^ > > > > unlink > > > > [need changing] > > > > > > > > > > > + > > > > > +# Import common functions. > > > > > +. ./common/filter > > > > > +. ./common/dmerror > > > > > > > > Why dmerror and filter are needed? If not necessary, remove these > > > > 3 lines. > > > > [need changing] > > > > > > > > > > Others looks good to me. > > > > > > > > Thanks, > > > > Zorro > > > > > > > > > + > > > > > +_require_test > > > > > +_require_test_program unlink-ctime > > > > _fixed_by_kernel_commit XXXXXXXXXXXX btrfs: update target inode's ctime on unlink > > > > > > > + > > > > > +testfile="$TEST_DIR/unlink-ctime.$$" > > > > > + > > > > > +$here/src/unlink-ctime $testfile > > > > > + > > > > > +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> > > > > > > > -- > Jeff Layton <jlayton@kernel.org> >
diff --git a/src/Makefile b/src/Makefile index 9979613711c9..c71fa41e4668 100644 --- a/src/Makefile +++ b/src/Makefile @@ -34,7 +34,7 @@ LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \ attr_replace_test swapon mkswap t_attr_corruption t_open_tmpfiles \ fscrypt-crypt-util bulkstat_null_ocount splice-test chprojid_fail \ detached_mounts_propagation ext4_resize t_readdir_3 splice2pipe \ - uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault + uuid_ioctl t_snapshot_deleted_subvolume fiemap-fault unlink-ctime EXTRA_EXECS = dmerror fill2attr fill2fs fill2fs_check scaleread.sh \ btrfs_crc32c_forged_name.py popdir.pl popattr.py \ diff --git a/src/unlink-ctime.c b/src/unlink-ctime.c new file mode 100644 index 000000000000..7661e340eaba --- /dev/null +++ b/src/unlink-ctime.c @@ -0,0 +1,50 @@ +#define _GNU_SOURCE 1 +#include <stdio.h> +#include <fcntl.h> +#include <unistd.h> +#include <errno.h> +#include <sys/stat.h> + +int main(int argc, char **argv) +{ + int fd, ret; + struct statx before, after; + + if (argc < 2) { + fprintf(stderr, "Must specify filename!\n"); + return 1; + } + + fd = open(argv[1], O_RDWR|O_CREAT, 0600); + if (fd < 0) { + perror("open"); + return 1; + } + + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &before); + if (ret) { + perror("statx"); + return 1; + } + + sleep(1); + + ret = unlink(argv[1]); + if (ret) { + perror("unlink"); + return 1; + } + + ret = statx(fd, "", AT_EMPTY_PATH, STATX_CTIME, &after); + if (ret) { + perror("statx"); + return 1; + } + + if (before.stx_ctime.tv_nsec == after.stx_ctime.tv_nsec && + before.stx_ctime.tv_sec == after.stx_ctime.tv_sec) { + printf("No change to ctime after unlink!\n"); + return 1; + } + return 0; +} diff --git a/tests/generic/755 b/tests/generic/755 new file mode 100755 index 000000000000..500c51f99630 --- /dev/null +++ b/tests/generic/755 @@ -0,0 +1,26 @@ +#! /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 + +# Import common functions. +. ./common/filter +. ./common/dmerror + +_require_test +_require_test_program unlink-ctime + +testfile="$TEST_DIR/unlink-ctime.$$" + +$here/src/unlink-ctime $testfile + +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
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. In principle, we probably could expand this to other dir operations and to check the parent timestamps, but having to do all that in C is a pain. I didn't see a good way to use xfs_io for this, however. --- src/Makefile | 2 +- src/unlink-ctime.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ tests/generic/755 | 26 ++++++++++++++++++++++++++ tests/generic/755.out | 2 ++ 4 files changed, 79 insertions(+), 1 deletion(-) --- base-commit: f5ada754d5838d29fd270257003d0d123a9d1cd2 change-id: 20240813-master-e3b46de630bd Best regards,