| Message ID | 20230516141407.201674-1-anna@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] generic/728: Add a test for xattr ctime updates | expand |
On Tue, May 16, 2023 at 10:14:07AM -0400, Anna Schumaker wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > The NFS client wasn't updating ctime after a setxattr request. This is a > test written while fixing the bug. > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > --- > v3: > - Add a 2 second sleep before changing the xattr > > v2: > - Move test to generic/ > - Address comments from the mailing list > --- > tests/generic/728 | 43 +++++++++++++++++++++++++++++++++++++++++++ > tests/generic/728.out | 2 ++ > 2 files changed, 45 insertions(+) > create mode 100755 tests/generic/728 > create mode 100644 tests/generic/728.out > > diff --git a/tests/generic/728 b/tests/generic/728 > new file mode 100755 > index 000000000000..8e52eb4b219c > --- /dev/null > +++ b/tests/generic/728 > @@ -0,0 +1,43 @@ > +#! /bin/bash > +# SPDX-License-Identifier: GPL-2.0 > +# Copyright (c) 2023 Netapp Inc., All Rights Reserved. > +# > +# FS QA Test 728 > +# > +# Test a bug where the NFS client wasn't sending a post-op GETATTR to the > +# server after setting an xattr, resulting in `stat` reporting a stale ctime. > +# > +. ./common/preamble > +_begin_fstest auto quick attr > + > +# Import common functions > +. ./common/attr > + > +# real QA test starts here > +_supported_fs generic > +_require_test > +_require_attrs > + > +rm -rf $TEST_DIR/testfile > +touch $TEST_DIR/testfile > + > + > +_check_xattr_op() nit: only common/* functions are supposed to have a leading underscore in the name. > +{ > + what=$1 > + shift 1 > + > + before_ctime=$(stat -c %z $TEST_DIR/testfile) > + sleep 2 I think it would be useful to document that 2 seconds is the worst ctime granularity that we expect from any filesystem (fat) that might pass through fstests. Just in case, you know, we /ever/ create a fsinfo call to export information like that, and need to refactor all these 'sleep 2'. "sleep 2 # maximum known ctime granularity is 2s for fat" With those two things fixed, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > + $SETFATTR_PROG $* $TEST_DIR/testfile > + after_ctime=$(stat -c %z $TEST_DIR/testfile) > + > + test "$before_ctime" != "$after_ctime" || echo "Expected ctime to change after $what." > +} > + > +_check_xattr_op setxattr -n user.foobar -v 123 > +_check_xattr_op removexattr -x user.foobar > + > +echo "Silence is golden" > +status=0 > +exit > diff --git a/tests/generic/728.out b/tests/generic/728.out > new file mode 100644 > index 000000000000..ab39f45fe5da > --- /dev/null > +++ b/tests/generic/728.out > @@ -0,0 +1,2 @@ > +QA output created by 728 > +Silence is golden > -- > 2.40.1 >
On 16/5/23 22:14, Anna Schumaker wrote: > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > The NFS client wasn't updating ctime after a setxattr request. This is a > test written while fixing the bug. You can include the '_fixed_by_kernel_commit' field for hints on unfixed kernels. With this and Darrick's suggestions, you can add my RB. Reviewed-by: Anand Jain <anand.jain@oracle.com>
On Tue, May 16, 2023 at 08:00:27AM -0700, Darrick J. Wong wrote: > On Tue, May 16, 2023 at 10:14:07AM -0400, Anna Schumaker wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > The NFS client wasn't updating ctime after a setxattr request. This is a > > test written while fixing the bug. > > > > Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > --- > > v3: > > - Add a 2 second sleep before changing the xattr > > > > v2: > > - Move test to generic/ > > - Address comments from the mailing list > > --- > > tests/generic/728 | 43 +++++++++++++++++++++++++++++++++++++++++++ > > tests/generic/728.out | 2 ++ > > 2 files changed, 45 insertions(+) > > create mode 100755 tests/generic/728 > > create mode 100644 tests/generic/728.out > > > > diff --git a/tests/generic/728 b/tests/generic/728 > > new file mode 100755 > > index 000000000000..8e52eb4b219c > > --- /dev/null > > +++ b/tests/generic/728 > > @@ -0,0 +1,43 @@ > > +#! /bin/bash > > +# SPDX-License-Identifier: GPL-2.0 > > +# Copyright (c) 2023 Netapp Inc., All Rights Reserved. > > +# > > +# FS QA Test 728 > > +# > > +# Test a bug where the NFS client wasn't sending a post-op GETATTR to the > > +# server after setting an xattr, resulting in `stat` reporting a stale ctime. > > +# > > +. ./common/preamble > > +_begin_fstest auto quick attr > > + > > +# Import common functions > > +. ./common/attr > > + > > +# real QA test starts here > > +_supported_fs generic > > +_require_test > > +_require_attrs > > + > > +rm -rf $TEST_DIR/testfile > > +touch $TEST_DIR/testfile > > + > > + > > +_check_xattr_op() > > nit: only common/* functions are supposed to have a leading underscore > in the name. I can help to remove the leading underscore when I merge this patch. > > > +{ > > + what=$1 > > + shift 1 > > + > > + before_ctime=$(stat -c %z $TEST_DIR/testfile) > > + sleep 2 > > I think it would be useful to document that 2 seconds is the worst ctime > granularity that we expect from any filesystem (fat) that might pass > through fstests. > > Just in case, you know, we /ever/ create a fsinfo call to export > information like that, and need to refactor all these 'sleep 2'. > > "sleep 2 # maximum known ctime granularity is 2s for fat" Hi Anna, if you don't want to send a v4, you can reply this email to tell us what comment you'd like to write. I can add it when I merge this patch. Or you'd like to send a v4 by yourself :) Thanks, Zorro > > With those two things fixed, > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > > --D > > > > + $SETFATTR_PROG $* $TEST_DIR/testfile > > + after_ctime=$(stat -c %z $TEST_DIR/testfile) > > + > > + test "$before_ctime" != "$after_ctime" || echo "Expected ctime to change after $what." > > +} > > + > > +_check_xattr_op setxattr -n user.foobar -v 123 > > +_check_xattr_op removexattr -x user.foobar > > + > > +echo "Silence is golden" > > +status=0 > > +exit > > diff --git a/tests/generic/728.out b/tests/generic/728.out > > new file mode 100644 > > index 000000000000..ab39f45fe5da > > --- /dev/null > > +++ b/tests/generic/728.out > > @@ -0,0 +1,2 @@ > > +QA output created by 728 > > +Silence is golden > > -- > > 2.40.1 > > >
On Wed, May 17, 2023 at 01:04:59PM +0800, Anand Jain wrote: > On 16/5/23 22:14, Anna Schumaker wrote: > > From: Anna Schumaker <Anna.Schumaker@Netapp.com> > > > > The NFS client wasn't updating ctime after a setxattr request. This is a > > test written while fixing the bug. > > You can include the '_fixed_by_kernel_commit' field for hints Anna said the related kernel commit hasn't been acked. I'm not sure what's the current status, if it's acked but not merged, Anna can use "xxxxxxxx" to replace the commit id temporarily, or add this later (just don't forget:) Thanks, Zorro > on unfixed kernels. With this and Darrick's suggestions, you > can add my RB. > > Reviewed-by: Anand Jain <anand.jain@oracle.com> > >
diff --git a/tests/generic/728 b/tests/generic/728 new file mode 100755 index 000000000000..8e52eb4b219c --- /dev/null +++ b/tests/generic/728 @@ -0,0 +1,43 @@ +#! /bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (c) 2023 Netapp Inc., All Rights Reserved. +# +# FS QA Test 728 +# +# Test a bug where the NFS client wasn't sending a post-op GETATTR to the +# server after setting an xattr, resulting in `stat` reporting a stale ctime. +# +. ./common/preamble +_begin_fstest auto quick attr + +# Import common functions +. ./common/attr + +# real QA test starts here +_supported_fs generic +_require_test +_require_attrs + +rm -rf $TEST_DIR/testfile +touch $TEST_DIR/testfile + + +_check_xattr_op() +{ + what=$1 + shift 1 + + before_ctime=$(stat -c %z $TEST_DIR/testfile) + sleep 2 + $SETFATTR_PROG $* $TEST_DIR/testfile + after_ctime=$(stat -c %z $TEST_DIR/testfile) + + test "$before_ctime" != "$after_ctime" || echo "Expected ctime to change after $what." +} + +_check_xattr_op setxattr -n user.foobar -v 123 +_check_xattr_op removexattr -x user.foobar + +echo "Silence is golden" +status=0 +exit diff --git a/tests/generic/728.out b/tests/generic/728.out new file mode 100644 index 000000000000..ab39f45fe5da --- /dev/null +++ b/tests/generic/728.out @@ -0,0 +1,2 @@ +QA output created by 728 +Silence is golden