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

[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>
> > 
> 

-- 
Jeff Layton <jlayton@kernel.org>