[pynfs PATCH v2 0/5] An assortment of pynfs patches

[pynfs PATCH v2 0/5] An assortment of pynfs patches

[Jeff Layton]

v2:
- instead of changing the meaning of the "all" flag, add a new
  "everything" flag.
- add patch to fix LOCK24

I sent some of these a couple of weeks ago, but didn't Cc Calum (the new
maintainer) and a couple of them needed some changes. This set should
address the concerns about changing the meaning of "all" and adds a
new patch that fixes LOCK24 on knfsd.

Jeff Layton (5):
  nfs4.0: add a retry loop on NFS4ERR_DELAY to compound function
  examples: add a new example localhost_helper.sh script
  nfs4.0/testserver.py: don't return an error when tests fail
  testserver.py: add a new (special) "everything" flag
  LOCK24: fix the lock_seqid in second lock request

 examples/localhost_helper.sh  | 29 +++++++++++++++++++++++++++++
 nfs4.0/nfs4lib.py             | 21 +++++++++++++++------
 nfs4.0/servertests/st_lock.py |  6 +++++-
 nfs4.0/testserver.py          |  2 --
 nfs4.1/testmod.py             |  2 ++
 5 files changed, 51 insertions(+), 9 deletions(-)
 create mode 100755 examples/localhost_helper.sh

-- 
2.39.2

[pynfs PATCH v2 1/5] nfs4.0: add a retry loop on NFS4ERR_DELAY to compound function

[Jeff Layton]

The latest knfsd server is "courteous" in that it will not revoke a
lease held by an expired client until there is competing access for it.
When there is competing access, it can now return NFS4ERR_DELAY until
the old client is expired. I've seen this happen when running pynfs in
a loop against a server with only 4g of memory.

The v4.0 compound handler doesn't retry automatically on NFS4ERR_DELAY
like the v4.1 version does. Add support for it using the same timeouts
as the v4.1 compound handler.

Cc: Dai Ngo <dai.ngo@oracle.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 nfs4.0/nfs4lib.py | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/nfs4.0/nfs4lib.py b/nfs4.0/nfs4lib.py
index 9b074f02b91f..eddcd862bc2f 100644
--- a/nfs4.0/nfs4lib.py
+++ b/nfs4.0/nfs4lib.py
@@ -338,12 +338,21 @@ class NFS4Client(rpc.RPCClient):
         un_p = self.nfs4unpacker
         p.reset()
         p.pack_COMPOUND4args(compoundargs)
-        res = self.call(NFSPROC4_COMPOUND, p.get_buffer())
-        un_p.reset(res)
-        res = un_p.unpack_COMPOUND4res()
-        if SHOW_TRAFFIC:
-            print(res)
-        un_p.done()
+        res = None
+
+        # NFS servers can return NFS4ERR_DELAY at any time for any reason.
+        # Just delay a second and retry the call again in that event. If
+        # it fails after 10 retries then just give up.
+        for i in range(1, 10):
+            res = self.call(NFSPROC4_COMPOUND, p.get_buffer())
+            un_p.reset(res)
+            res = un_p.unpack_COMPOUND4res()
+            if SHOW_TRAFFIC:
+                print(res)
+            un_p.done()
+            if res.status != NFS4ERR_DELAY:
+                break
+            time.sleep(1)
 
         # Do some error checking
 
-- 
2.39.2

[pynfs PATCH v2 2/5] examples: add a new example localhost_helper.sh script

[Jeff Layton]

It's possible (and pretty common) to run pynfs against the server
running on the same host. Add a new serverhelper script for that
purpose.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 examples/localhost_helper.sh | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100755 examples/localhost_helper.sh

diff --git a/examples/localhost_helper.sh b/examples/localhost_helper.sh
new file mode 100755
index 000000000000..6db123311e7a
--- /dev/null
+++ b/examples/localhost_helper.sh
@@ -0,0 +1,29 @@
+#!/bin/bash
+#
+# serverhelper script for running tests against knfsd running on localhost.
+# Note that this requires that the running user can use sudo to restart nfsd
+# without a password.
+#
+
+# server argument is ignored here
+server=$1
+command=$2
+shift; shift
+
+case $command in
+reboot )
+	sudo systemctl restart nfs-server.service
+	;;
+unlink )
+	rm $1
+	;;
+rename )
+	mv $1 $2
+	;;
+link )
+	ln $1 $2
+	;;
+chmod )
+	chmod $1 $2
+	;;
+esac
-- 
2.39.2

[pynfs PATCH v2 3/5] nfs4.0/testserver.py: don't return an error when tests fail

[Jeff Layton]

This script was originally changed in eb3ba0b60055 ("Have testserver.py
have non-zero exit code if any tests fail"), but the same change wasn't
made to the 4.1 testserver.py script.

There also wasn't much explanation for it, and it makes it difficult to
tell whether the test harness itself failed, or whether there was a
failure in a requested test.

Stop the 4.0 testserver.py from exiting with an error code when a test
fails, so that a successful return means only that the test harness
itself worked, not that every requested test passed.

Cc: Frank Filz <ffilzlnx@mindspring.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 nfs4.0/testserver.py | 2 --
 1 file changed, 2 deletions(-)

diff --git a/nfs4.0/testserver.py b/nfs4.0/testserver.py
index f2c41568e5c7..4f4286daa657 100755
--- a/nfs4.0/testserver.py
+++ b/nfs4.0/testserver.py
@@ -387,8 +387,6 @@ def main():
 
     if nfail < 0:
         sys.exit(3)
-    if nfail > 0:
-        sys.exit(2)
 
 if __name__ == "__main__":
     main()
-- 
2.39.2

[pynfs PATCH v2 4/5] testserver.py: add a new (special) "everything" flag

[Jeff Layton]

The READMEs for v4.0 and v4.1 are inconsistent here. For v4.0, the "all"
flag is supposed to run all of the "standard" tests. For v4.1 "all" is
documented to run all of the tests, but it actually doesn't since not
every tests has "all" in its FLAGS: field.

I really want to be able to run _all_ the tests sometimes. Add a special
case new "everything" flag that is automatically added to every test.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 nfs4.1/testmod.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/nfs4.1/testmod.py b/nfs4.1/testmod.py
index 11e759d673fd..b3174006a0a3 100644
--- a/nfs4.1/testmod.py
+++ b/nfs4.1/testmod.py
@@ -386,6 +386,8 @@ def createtests(testdir):
     for t in tests:
 ##         if not t.flags_list:
 ##             raise RuntimeError("%s has no flags" % t.fullname)
+        if "everything" not in t.flags_list:
+            t.flags_list.append("everything")
         for f in t.flags_list:
             if f not in flag_dict:
                 flag_dict[f] = bit
-- 
2.39.2

[pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Jeff Layton]

This test currently fails against Linux nfsd, but I think it's the test
that's wrong. It basically does:

open for read
read lock
unlock
open upgrade
write lock

The write lock above is sent with a lock_seqid of 0, which is wrong.
RFC7530/16.10.5 says:

   o  In the case in which the state has been created and the [new
      lockowner] boolean is true, the server rejects the request with the
      error NFS4ERR_BAD_SEQID.  The only exception is where there is a
      retransmission of a previous request in which the boolean was
      true.  In this case, the lock_seqid will match the original
      request, and the response will reflect the final case, below.

Since the above is not a retransmission, knfsd is correct to reject
this call. This patch fixes the open_sequence object to track the lock
seqid and set it correctly in the LOCK request.

With this, LOCK24 passes against knfsd.

Cc: Frank Filz <ffilz@redhat.com>
Fixes: 4299316fb357 (Add LOCK24 test case to test open uprgade/downgrade scenario)
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 nfs4.0/servertests/st_lock.py | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/nfs4.0/servertests/st_lock.py b/nfs4.0/servertests/st_lock.py
index 468672403ffe..9d650ab017b9 100644
--- a/nfs4.0/servertests/st_lock.py
+++ b/nfs4.0/servertests/st_lock.py
@@ -886,6 +886,7 @@ class open_sequence:
         self.client = client
         self.owner = owner
         self.lockowner = lockowner
+        self.lockseqid = 0
     def open(self, access):
         self.fh, self.stateid = self.client.create_confirm(self.owner,
 						access=access,
@@ -900,14 +901,17 @@ class open_sequence:
         self.client.close_file(self.owner, self.fh, self.stateid)
     def lock(self, type):
         res = self.client.lock_file(self.owner, self.fh, self.stateid,
-                    type=type, lockowner=self.lockowner)
+                                    type=type, lockowner=self.lockowner,
+                                    lockseqid=self.lockseqid)
         check(res)
         if res.status == NFS4_OK:
             self.lockstateid = res.lockid
+            self.lockseqid += 1
     def unlock(self):
         res = self.client.unlock_file(1, self.fh, self.lockstateid)
         if res.status == NFS4_OK:
             self.lockstateid = res.lockid
+            self.lockseqid += 1
 
 def testOpenUpgradeLock(t, env):
     """Try open, lock, open, downgrade, close
-- 
2.39.2

Re: [pynfs PATCH v2 0/5] An assortment of pynfs patches

[Calum Mackay]

[-- Attachment #1.1: Type: text/plain, Size: 1303 bytes --]

On 13/03/2023 11:23 am, Jeff Layton wrote:
> v2:
> - instead of changing the meaning of the "all" flag, add a new
>    "everything" flag.
> - add patch to fix LOCK24
> 
> I sent some of these a couple of weeks ago, but didn't Cc Calum (the new
> maintainer) and a couple of them needed some changes. This set should
> address the concerns about changing the meaning of "all" and adds a
> new patch that fixes LOCK24 on knfsd.

Thanks very much, Jeff.

I don't have my repo set-up yet, but hopefully soon, and will get to 
these (and others) asap.

cheers,
calum.

> 
> Jeff Layton (5):
>    nfs4.0: add a retry loop on NFS4ERR_DELAY to compound function
>    examples: add a new example localhost_helper.sh script
>    nfs4.0/testserver.py: don't return an error when tests fail
>    testserver.py: add a new (special) "everything" flag
>    LOCK24: fix the lock_seqid in second lock request
> 
>   examples/localhost_helper.sh  | 29 +++++++++++++++++++++++++++++
>   nfs4.0/nfs4lib.py             | 21 +++++++++++++++------
>   nfs4.0/servertests/st_lock.py |  6 +++++-
>   nfs4.0/testserver.py          |  2 --
>   nfs4.1/testmod.py             |  2 ++
>   5 files changed, 51 insertions(+), 9 deletions(-)
>   create mode 100755 examples/localhost_helper.sh
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

RE: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Frank Filz]

Looks good to me, tested against Ganesha and the updated patch passes.

Frank

> -----Original Message-----
> From: Jeff Layton [mailto:jlayton@kernel.org]
> Sent: Monday, March 13, 2023 4:24 AM
> To: calum.mackay@oracle.com
> Cc: bfields@fieldses.org; ffilzlnx@mindspring.com;
linux-nfs@vger.kernel.org;
> Frank Filz <ffilz@redhat.com>
> Subject: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock
request
> 
> This test currently fails against Linux nfsd, but I think it's the test
that's wrong. It
> basically does:
> 
> open for read
> read lock
> unlock
> open upgrade
> write lock
> 
> The write lock above is sent with a lock_seqid of 0, which is wrong.
> RFC7530/16.10.5 says:
> 
>    o  In the case in which the state has been created and the [new
>       lockowner] boolean is true, the server rejects the request with the
>       error NFS4ERR_BAD_SEQID.  The only exception is where there is a
>       retransmission of a previous request in which the boolean was
>       true.  In this case, the lock_seqid will match the original
>       request, and the response will reflect the final case, below.
> 
> Since the above is not a retransmission, knfsd is correct to reject this
call. This
> patch fixes the open_sequence object to track the lock seqid and set it
correctly
> in the LOCK request.
> 
> With this, LOCK24 passes against knfsd.
> 
> Cc: Frank Filz <ffilz@redhat.com>
> Fixes: 4299316fb357 (Add LOCK24 test case to test open uprgade/downgrade
> scenario)
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  nfs4.0/servertests/st_lock.py | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/nfs4.0/servertests/st_lock.py b/nfs4.0/servertests/st_lock.py
index
> 468672403ffe..9d650ab017b9 100644
> --- a/nfs4.0/servertests/st_lock.py
> +++ b/nfs4.0/servertests/st_lock.py
> @@ -886,6 +886,7 @@ class open_sequence:
>          self.client = client
>          self.owner = owner
>          self.lockowner = lockowner
> +        self.lockseqid = 0
>      def open(self, access):
>          self.fh, self.stateid = self.client.create_confirm(self.owner,
>  						access=access,
> @@ -900,14 +901,17 @@ class open_sequence:
>          self.client.close_file(self.owner, self.fh, self.stateid)
>      def lock(self, type):
>          res = self.client.lock_file(self.owner, self.fh, self.stateid,
> -                    type=type, lockowner=self.lockowner)
> +                                    type=type, lockowner=self.lockowner,
> +                                    lockseqid=self.lockseqid)
>          check(res)
>          if res.status == NFS4_OK:
>              self.lockstateid = res.lockid
> +            self.lockseqid += 1
>      def unlock(self):
>          res = self.client.unlock_file(1, self.fh, self.lockstateid)
>          if res.status == NFS4_OK:
>              self.lockstateid = res.lockid
> +            self.lockseqid += 1
> 
>  def testOpenUpgradeLock(t, env):
>      """Try open, lock, open, downgrade, close
> --
> 2.39.2

Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Jeff Layton]

Thanks for testing it, Frank.

FWIW, if the unmodified test still passes on ganesha then that's
probably an indicator that it's not doing adequate vetting of the lock
seqid with v4.0.

Cheers,
Jeff

On Mon, 2023-03-13 at 11:51 -0700, Frank Filz wrote:
> Looks good to me, tested against Ganesha and the updated patch passes.
> 
> Frank
> 
> > -----Original Message-----
> > From: Jeff Layton [mailto:jlayton@kernel.org]
> > Sent: Monday, March 13, 2023 4:24 AM
> > To: calum.mackay@oracle.com
> > Cc: bfields@fieldses.org; ffilzlnx@mindspring.com;
> linux-nfs@vger.kernel.org;
> > Frank Filz <ffilz@redhat.com>
> > Subject: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock
> request
> > 
> > This test currently fails against Linux nfsd, but I think it's the test
> that's wrong. It
> > basically does:
> > 
> > open for read
> > read lock
> > unlock
> > open upgrade
> > write lock
> > 
> > The write lock above is sent with a lock_seqid of 0, which is wrong.
> > RFC7530/16.10.5 says:
> > 
> >    o  In the case in which the state has been created and the [new
> >       lockowner] boolean is true, the server rejects the request with the
> >       error NFS4ERR_BAD_SEQID.  The only exception is where there is a
> >       retransmission of a previous request in which the boolean was
> >       true.  In this case, the lock_seqid will match the original
> >       request, and the response will reflect the final case, below.
> > 
> > Since the above is not a retransmission, knfsd is correct to reject this
> call. This
> > patch fixes the open_sequence object to track the lock seqid and set it
> correctly
> > in the LOCK request.
> > 
> > With this, LOCK24 passes against knfsd.
> > 
> > Cc: Frank Filz <ffilz@redhat.com>
> > Fixes: 4299316fb357 (Add LOCK24 test case to test open uprgade/downgrade
> > scenario)
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  nfs4.0/servertests/st_lock.py | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/nfs4.0/servertests/st_lock.py b/nfs4.0/servertests/st_lock.py
> index
> > 468672403ffe..9d650ab017b9 100644
> > --- a/nfs4.0/servertests/st_lock.py
> > +++ b/nfs4.0/servertests/st_lock.py
> > @@ -886,6 +886,7 @@ class open_sequence:
> >          self.client = client
> >          self.owner = owner
> >          self.lockowner = lockowner
> > +        self.lockseqid = 0
> >      def open(self, access):
> >          self.fh, self.stateid = self.client.create_confirm(self.owner,
> >  						access=access,
> > @@ -900,14 +901,17 @@ class open_sequence:
> >          self.client.close_file(self.owner, self.fh, self.stateid)
> >      def lock(self, type):
> >          res = self.client.lock_file(self.owner, self.fh, self.stateid,
> > -                    type=type, lockowner=self.lockowner)
> > +                                    type=type, lockowner=self.lockowner,
> > +                                    lockseqid=self.lockseqid)
> >          check(res)
> >          if res.status == NFS4_OK:
> >              self.lockstateid = res.lockid
> > +            self.lockseqid += 1
> >      def unlock(self):
> >          res = self.client.unlock_file(1, self.fh, self.lockstateid)
> >          if res.status == NFS4_OK:
> >              self.lockstateid = res.lockid
> > +            self.lockseqid += 1
> > 
> >  def testOpenUpgradeLock(t, env):
> >      """Try open, lock, open, downgrade, close
> > --
> > 2.39.2
> 

-- 
Jeff Layton <jlayton@kernel.org>

Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Petr Vorel]

Hi Jeff,

thanks for fixing a long standing issue.

Tested-by: Petr Vorel <pvorel@suse.cz>
(on openSUSE and SLES)

Kind regards,
Petr

Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Calum Mackay]

[-- Attachment #1.1: Type: text/plain, Size: 3422 bytes --]

Now that I have some repo space (thank you Trond), I am putting things 
together…

On 13/03/2023 6:51 pm, Frank Filz wrote:
> Looks good to me, tested against Ganesha and the updated patch passes.

Frank, may I add your Tested-by:, for 5/5 please?

cheers,
calum.


> 
> Frank
> 
>> -----Original Message-----
>> From: Jeff Layton [mailto:jlayton@kernel.org]
>> Sent: Monday, March 13, 2023 4:24 AM
>> To: calum.mackay@oracle.com
>> Cc: bfields@fieldses.org; ffilzlnx@mindspring.com;
> linux-nfs@vger.kernel.org;
>> Frank Filz <ffilz@redhat.com>
>> Subject: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock
> request
>>
>> This test currently fails against Linux nfsd, but I think it's the test
> that's wrong. It
>> basically does:
>>
>> open for read
>> read lock
>> unlock
>> open upgrade
>> write lock
>>
>> The write lock above is sent with a lock_seqid of 0, which is wrong.
>> RFC7530/16.10.5 says:
>>
>>     o  In the case in which the state has been created and the [new
>>        lockowner] boolean is true, the server rejects the request with the
>>        error NFS4ERR_BAD_SEQID.  The only exception is where there is a
>>        retransmission of a previous request in which the boolean was
>>        true.  In this case, the lock_seqid will match the original
>>        request, and the response will reflect the final case, below.
>>
>> Since the above is not a retransmission, knfsd is correct to reject this
> call. This
>> patch fixes the open_sequence object to track the lock seqid and set it
> correctly
>> in the LOCK request.
>>
>> With this, LOCK24 passes against knfsd.
>>
>> Cc: Frank Filz <ffilz@redhat.com>
>> Fixes: 4299316fb357 (Add LOCK24 test case to test open uprgade/downgrade
>> scenario)
>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>> ---
>>   nfs4.0/servertests/st_lock.py | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/nfs4.0/servertests/st_lock.py b/nfs4.0/servertests/st_lock.py
> index
>> 468672403ffe..9d650ab017b9 100644
>> --- a/nfs4.0/servertests/st_lock.py
>> +++ b/nfs4.0/servertests/st_lock.py
>> @@ -886,6 +886,7 @@ class open_sequence:
>>           self.client = client
>>           self.owner = owner
>>           self.lockowner = lockowner
>> +        self.lockseqid = 0
>>       def open(self, access):
>>           self.fh, self.stateid = self.client.create_confirm(self.owner,
>>   						access=access,
>> @@ -900,14 +901,17 @@ class open_sequence:
>>           self.client.close_file(self.owner, self.fh, self.stateid)
>>       def lock(self, type):
>>           res = self.client.lock_file(self.owner, self.fh, self.stateid,
>> -                    type=type, lockowner=self.lockowner)
>> +                                    type=type, lockowner=self.lockowner,
>> +                                    lockseqid=self.lockseqid)
>>           check(res)
>>           if res.status == NFS4_OK:
>>               self.lockstateid = res.lockid
>> +            self.lockseqid += 1
>>       def unlock(self):
>>           res = self.client.unlock_file(1, self.fh, self.lockstateid)
>>           if res.status == NFS4_OK:
>>               self.lockstateid = res.lockid
>> +            self.lockseqid += 1
>>
>>   def testOpenUpgradeLock(t, env):
>>       """Try open, lock, open, downgrade, close
>> --
>> 2.39.2
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

RE: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Frank Filz]

> -----Original Message-----
> From: Calum Mackay [mailto:calum.mackay@oracle.com]
> Sent: Thursday, April 13, 2023 11:35 AM
> To: Frank Filz <ffilzlnx@mindspring.com>; 'Jeff Layton' <jlayton@kernel.org>
> Cc: Calum Mackay <calum.mackay@oracle.com>; bfields@fieldses.org; linux-
> nfs@vger.kernel.org; 'Frank Filz' <ffilz@redhat.com>
> Subject: Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock
> request
> 
> Now that I have some repo space (thank you Trond), I am putting things
> together…
> 
> On 13/03/2023 6:51 pm, Frank Filz wrote:
> > Looks good to me, tested against Ganesha and the updated patch passes.
> 
> Frank, may I add your Tested-by:, for 5/5 please?

Yes, definitely.

Frank

Tested-by: Frank Filz <ffilzlnx@mindspring.com>
> 
> cheers,
> calum.
> 
> 
> >
> > Frank
> >
> >> -----Original Message-----
> >> From: Jeff Layton [mailto:jlayton@kernel.org]
> >> Sent: Monday, March 13, 2023 4:24 AM
> >> To: calum.mackay@oracle.com
> >> Cc: bfields@fieldses.org; ffilzlnx@mindspring.com;
> > linux-nfs@vger.kernel.org;
> >> Frank Filz <ffilz@redhat.com>
> >> Subject: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second
> >> lock
> > request
> >>
> >> This test currently fails against Linux nfsd, but I think it's the
> >> test
> > that's wrong. It
> >> basically does:
> >>
> >> open for read
> >> read lock
> >> unlock
> >> open upgrade
> >> write lock
> >>
> >> The write lock above is sent with a lock_seqid of 0, which is wrong.
> >> RFC7530/16.10.5 says:
> >>
> >>     o  In the case in which the state has been created and the [new
> >>        lockowner] boolean is true, the server rejects the request with the
> >>        error NFS4ERR_BAD_SEQID.  The only exception is where there is a
> >>        retransmission of a previous request in which the boolean was
> >>        true.  In this case, the lock_seqid will match the original
> >>        request, and the response will reflect the final case, below.
> >>
> >> Since the above is not a retransmission, knfsd is correct to reject
> >> this
> > call. This
> >> patch fixes the open_sequence object to track the lock seqid and set
> >> it
> > correctly
> >> in the LOCK request.
> >>
> >> With this, LOCK24 passes against knfsd.
> >>
> >> Cc: Frank Filz <ffilz@redhat.com>
> >> Fixes: 4299316fb357 (Add LOCK24 test case to test open
> >> uprgade/downgrade
> >> scenario)
> >> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> >> ---
> >>   nfs4.0/servertests/st_lock.py | 6 +++++-
> >>   1 file changed, 5 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/nfs4.0/servertests/st_lock.py
> >> b/nfs4.0/servertests/st_lock.py
> > index
> >> 468672403ffe..9d650ab017b9 100644
> >> --- a/nfs4.0/servertests/st_lock.py
> >> +++ b/nfs4.0/servertests/st_lock.py
> >> @@ -886,6 +886,7 @@ class open_sequence:
> >>           self.client = client
> >>           self.owner = owner
> >>           self.lockowner = lockowner
> >> +        self.lockseqid = 0
> >>       def open(self, access):
> >>           self.fh, self.stateid = self.client.create_confirm(self.owner,
> >>   						access=access,
> >> @@ -900,14 +901,17 @@ class open_sequence:
> >>           self.client.close_file(self.owner, self.fh, self.stateid)
> >>       def lock(self, type):
> >>           res = self.client.lock_file(self.owner, self.fh, self.stateid,
> >> -                    type=type, lockowner=self.lockowner)
> >> +                                    type=type, lockowner=self.lockowner,
> >> +                                    lockseqid=self.lockseqid)
> >>           check(res)
> >>           if res.status == NFS4_OK:
> >>               self.lockstateid = res.lockid
> >> +            self.lockseqid += 1
> >>       def unlock(self):
> >>           res = self.client.unlock_file(1, self.fh, self.lockstateid)
> >>           if res.status == NFS4_OK:
> >>               self.lockstateid = res.lockid
> >> +            self.lockseqid += 1
> >>
> >>   def testOpenUpgradeLock(t, env):
> >>       """Try open, lock, open, downgrade, close
> >> --
> >> 2.39.2
> >

Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request

[Calum Mackay]

[-- Attachment #1.1: Type: text/plain, Size: 4311 bytes --]

On 14/04/2023 3:41 pm, Frank Filz wrote:
>> -----Original Message-----
>> From: Calum Mackay [mailto:calum.mackay@oracle.com]
>> Sent: Thursday, April 13, 2023 11:35 AM
>> To: Frank Filz <ffilzlnx@mindspring.com>; 'Jeff Layton' <jlayton@kernel.org>
>> Cc: Calum Mackay <calum.mackay@oracle.com>; bfields@fieldses.org; linux-
>> nfs@vger.kernel.org; 'Frank Filz' <ffilz@redhat.com>
>> Subject: Re: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock
>> request
>>
>> Now that I have some repo space (thank you Trond), I am putting things
>> together…
>>
>> On 13/03/2023 6:51 pm, Frank Filz wrote:
>>> Looks good to me, tested against Ganesha and the updated patch passes.
>>
>> Frank, may I add your Tested-by:, for 5/5 please?
> 
> Yes, definitely.
> 
> Frank
> 
> Tested-by: Frank Filz <ffilzlnx@mindspring.com>

thanks very much, Frank.

cheers,
calum.

>>
>> cheers,
>> calum.
>>
>>
>>>
>>> Frank
>>>
>>>> -----Original Message-----
>>>> From: Jeff Layton [mailto:jlayton@kernel.org]
>>>> Sent: Monday, March 13, 2023 4:24 AM
>>>> To: calum.mackay@oracle.com
>>>> Cc: bfields@fieldses.org; ffilzlnx@mindspring.com;
>>> linux-nfs@vger.kernel.org;
>>>> Frank Filz <ffilz@redhat.com>
>>>> Subject: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second
>>>> lock
>>> request
>>>>
>>>> This test currently fails against Linux nfsd, but I think it's the
>>>> test
>>> that's wrong. It
>>>> basically does:
>>>>
>>>> open for read
>>>> read lock
>>>> unlock
>>>> open upgrade
>>>> write lock
>>>>
>>>> The write lock above is sent with a lock_seqid of 0, which is wrong.
>>>> RFC7530/16.10.5 says:
>>>>
>>>>      o  In the case in which the state has been created and the [new
>>>>         lockowner] boolean is true, the server rejects the request with the
>>>>         error NFS4ERR_BAD_SEQID.  The only exception is where there is a
>>>>         retransmission of a previous request in which the boolean was
>>>>         true.  In this case, the lock_seqid will match the original
>>>>         request, and the response will reflect the final case, below.
>>>>
>>>> Since the above is not a retransmission, knfsd is correct to reject
>>>> this
>>> call. This
>>>> patch fixes the open_sequence object to track the lock seqid and set
>>>> it
>>> correctly
>>>> in the LOCK request.
>>>>
>>>> With this, LOCK24 passes against knfsd.
>>>>
>>>> Cc: Frank Filz <ffilz@redhat.com>
>>>> Fixes: 4299316fb357 (Add LOCK24 test case to test open
>>>> uprgade/downgrade
>>>> scenario)
>>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>>> ---
>>>>    nfs4.0/servertests/st_lock.py | 6 +++++-
>>>>    1 file changed, 5 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/nfs4.0/servertests/st_lock.py
>>>> b/nfs4.0/servertests/st_lock.py
>>> index
>>>> 468672403ffe..9d650ab017b9 100644
>>>> --- a/nfs4.0/servertests/st_lock.py
>>>> +++ b/nfs4.0/servertests/st_lock.py
>>>> @@ -886,6 +886,7 @@ class open_sequence:
>>>>            self.client = client
>>>>            self.owner = owner
>>>>            self.lockowner = lockowner
>>>> +        self.lockseqid = 0
>>>>        def open(self, access):
>>>>            self.fh, self.stateid = self.client.create_confirm(self.owner,
>>>>    						access=access,
>>>> @@ -900,14 +901,17 @@ class open_sequence:
>>>>            self.client.close_file(self.owner, self.fh, self.stateid)
>>>>        def lock(self, type):
>>>>            res = self.client.lock_file(self.owner, self.fh, self.stateid,
>>>> -                    type=type, lockowner=self.lockowner)
>>>> +                                    type=type, lockowner=self.lockowner,
>>>> +                                    lockseqid=self.lockseqid)
>>>>            check(res)
>>>>            if res.status == NFS4_OK:
>>>>                self.lockstateid = res.lockid
>>>> +            self.lockseqid += 1
>>>>        def unlock(self):
>>>>            res = self.client.unlock_file(1, self.fh, self.lockstateid)
>>>>            if res.status == NFS4_OK:
>>>>                self.lockstateid = res.lockid
>>>> +            self.lockseqid += 1
>>>>
>>>>    def testOpenUpgradeLock(t, env):
>>>>        """Try open, lock, open, downgrade, close
>>>> --
>>>> 2.39.2
>>>
> 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]