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@xxxxxxxxxx/ > > > > 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@xxxxxxxxxx> > > > > > --- > > > > > 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@xxxxxxxxxx> > > > > > +# > > > > > +# 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@xxxxxxxxxx> > > > > > > > > > > > > > > > > > > > > -- > > > Jeff Layton <jlayton@xxxxxxxxxx> > > > > > > > -- > Jeff Layton <jlayton@xxxxxxxxxx> >
