| Message ID | 20230313112401.20488-6-jlayton@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | An assortment of pynfs patches | expand |
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
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 >
Hi Jeff,
thanks for fixing a long standing issue.
Tested-by: Petr Vorel <pvorel@suse.cz>
(on openSUSE and SLES)
Kind regards,
Petr
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 >
> -----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 > >
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 >>> > >
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
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(-)