From:   "Frank Filz" <ffilzlnx@mindspring.com>
To:     "'Jeff Layton'" <jlayton@kernel.org>, <calum.mackay@oracle.com>
Cc:     <bfields@fieldses.org>, <linux-nfs@vger.kernel.org>,
        "'Frank Filz'" <ffilz@redhat.com>
References: <20230313112401.20488-1-jlayton@kernel.org> <20230313112401.20488-6-jlayton@kernel.org>
In-Reply-To: <20230313112401.20488-6-jlayton@kernel.org>
Subject: RE: [pynfs PATCH v2 5/5] LOCK24: fix the lock_seqid in second lock request
Date:   Mon, 13 Mar 2023 11:51:45 -0700
Message-ID: <05c001d955dc$dc7e6fa0$957b4ee0$@mindspring.com>
MIME-Version: 1.0
Content-Type: text/plain;
        charset="us-ascii"
Content-Transfer-Encoding: 7bit
Content-Language: en-us
Thread-Index: AQIOChUlYc/UeF2748/fISU/E8FFiQF2dM0froPxqqA=
Precedence: bulk

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