[PATCH v2 0/5] pynfs: a handful of fixes and a new CB_GETATTR test

[PATCH v2 0/5] pynfs: a handful of fixes and a new CB_GETATTR test

[Scott Mayhew]

Patches 1-4 are fixes/cleanups.

Patch 5 simulates an actual application issue where the application is
failing because it's getting a unexpected/surprise mtime update for a
file that it's tracking.

v2 changes:
- Reordered patches 3 & 4
- Patch 4 now fixes the erroneous check instead of removing it, fixes
  issues w/ the initialization of cbattrs, and calls compareTimes()
  to compare the mtimes
- Patch 5 now calls compareTimes() to compare the mtimes

Scott Mayhew (5):
  pynfs: ensure tests clean up after themselves
  pynfs: fix the check for delegated attribute support
  pynfs: _testCbGetattr() should check the delegation that was received
  pynfs: fix erroneous test in DELEG24 and DELEG25
  pynfs: add delegation test for CB_GETATTR after sync WRITE

 nfs4.1/server41tests/st_courtesy.py   |   2 +
 nfs4.1/server41tests/st_delegation.py | 247 ++++++++++++++++++++++----
 nfs4.1/server41tests/st_sparse.py     |  10 +-
 3 files changed, 228 insertions(+), 31 deletions(-)

-- 
2.53.0

[PATCH v2 1/5] pynfs: ensure tests clean up after themselves

[Scott Mayhew]

I noticed my server was taking longer to exit grace on restart after
running pynfs.  The server was waiting for several clients (which have
already gone away) to finish reclaim.  You can observe the client ids
using 'nfsdclddb print' while the server is in grace (or prior to
reboot):

$ sudo nfsdclddb print
Schema version: 4 current epoch: 47 recovery epoch: 46
Clients in current epoch:
Clients in recovery epoch:
id = COUR2_1774823970_2, princhash = (null)
id = ALLOC1_1774823970, princhash = (null)
id = ALLOC2_1774823970, princhash = (null)
id = ALLOC3_1774823970, princhash = (null)
id = DELEG9_1774823970_1, princhash = (null)
id = DELEG9_1774823970_2, princhash = (null)
id = DELEG2_1774823970_1, princhash = (null)
id = DELEG23_1774823970_1, princhash = (null)
id = DELEG1_1774823970_1, princhash = (null)
id = DELEG4_1774823970_1, princhash = (null)
id = DELEG8_1774823970_1, princhash = (null)
id = DELEG8_1774823970_2, princhash = (null)
id = DELEG25_1774823970_1, princhash = (null)
id = DELEG24_1774823970_1, princhash = (null)
id = DELEG6_1774823970_1, princhash = (null)
id = DELEG7_1774823970_1, princhash = (null)
id = DELEG5_1774823970_1, princhash = (null)
id = DELEG3_1774823970_1, princhash = (null)

These tests were all failing to close some of their open files, causing
DESTROY_CLIENTID at the end of the test to return NFS4ERR_CLIENTID_BUSY.

On older servers these client ids would stick around for 1 lease period.
With the newer courteous server they may stick around indefinitely, so
it's important for the tests to clean up after themselves.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs4.1/server41tests/st_courtesy.py   |  2 +
 nfs4.1/server41tests/st_delegation.py | 59 +++++++++++++++++++++------
 nfs4.1/server41tests/st_sparse.py     | 10 ++++-
 3 files changed, 58 insertions(+), 13 deletions(-)

diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
index 54cf17d..60089c9 100644
--- a/nfs4.1/server41tests/st_courtesy.py
+++ b/nfs4.1/server41tests/st_courtesy.py
@@ -84,6 +84,8 @@ def testLockSleepLock(t, env):
     res = sess2.compound(cour_lockargs(fh, stateid))
     check(res, NFS4_OK)
 
+    close_file(sess2, fh, stateid=stateid)
+
 def testShareReservation00(t, env):
     """Test OPEN file with OPEN4_SHARE_DENY_WRITE
        1st client opens file with OPEN4_SHARE_DENY_WRITE
diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
index 41095b9..d41f12d 100644
--- a/nfs4.1/server41tests/st_delegation.py
+++ b/nfs4.1/server41tests/st_delegation.py
@@ -17,6 +17,7 @@ def __create_file_with_deleg(sess, name, access):
     res = create_file(sess, name, access = access)
     check(res)
     fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
     deleg = res.resarray[-2].delegation
     if (not _got_deleg(deleg)):
         res = open_file(sess, name, access = access)
@@ -24,11 +25,11 @@ def __create_file_with_deleg(sess, name, access):
         deleg = res.resarray[-2].delegation
         if (not _got_deleg(deleg)):
             fail("Could not get delegation")
-    return (fh, deleg)
+    return (fh, stateid, deleg)
 
 def _create_file_with_deleg(sess, name, access):
-    fh, deleg = __create_file_with_deleg(sess, name, access)
-    return fh
+    fh, stateid, deleg = __create_file_with_deleg(sess, name, access)
+    return (fh, stateid)
 
 def _testDeleg(t, env, openaccess, want, breakaccess, sec = None, sec2 = None):
     recall = threading.Event()
@@ -43,7 +44,7 @@ def _testDeleg(t, env, openaccess, want, breakaccess, sec = None, sec2 = None):
     sess1.client.cb_post_hook(OP_CB_RECALL, post_hook)
     if sec2:
         sess1.compound([op.backchannel_ctl(env.c1.prog, sec2)])
-    fh = _create_file_with_deleg(sess1, env.testname(t), openaccess | want)
+    fh, stateid = _create_file_with_deleg(sess1, env.testname(t), openaccess | want)
     sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
     claim = open_claim4(CLAIM_NULL, env.testname(t))
     owner = open_owner4(0, b"My Open Owner 2")
@@ -62,6 +63,7 @@ def _testDeleg(t, env, openaccess, want, breakaccess, sec = None, sec2 = None):
     check(res, [NFS4_OK, NFS4ERR_DELAY])
     if not completed:
         fail("delegation break not received")
+    close_file(sess1, fh, stateid=stateid)
     return recall
 
 def testReadDeleg(t, env):
@@ -103,6 +105,7 @@ def testNoDeleg(t, env):
                       OPEN4_SHARE_ACCESS_WANT_NO_DELEG)
     check(res)
     fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
     deleg = res.resarray[-2].delegation
     if deleg.delegation_type == OPEN_DELEGATE_NONE:
         fail("Got no delegation, expected OPEN_DELEGATE_NONE_EXT")
@@ -110,6 +113,7 @@ def testNoDeleg(t, env):
         fail("Got a delegation (type "+str(deleg.delegation_type)+") despite asking for none")
     if deleg.ond_why != WND4_NOT_WANTED:
         fail("Wrong reason ("+str(deleg.ond_why)+") for giving no delegation")
+    close_file(sess1, fh, stateid=stateid)
 
 
 def testCBSecParms(t, env):
@@ -170,7 +174,7 @@ def testDelegRevocation(t, env):
     """
 
     sess1 = env.c1.new_client_session(b"%s_1" % env.testname(t))
-    fh, deleg = __create_file_with_deleg(sess1, env.testname(t),
+    fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t),
             OPEN4_SHARE_ACCESS_READ | OPEN4_SHARE_ACCESS_WANT_READ_DELEG)
     delegstateid = deleg.read.stateid
     sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
@@ -180,7 +184,7 @@ def testDelegRevocation(t, env):
     open_op = op.open(0, OPEN4_SHARE_ACCESS_WRITE, OPEN4_SHARE_DENY_NONE,
                         owner, how, claim)
     while 1:
-        res = sess2.compound(env.home + [open_op])
+        res = sess2.compound(env.home + [open_op, op.getfh()])
         if res.status == NFS4_OK:
             break;
         check(res, [NFS4_OK, NFS4ERR_DELAY])
@@ -188,6 +192,12 @@ def testDelegRevocation(t, env):
         # depend on the above compound waiting no longer than the
         # server's lease period:
         res = sess1.compound([])
+    if res.status == NFS4_OK:
+        fh2 = res.resarray[-1].object
+        stateid2 = res.resarray[-2].stateid
+    else:
+        fh2 = None
+        stateid2 = None
     res = sess1.compound([op.putfh(fh), op.read(delegstateid, 0, 1000)])
     check(res, NFS4ERR_DELEG_REVOKED, "Read with a revoked delegation")
     slot, seq_op = sess1._prepare_compound({})
@@ -217,6 +227,10 @@ def testDelegRevocation(t, env):
     if flags & ~SEQ4_STATUS_RECALLABLE_STATE_REVOKED:
         print("WARNING: unexpected status flag(s) 0x%x set" % flags)
 
+    close_file(sess1, fh, stateid=stateid)
+    if fh2 is not None and stateid2 is not None:
+        close_file(sess2, fh2, stateid=stateid2)
+
 def testWriteOpenvsReadDeleg(t, env):
     """Ensure that a write open prevents granting a read delegation
 
@@ -228,20 +242,28 @@ def testWriteOpenvsReadDeleg(t, env):
     owner = b"owner_%s" % env.testname(t)
     res = create_file(sess1, owner, access=OPEN4_SHARE_ACCESS_WRITE)
     check(res)
+    fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
 
     sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
     access = OPEN4_SHARE_ACCESS_READ | OPEN4_SHARE_ACCESS_WANT_READ_DELEG;
     res = open_file(sess2, owner, access = access)
     check(res)
+    fh2 = res.resarray[-1].object
+    stateid2 = res.resarray[-2].stateid
 
     deleg = res.resarray[-2].delegation
     if (not _got_deleg(deleg)):
         res = open_file(sess2, owner, access = access)
-        fh = res.resarray[-1].object
+        fh2 = res.resarray[-1].object
+        stateid2 = res.resarray[-2].stateid
         deleg = res.resarray[-2].delegation
     if (_got_deleg(deleg)):
         fail("Granted delegation to a file write-opened by another client")
 
+    close_file(sess1, fh, stateid=stateid)
+    close_file(sess2, fh2, stateid=stateid2)
+
 def testServerSelfConflict3(t, env):
     """DELEGATION test
 
@@ -264,13 +286,15 @@ def testServerSelfConflict3(t, env):
     sess1.client.cb_pre_hook(OP_CB_RECALL, pre_hook)
     sess1.client.cb_post_hook(OP_CB_RECALL, post_hook)
 
-    fh, deleg = __create_file_with_deleg(sess1, env.testname(t),
+    fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t),
             OPEN4_SHARE_ACCESS_READ | OPEN4_SHARE_ACCESS_WANT_READ_DELEG)
-    print("__create_file_with_deleg: ", fh, deleg)
+    print("__create_file_with_deleg: ", fh, stateid, deleg)
     delegstateid = deleg.read.stateid
     res = open_file(sess1, env.testname(t), access = OPEN4_SHARE_ACCESS_WRITE)
     print("open_file res: ", res)
     check(res)
+    fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
 
     # XXX: cut-n-paste from _testDeleg; make helper instead:
     sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
@@ -280,16 +304,26 @@ def testServerSelfConflict3(t, env):
     how = openflag4(OPEN4_NOCREATE)
     open_op = op.open(0, OPEN4_SHARE_ACCESS_WRITE,
                       OPEN4_SHARE_DENY_NONE, owner, how, claim)
-    slot = sess2.compound_async(env.home + [open_op])
+    slot = sess2.compound_async(env.home + [open_op, op.getfh()])
     completed = recall.wait(2)
     env.sleep(.1)
     res = sess1.compound([op.putfh(fh), op.delegreturn(delegstateid)])
     check(res)
     res = sess2.listen(slot)
     check(res, [NFS4_OK, NFS4ERR_DELAY])
+    if res.status == NFS4_OK:
+        fh2 = res.resarray[-1].object
+        stateid2 = res.resarray[-2].stateid
+    else:
+        fh2 = None
+        stateid2 = None
     if not completed:
         fail("delegation break not received")
 
+    close_file(sess1, fh, stateid=stateid)
+    if fh2 is not None and stateid2 is not None:
+        close_file(sess2, fh2, stateid=stateid2)
+
 def _testCbGetattr(t, env, change=0, size=0):
     cb = threading.Event()
     cbattrs = {}
@@ -318,8 +352,8 @@ def _testCbGetattr(t, env, change=0, size=0):
         if caps[FATTR4_OPEN_ARGUMENTS].oa_share_access_want & OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS:
             openmask |= 1<<OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS
 
-    fh, deleg = __create_file_with_deleg(sess1, env.testname(t), openmask)
-    print("__create_file_with_deleg: ", fh, deleg)
+    fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t), openmask)
+    print("__create_file_with_deleg: ", fh, stateid, deleg)
     attrs1 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
                                         FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
 
@@ -351,6 +385,7 @@ def _testCbGetattr(t, env, change=0, size=0):
     check(res, [NFS4_OK, NFS4ERR_DELAY])
     if not completed:
         fail("CB_GETATTR not received")
+    close_file(sess1, fh, stateid=stateid)
     return attrs1, attrs2
 
 def testCbGetattrNoChange(t, env):
diff --git a/nfs4.1/server41tests/st_sparse.py b/nfs4.1/server41tests/st_sparse.py
index 960ed5a..0c05e44 100644
--- a/nfs4.1/server41tests/st_sparse.py
+++ b/nfs4.1/server41tests/st_sparse.py
@@ -1,6 +1,6 @@
 from .st_create_session import create_session
 from xdrdef.nfs4_const import *
-from .environment import check, fail, create_file
+from .environment import check, fail, create_file, close_file
 import nfs_ops
 op = nfs_ops.NFS4ops()
 import nfs4lib
@@ -21,6 +21,8 @@ def testAllocateSupported(t, env):
     res = sess.compound([op.putfh(fh), op.allocate(stateid, 0, 1)])
     check(res)
 
+    close_file(sess, fh, stateid=stateid)
+
 def testAllocateStateidZero(t, env):
     """Do a simple ALLOCATE with all-zero stateid
 
@@ -31,10 +33,13 @@ def testAllocateStateidZero(t, env):
     sess = env.c1.new_client_session(env.testname(t))
     res = create_file(sess, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
     fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
 
     res = sess.compound([op.putfh(fh), op.allocate(env.stateid0, 0, 1)])
     check(res)
 
+    close_file(sess, fh, stateid=stateid)
+
 def testAllocateStateidOne(t, env):
     """Do a simple ALLOCATE with all-one stateid
 
@@ -45,6 +50,9 @@ def testAllocateStateidOne(t, env):
     sess = env.c1.new_client_session(env.testname(t))
     res = create_file(sess, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
     fh = res.resarray[-1].object
+    stateid = res.resarray[-2].stateid
 
     res = sess.compound([op.putfh(fh), op.allocate(env.stateid1, 0, 1)])
     check(res)
+
+    close_file(sess, fh, stateid=stateid)
-- 
2.53.0

[PATCH v2 2/5] pynfs: fix the check for delegated attribute support

[Scott Mayhew]

The DELEG24 and DELEG25 tests should work regardless of whether the
server supports delegated attributes or not.  fattr4_supported_attrs is
a bitmap4, so FATTR4_OPEN_ARGUMENTS needs to be shifted when checking to
see if that bit is set.  Fix it, and remove the now unnecessary check
added by 3216fc3 ("pynfs: fix key error if FATTR4_OPEN_ARGUMENTS is not
supported").

Cc: Chen Hanxiao <chenhx.fnst@fujitsu.com>
Cc: Jeff Layton <jlayton@kernel.org>
Fixes: f684000 ("st_deleg: test delegated timestamps in CB_GETATTR")
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs4.1/server41tests/st_delegation.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
index d41f12d..934e558 100644
--- a/nfs4.1/server41tests/st_delegation.py
+++ b/nfs4.1/server41tests/st_delegation.py
@@ -345,10 +345,7 @@ def _testCbGetattr(t, env, change=0, size=0):
                 OPEN4_SHARE_ACCESS_WRITE |
                 OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG)
 
-    if FATTR4_OPEN_ARGUMENTS not in caps:
-        fail("FATTR4_OPEN_ARGUMENTS not supported")
-
-    if caps[FATTR4_SUPPORTED_ATTRS] & FATTR4_OPEN_ARGUMENTS:
+    if caps[FATTR4_SUPPORTED_ATTRS] & (1 << FATTR4_OPEN_ARGUMENTS):
         if caps[FATTR4_OPEN_ARGUMENTS].oa_share_access_want & OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS:
             openmask |= 1<<OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS
 
-- 
2.53.0

[PATCH v2 3/5] pynfs: _testCbGetattr() should check the delegation that was received

[Scott Mayhew]

It currently assumes that if it requested an attribute delegation, then
that's what it received.  Better to actually check it instead.

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs4.1/server41tests/st_delegation.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
index 934e558..754b56c 100644
--- a/nfs4.1/server41tests/st_delegation.py
+++ b/nfs4.1/server41tests/st_delegation.py
@@ -351,6 +351,9 @@ def _testCbGetattr(t, env, change=0, size=0):
 
     fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t), openmask)
     print("__create_file_with_deleg: ", fh, stateid, deleg)
+    delegtype = deleg.delegation_type
+    if delegtype != OPEN_DELEGATE_WRITE_ATTRS_DELEG and delegtype != OPEN_DELEGATE_WRITE:
+        fail("Didn't get a write delegation.")
     attrs1 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
                                         FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
 
@@ -362,7 +365,7 @@ def _testCbGetattr(t, env, change=0, size=0):
         if size > 0:
             cbattrs[FATTR4_SIZE] = size
 
-    if openmask & 1<<OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS:
+    if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG:
         cbattrs[FATTR4_TIME_DELEG_ACCESS] = attrs1[FATTR4_TIME_ACCESS]
         cbattrs[FATTR4_TIME_DELEG_MODIFY] = attrs1[FATTR4_TIME_MODIFY]
         if change != 0:
-- 
2.53.0

[PATCH v2 4/5] pynfs: fix erroneous test in DELEG24 and DELEG25

[Scott Mayhew]

fattr4_time_deleg_modify is valid in CB_GETATTR and SETATTR.  attrs2
contains the attributes from a GETATTR reply, and will never contain the
fattr4_time_deleg_modify attribute.  Instead, we want to compare
fattr4_time_modify from attrs1 and attrs2.

Fixing that revealed a secondary issue.  In pynfs, these attributes are
nfstime4 objects, so:

1. We need to be careful about how we initialize the timestamps in
   cbattrs.  Otherwise we wind up indadvertently modifying the values in
   attrs1 and causing the DELEG25 test to fail.
2. We need compare the instance attributes (seconds, nseconds) rather
   than the object references (environment.py already has a
   compareTimes() function for this).

Finally, since I'm in there anyway I decided to convert the messages
from legacy printf-style formatting to f-strings.

Cc: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs4.1/server41tests/st_delegation.py | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
index 754b56c..bbf6925 100644
--- a/nfs4.1/server41tests/st_delegation.py
+++ b/nfs4.1/server41tests/st_delegation.py
@@ -2,7 +2,7 @@ from .st_create_session import create_session
 from .st_open import open_claim4
 from xdrdef.nfs4_const import *
 
-from .environment import check, fail, create_file, open_file, close_file, do_getattrdict, close_file, write_file, read_file
+from .environment import check, fail, create_file, open_file, close_file, do_getattrdict, close_file, write_file, read_file, compareTimes
 from xdrdef.nfs4_type import *
 import nfs_ops
 op = nfs_ops.NFS4ops()
@@ -366,8 +366,10 @@ def _testCbGetattr(t, env, change=0, size=0):
             cbattrs[FATTR4_SIZE] = size
 
     if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG:
-        cbattrs[FATTR4_TIME_DELEG_ACCESS] = attrs1[FATTR4_TIME_ACCESS]
-        cbattrs[FATTR4_TIME_DELEG_MODIFY] = attrs1[FATTR4_TIME_MODIFY]
+        cbattrs[FATTR4_TIME_DELEG_ACCESS] = nfstime4(attrs1[FATTR4_TIME_ACCESS].seconds,
+                                                     attrs1[FATTR4_TIME_ACCESS].nseconds)
+        cbattrs[FATTR4_TIME_DELEG_MODIFY] = nfstime4(attrs1[FATTR4_TIME_MODIFY].seconds,
+                                                     attrs1[FATTR4_TIME_MODIFY].nseconds)
         if change != 0:
             cbattrs[FATTR4_TIME_DELEG_ACCESS].seconds += 1
             cbattrs[FATTR4_TIME_DELEG_MODIFY].seconds += 1
@@ -400,12 +402,11 @@ def testCbGetattrNoChange(t, env):
     """
     attrs1, attrs2 = _testCbGetattr(t, env)
     if attrs1[FATTR4_SIZE] != attrs2[FATTR4_SIZE]:
-        fail("Bad size: %u != %u" % (attrs1[FATTR4_SIZE], attrs2[FATTR4_SIZE]))
+        fail(f"Bad size: {attrs1[FATTR4_SIZE]} != {attrs2[FATTR4_SIZE]}")
     if attrs1[FATTR4_CHANGE] != attrs2[FATTR4_CHANGE]:
-        fail("Bad change attribute: %u != %u" % (attrs1[FATTR4_CHANGE], attrs2[FATTR4_CHANGE]))
-    if FATTR4_TIME_DELEG_MODIFY in attrs2:
-        if attrs1[FATTR4_TIME_MODIFY] != attrs2[FATTR4_TIME_DELEG_MODIFY]:
-            fail("Bad modify time: ", attrs1[FATTR4_TIME_MODIFY], " != ", attrs2[FATTR4_TIME_DELEG_MODIFY])
+        fail(f"Bad change attribute: {attrs1[FATTR4_CHANGE]} != {attrs2[FATTR4_CHANGE]}")
+    if compareTimes(attrs1[FATTR4_TIME_MODIFY], attrs2[FATTR4_TIME_MODIFY]) != 0:
+        fail(f"Bad modify time: {attrs1[FATTR4_TIME_MODIFY]} != {attrs2[FATTR4_TIME_MODIFY]}")
 
 def testCbGetattrWithChange(t, env):
     """Test CB_GETATTR with simulated changes to file
@@ -419,12 +420,11 @@ def testCbGetattrWithChange(t, env):
     """
     attrs1, attrs2 = _testCbGetattr(t, env, change=1, size=5)
     if attrs2[FATTR4_SIZE] != 5:
-        fail("Bad size: %u != 5" % attrs2[FATTR4_SIZE])
+        fail(f"Bad size: {attrs2[FATTR4_SIZE]} != 5")
     if attrs1[FATTR4_CHANGE] == attrs2[FATTR4_CHANGE]:
-        fail("Bad change attribute: %u == %u" % (attrs1[FATTR4_CHANGE], attrs2[FATTR4_CHANGE]))
-    if FATTR4_TIME_DELEG_MODIFY in attrs2:
-        if attrs1[FATTR4_TIME_MODIFY] == attrs2[FATTR4_TIME_DELEG_MODIFY]:
-            fail("Bad modify time: ", attrs1[FATTR4_TIME_MODIFY], " == ", attrs2[FATTR4_TIME_DELEG_MODIFY])
+        fail(f"Bad change attribute: {attrs1[FATTR4_CHANGE]} == {attrs2[FATTR4_CHANGE]}")
+    if compareTimes(attrs1[FATTR4_TIME_MODIFY], attrs2[FATTR4_TIME_MODIFY]) == 0:
+        fail(f"Bad modify time: {attrs1[FATTR4_TIME_MODIFY]} == {attrs2[FATTR4_TIME_MODIFY]}")
 
 def testDelegReadAfterClose(t, env):
     """Test read with delegation stateid after close
-- 
2.53.0

[PATCH v2] pynfs: add delegation test for CB_GETATTR after sync WRITE

[Scott Mayhew]

DELEG27 tests the scenario where a client has written data to the server
while holding a write delegation, but is not *currently* holding
modified data in its cache.

In this case, the CB_GETATTR should not trigger an mtime update (the
time_modify that client1 gets in the GETATTR after the WRITE should
match the time_modify it gets in the GETATTR in the DELEGRETURN
compound).

Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 nfs4.1/server41tests/st_delegation.py | 151 ++++++++++++++++++++++++++
 1 file changed, 151 insertions(+)

diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
index bbf6925..6a08950 100644
--- a/nfs4.1/server41tests/st_delegation.py
+++ b/nfs4.1/server41tests/st_delegation.py
@@ -8,6 +8,8 @@ import nfs_ops
 op = nfs_ops.NFS4ops()
 import nfs4lib
 import threading
+import copy
+import time
 
 def _got_deleg(deleg):
     return (deleg.delegation_type != OPEN_DELEGATE_NONE and
@@ -476,3 +478,152 @@ def testDelegReadAfterClose(t, env):
     # cleanup: return delegation
     res = sess1.compound([op.putfh(fh), op.delegreturn(delegstateid)])
     check(res)
+
+def testCbGetattrAfterSyncWrite(t, env):
+    """Test CB_GETATTR after a FILE_SYNC4 WRITE
+
+    1. Client 1 opens a file (getting a write deleg or a write attrs deleg) and
+       does a GETATTR
+    2. Client 1 does a FILE_SYNC4 WRITE.  If we got a write delegation, it
+       follows this up with a GETATTR.  Otherwise we got a write attrs deleg
+       and we construct the attrs ourself.
+    3. Client 2 does a GETATTR, triggering a CB_GETATTR to client 1.  Client 2
+       then does an OPEN, triggering a CB_RECALL to client 1.
+    4. Client 1 does a PUTFH|SETATTR|GETATTR|DELEGRETURN if we have a write
+       attrs deleg, otherwise it does a PUTFH|GETATTR|DELEGRETURN.
+
+    time_modify should only change between steps 1 and 2.  It should not change
+    from steps 2 thru 4.
+
+    FLAGS: deleg all
+    CODE: DELEG27
+    """
+    cb = threading.Event()
+    cbattrs = {}
+    def getattr_post_hook(arg, env, res):
+        res.obj_attributes = cbattrs
+        env.notify = cb.set
+        return res
+
+    recall = threading.Event()
+    def recall_pre_hook(arg, env):
+        recall.stateid = arg.stateid
+        recall.cred = env.cred.raw_cred
+        env.notify = recall.set
+    def recall_post_hook(arg, env, res):
+        return res
+
+    size = 5
+
+    sess1 = env.c1.new_client_session(b"%s_1" % env.testname(t))
+    sess1.client.cb_post_hook(OP_CB_GETATTR, getattr_post_hook)
+    sess1.client.cb_pre_hook(OP_CB_RECALL, recall_pre_hook)
+    sess1.client.cb_post_hook(OP_CB_RECALL, recall_post_hook)
+
+    res = sess1.compound([op.putrootfh(),
+                          op.getattr(nfs4lib.list2bitmap([FATTR4_SUPPORTED_ATTRS,
+                                                          FATTR4_OPEN_ARGUMENTS]))])
+    check(res)
+    caps = res.resarray[-1].obj_attributes
+
+    openmask = (OPEN4_SHARE_ACCESS_READ  |
+                OPEN4_SHARE_ACCESS_WRITE |
+                OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG)
+
+    if caps[FATTR4_SUPPORTED_ATTRS] & (1 << FATTR4_OPEN_ARGUMENTS):
+        if caps[FATTR4_OPEN_ARGUMENTS].oa_share_access_want & OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS:
+            openmask |= 1<<OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS
+
+    fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t), openmask)
+    delegtype = deleg.delegation_type
+    if delegtype != OPEN_DELEGATE_WRITE_ATTRS_DELEG and delegtype != OPEN_DELEGATE_WRITE:
+        fail("Didn't get a write delegation.")
+    delegstateid = deleg.write.stateid
+
+    attrs1 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
+                                        FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
+
+    cbattrs[FATTR4_CHANGE] = attrs1[FATTR4_CHANGE]
+    cbattrs[FATTR4_SIZE] = attrs1[FATTR4_SIZE]
+
+    env.sleep(1)
+    res = write_file(sess1, fh, b'z' * size, 0, delegstateid)
+    check(res)
+
+    if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG:
+        attrs2 = copy.deepcopy(attrs1)
+        now = divmod(time.time_ns(), 1000000000)
+        attrs2[FATTR4_TIME_ACCESS] = nfstime4(*now)
+        attrs2[FATTR4_TIME_MODIFY] = nfstime4(*now)
+        cbattrs[FATTR4_TIME_DELEG_ACCESS] = nfstime4(*now)
+        cbattrs[FATTR4_TIME_DELEG_MODIFY] = nfstime4(*now)
+    else:
+        attrs2 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
+                                            FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
+
+    # No need to bump FATTR4_CHANGE because we've already flushed our data
+    cbattrs[FATTR4_SIZE] = size
+
+    sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
+    slot = sess2.compound_async([op.putfh(fh),
+                                 op.getattr(1<<FATTR4_CHANGE |
+                                            1<<FATTR4_SIZE |
+                                            1<<FATTR4_TIME_ACCESS |
+                                            1<<FATTR4_TIME_MODIFY)])
+
+    completed = cb.wait(2)
+    if not completed:
+        fail("CB_GETATTR not received")
+
+    res = sess2.listen(slot)
+    check(res)
+    attrs3 = res.resarray[-1].obj_attributes
+
+    claim = open_claim4(CLAIM_NULL, env.testname(t))
+    owner = open_owner4(0, b"owner")
+    how = openflag4(OPEN4_NOCREATE)
+    open_op = op.open(0, OPEN4_SHARE_ACCESS_WRITE,
+                      OPEN4_SHARE_DENY_NONE, owner, how, claim)
+    slot = sess2.compound_async(env.home + [open_op, op.getfh()])
+    completed = recall.wait(2)
+    if not completed:
+        fail("CB_RECALL not received")
+
+    env.sleep(.1)
+
+    # Note if we have a write attrs deleg we should do a setattr before the
+    # delegreturn (see RFC 9754, section 5)
+    res = sess1.compound([op.putfh(fh),
+                          *([op.setattr(delegstateid,
+                                        {FATTR4_TIME_DELEG_ACCESS: cbattrs[FATTR4_TIME_DELEG_ACCESS],
+                                         FATTR4_TIME_DELEG_MODIFY: cbattrs[FATTR4_TIME_DELEG_MODIFY]})]
+                              if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG else []),
+                          op.getattr(1<<FATTR4_CHANGE |
+                                     1<<FATTR4_SIZE |
+                                     1<<FATTR4_TIME_ACCESS |
+                                     1<<FATTR4_TIME_MODIFY),
+                          op.delegreturn(delegstateid)])
+    check(res)
+    attrs4 = res.resarray[-2].obj_attributes
+
+    res = sess2.listen(slot)
+    check(res, [NFS4_OK, NFS4ERR_DELAY])
+    if res.status == NFS4_OK:
+        fh2 = res.resarray[-1].object
+        stateid2 = res.resarray[-2].stateid
+    else:
+        fh2 = None
+        stateid2 = None
+
+    close_file(sess1, fh, stateid=stateid)
+    if fh2 is not None and stateid2 is not None:
+        close_file(sess2, fh2, stateid=stateid2)
+
+    #print(f"attrs1: size {attrs1[FATTR4_SIZE]} change {attrs1[FATTR4_CHANGE]} mtime {attrs1[FATTR4_TIME_MODIFY]}")
+    #print(f"attrs2: size {attrs2[FATTR4_SIZE]} change {attrs2[FATTR4_CHANGE]} mtime {attrs2[FATTR4_TIME_MODIFY]}")
+    #print(f"attrs3: size {attrs3[FATTR4_SIZE]} change {attrs3[FATTR4_CHANGE]} mtime {attrs3[FATTR4_TIME_MODIFY]}")
+    #print(f"attrs4: size {attrs4[FATTR4_SIZE]} change {attrs4[FATTR4_CHANGE]} mtime {attrs4[FATTR4_TIME_MODIFY]}")
+
+    if compareTimes(attrs2[FATTR4_TIME_MODIFY], attrs4[FATTR4_TIME_MODIFY]) != 0:
+        fail(f"mtime after write ({attrs2[FATTR4_TIME_MODIFY]}) != "
+             f"mtime from delegreturn ({attrs4[FATTR4_TIME_MODIFY]})")
-- 
2.53.0

Re: [PATCH v2] pynfs: add delegation test for CB_GETATTR after sync WRITE

[Scott Mayhew]

My bad, the subject should say [PATCH v2 5/5]...

On Fri, 03 Apr 2026, Scott Mayhew wrote:

> DELEG27 tests the scenario where a client has written data to the server
> while holding a write delegation, but is not *currently* holding
> modified data in its cache.
> 
> In this case, the CB_GETATTR should not trigger an mtime update (the
> time_modify that client1 gets in the GETATTR after the WRITE should
> match the time_modify it gets in the GETATTR in the DELEGRETURN
> compound).
> 
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  nfs4.1/server41tests/st_delegation.py | 151 ++++++++++++++++++++++++++
>  1 file changed, 151 insertions(+)
> 
> diff --git a/nfs4.1/server41tests/st_delegation.py b/nfs4.1/server41tests/st_delegation.py
> index bbf6925..6a08950 100644
> --- a/nfs4.1/server41tests/st_delegation.py
> +++ b/nfs4.1/server41tests/st_delegation.py
> @@ -8,6 +8,8 @@ import nfs_ops
>  op = nfs_ops.NFS4ops()
>  import nfs4lib
>  import threading
> +import copy
> +import time
>  
>  def _got_deleg(deleg):
>      return (deleg.delegation_type != OPEN_DELEGATE_NONE and
> @@ -476,3 +478,152 @@ def testDelegReadAfterClose(t, env):
>      # cleanup: return delegation
>      res = sess1.compound([op.putfh(fh), op.delegreturn(delegstateid)])
>      check(res)
> +
> +def testCbGetattrAfterSyncWrite(t, env):
> +    """Test CB_GETATTR after a FILE_SYNC4 WRITE
> +
> +    1. Client 1 opens a file (getting a write deleg or a write attrs deleg) and
> +       does a GETATTR
> +    2. Client 1 does a FILE_SYNC4 WRITE.  If we got a write delegation, it
> +       follows this up with a GETATTR.  Otherwise we got a write attrs deleg
> +       and we construct the attrs ourself.
> +    3. Client 2 does a GETATTR, triggering a CB_GETATTR to client 1.  Client 2
> +       then does an OPEN, triggering a CB_RECALL to client 1.
> +    4. Client 1 does a PUTFH|SETATTR|GETATTR|DELEGRETURN if we have a write
> +       attrs deleg, otherwise it does a PUTFH|GETATTR|DELEGRETURN.
> +
> +    time_modify should only change between steps 1 and 2.  It should not change
> +    from steps 2 thru 4.
> +
> +    FLAGS: deleg all
> +    CODE: DELEG27
> +    """
> +    cb = threading.Event()
> +    cbattrs = {}
> +    def getattr_post_hook(arg, env, res):
> +        res.obj_attributes = cbattrs
> +        env.notify = cb.set
> +        return res
> +
> +    recall = threading.Event()
> +    def recall_pre_hook(arg, env):
> +        recall.stateid = arg.stateid
> +        recall.cred = env.cred.raw_cred
> +        env.notify = recall.set
> +    def recall_post_hook(arg, env, res):
> +        return res
> +
> +    size = 5
> +
> +    sess1 = env.c1.new_client_session(b"%s_1" % env.testname(t))
> +    sess1.client.cb_post_hook(OP_CB_GETATTR, getattr_post_hook)
> +    sess1.client.cb_pre_hook(OP_CB_RECALL, recall_pre_hook)
> +    sess1.client.cb_post_hook(OP_CB_RECALL, recall_post_hook)
> +
> +    res = sess1.compound([op.putrootfh(),
> +                          op.getattr(nfs4lib.list2bitmap([FATTR4_SUPPORTED_ATTRS,
> +                                                          FATTR4_OPEN_ARGUMENTS]))])
> +    check(res)
> +    caps = res.resarray[-1].obj_attributes
> +
> +    openmask = (OPEN4_SHARE_ACCESS_READ  |
> +                OPEN4_SHARE_ACCESS_WRITE |
> +                OPEN4_SHARE_ACCESS_WANT_WRITE_DELEG)
> +
> +    if caps[FATTR4_SUPPORTED_ATTRS] & (1 << FATTR4_OPEN_ARGUMENTS):
> +        if caps[FATTR4_OPEN_ARGUMENTS].oa_share_access_want & OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS:
> +            openmask |= 1<<OPEN_ARGS_SHARE_ACCESS_WANT_DELEG_TIMESTAMPS
> +
> +    fh, stateid, deleg = __create_file_with_deleg(sess1, env.testname(t), openmask)
> +    delegtype = deleg.delegation_type
> +    if delegtype != OPEN_DELEGATE_WRITE_ATTRS_DELEG and delegtype != OPEN_DELEGATE_WRITE:
> +        fail("Didn't get a write delegation.")
> +    delegstateid = deleg.write.stateid
> +
> +    attrs1 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
> +                                        FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
> +
> +    cbattrs[FATTR4_CHANGE] = attrs1[FATTR4_CHANGE]
> +    cbattrs[FATTR4_SIZE] = attrs1[FATTR4_SIZE]
> +
> +    env.sleep(1)
> +    res = write_file(sess1, fh, b'z' * size, 0, delegstateid)
> +    check(res)
> +
> +    if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG:
> +        attrs2 = copy.deepcopy(attrs1)
> +        now = divmod(time.time_ns(), 1000000000)
> +        attrs2[FATTR4_TIME_ACCESS] = nfstime4(*now)
> +        attrs2[FATTR4_TIME_MODIFY] = nfstime4(*now)
> +        cbattrs[FATTR4_TIME_DELEG_ACCESS] = nfstime4(*now)
> +        cbattrs[FATTR4_TIME_DELEG_MODIFY] = nfstime4(*now)
> +    else:
> +        attrs2 = do_getattrdict(sess1, fh, [FATTR4_CHANGE, FATTR4_SIZE,
> +                                            FATTR4_TIME_ACCESS, FATTR4_TIME_MODIFY])
> +
> +    # No need to bump FATTR4_CHANGE because we've already flushed our data
> +    cbattrs[FATTR4_SIZE] = size
> +
> +    sess2 = env.c1.new_client_session(b"%s_2" % env.testname(t))
> +    slot = sess2.compound_async([op.putfh(fh),
> +                                 op.getattr(1<<FATTR4_CHANGE |
> +                                            1<<FATTR4_SIZE |
> +                                            1<<FATTR4_TIME_ACCESS |
> +                                            1<<FATTR4_TIME_MODIFY)])
> +
> +    completed = cb.wait(2)
> +    if not completed:
> +        fail("CB_GETATTR not received")
> +
> +    res = sess2.listen(slot)
> +    check(res)
> +    attrs3 = res.resarray[-1].obj_attributes
> +
> +    claim = open_claim4(CLAIM_NULL, env.testname(t))
> +    owner = open_owner4(0, b"owner")
> +    how = openflag4(OPEN4_NOCREATE)
> +    open_op = op.open(0, OPEN4_SHARE_ACCESS_WRITE,
> +                      OPEN4_SHARE_DENY_NONE, owner, how, claim)
> +    slot = sess2.compound_async(env.home + [open_op, op.getfh()])
> +    completed = recall.wait(2)
> +    if not completed:
> +        fail("CB_RECALL not received")
> +
> +    env.sleep(.1)
> +
> +    # Note if we have a write attrs deleg we should do a setattr before the
> +    # delegreturn (see RFC 9754, section 5)
> +    res = sess1.compound([op.putfh(fh),
> +                          *([op.setattr(delegstateid,
> +                                        {FATTR4_TIME_DELEG_ACCESS: cbattrs[FATTR4_TIME_DELEG_ACCESS],
> +                                         FATTR4_TIME_DELEG_MODIFY: cbattrs[FATTR4_TIME_DELEG_MODIFY]})]
> +                              if delegtype == OPEN_DELEGATE_WRITE_ATTRS_DELEG else []),
> +                          op.getattr(1<<FATTR4_CHANGE |
> +                                     1<<FATTR4_SIZE |
> +                                     1<<FATTR4_TIME_ACCESS |
> +                                     1<<FATTR4_TIME_MODIFY),
> +                          op.delegreturn(delegstateid)])
> +    check(res)
> +    attrs4 = res.resarray[-2].obj_attributes
> +
> +    res = sess2.listen(slot)
> +    check(res, [NFS4_OK, NFS4ERR_DELAY])
> +    if res.status == NFS4_OK:
> +        fh2 = res.resarray[-1].object
> +        stateid2 = res.resarray[-2].stateid
> +    else:
> +        fh2 = None
> +        stateid2 = None
> +
> +    close_file(sess1, fh, stateid=stateid)
> +    if fh2 is not None and stateid2 is not None:
> +        close_file(sess2, fh2, stateid=stateid2)
> +
> +    #print(f"attrs1: size {attrs1[FATTR4_SIZE]} change {attrs1[FATTR4_CHANGE]} mtime {attrs1[FATTR4_TIME_MODIFY]}")
> +    #print(f"attrs2: size {attrs2[FATTR4_SIZE]} change {attrs2[FATTR4_CHANGE]} mtime {attrs2[FATTR4_TIME_MODIFY]}")
> +    #print(f"attrs3: size {attrs3[FATTR4_SIZE]} change {attrs3[FATTR4_CHANGE]} mtime {attrs3[FATTR4_TIME_MODIFY]}")
> +    #print(f"attrs4: size {attrs4[FATTR4_SIZE]} change {attrs4[FATTR4_CHANGE]} mtime {attrs4[FATTR4_TIME_MODIFY]}")
> +
> +    if compareTimes(attrs2[FATTR4_TIME_MODIFY], attrs4[FATTR4_TIME_MODIFY]) != 0:
> +        fail(f"mtime after write ({attrs2[FATTR4_TIME_MODIFY]}) != "
> +             f"mtime from delegreturn ({attrs4[FATTR4_TIME_MODIFY]})")
> -- 
> 2.53.0
> 
>