[PATCH v3] generic/728: Add a test for xattr ctime updates

[PATCH v3] generic/728: Add a test for xattr ctime updates

[Anna Schumaker]

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()
+{
+  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

Re: [PATCH v3] generic/728: Add a test for xattr ctime updates

[Darrick J. Wong]

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
> 

Re: [PATCH v3] generic/728: Add a test for xattr ctime updates

[Zorro Lang]

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

Re: [PATCH v3] generic/728: Add a test for xattr ctime updates

[Anand Jain]

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>

Re: [PATCH v3] generic/728: Add a test for xattr ctime updates

[Zorro Lang]

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