From: Anna Schumaker <[email protected]>
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 <[email protected]>
---
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()
+{
+ 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
--
2.40.1
On Tue, May 16, 2023 at 10:14:07AM -0400, Anna Schumaker wrote:
> From: Anna Schumaker <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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 <[email protected]>
--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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
> >
> > 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 <[email protected]>
> >
> > ---
> > 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 <[email protected]>
>
> --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 <[email protected]>
> >
> > 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 <[email protected]>
>
>