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

[Calum Mackay <calum.mackay@xxxxxxxxxx>]

On 14/04/2023 3:41 pm, Frank Filz wrote:
> > -----Original Message-----
> > From: Calum Mackay [mailto:calum.mackay@xxxxxxxxxx]
> > Sent: Thursday, April 13, 2023 11:35 AM
> > To: Frank Filz <ffilzlnx@xxxxxxxxxxxxxx>; 'Jeff Layton' <jlayton@xxxxxxxxxx>
> > Cc: Calum Mackay <calum.mackay@xxxxxxxxxx>; bfields@xxxxxxxxxxxx; linux-
> > nfs@xxxxxxxxxxxxxxx; 'Frank Filz' <ffilz@xxxxxxxxxx>
> > 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@xxxxxxxxxxxxxx>

thanks very much, Frank.

cheers,
calum.

> > cheers,
> > calum.
> >
> >
> > > Frank
> > >
> > > > -----Original Message-----
> > > > From: Jeff Layton [mailto:jlayton@xxxxxxxxxx]
> > > > Sent: Monday, March 13, 2023 4:24 AM
> > > > To: calum.mackay@xxxxxxxxxx
> > > > Cc: bfields@xxxxxxxxxxxx; ffilzlnx@xxxxxxxxxxxxxx;
> > > linux-nfs@xxxxxxxxxxxxxxx;
> > > > Frank Filz <ffilz@xxxxxxxxxx>
> > > > 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@xxxxxxxxxx>
> > > > Fixes: 4299316fb357 (Add LOCK24 test case to test open
> > > > uprgade/downgrade
> > > > scenario)
> > > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > > ---
> > > >    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: OpenPGP_signature
Description: OpenPGP digital signature