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

[Date Prev] [Date Next] [Thread Prev] [Thread Next] [Date Index] [Thread Index]



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?

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





[Index of Archives]     [Linux Filesystems Development]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux