[PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[suy.fnst]

The test fails on v5.13-rc5 and old kernels. Because the second session
doesn't send RECLAIM_COMPLETE before attempting to do a non-reclaim
open. So the server returns NFS4ERR_GRACE instead of NFS4_OK.

    # ./testserver.py ${server_IP}:/nfsroot --rundeps COUR2
    INFO   :rpc.poll:got connection from ('127.0.0.1', 39206), assigned to
    fd=5
    INFO   :rpc.thread:Called connect(('193.168.140.239', 2049))
    INFO   :rpc.poll:Adding 6 generated by another thread
    INFO   :test.env:Created client to 193.168.140.239, 2049
    INFO   :test.env:Called do_readdir()
    INFO   :test.env:do_readdir() = [entry4(cookie=512,
    name=b'COUR2_1623055313', attrs={})]
    fileb'COUR2_1623119443'created by sess1
    INFO   :test.env:Sleeping for 22 seconds: twice the lease period
    INFO   :test.env:Woke up
    session created
    **************************************************
    COUR2    st_courtesy.testLockSleepLock                            :
    FAILURE
           OP_OPEN should return NFS4_OK, instead got
                     NFS4ERR_GRACE
    **************************************************
    Command line asked for 1 of 255 tests
      Of those: 0 Skipped, 1 Failed, 0 Warned, 0 Passed

RFC5661, page 567:
"Whenever a client establishes a new client ID and before it does the
first non-reclaim operation that obtains a lock, it MUST send a
RECLAIM_COMPLETE with rca_one_fs set to FALSE, even if there are no
locks to reclaim. If non-reclaim locking operations are done before
the RECLAIM_COMPLETE, an NFS4ERR_GRACE error will be returned."

Send RECLAIM_COMPLETE before the file open to let the test pass.
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 nfs4.1/server41tests/st_courtesy.py | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
index dd911a37772d..3478a9d93dbf 100644
--- a/nfs4.1/server41tests/st_courtesy.py
+++ b/nfs4.1/server41tests/st_courtesy.py
@@ -74,6 +74,9 @@ def testLockSleepLock(t, env):
     c2 = env.c1.new_client(b"%s_2" % env.testname(t))
     sess2 = c2.create_session()
 
+    res = sess2.compound([op.reclaim_complete(FALSE)])
+    check(res)
+
     res = open_file(sess2, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
     check(res)
 
-- 
2.30.1

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[Calum Mackay]

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

On 10/06/2021 2:01 am, suy.fnst@fujitsu.com wrote:
> The test fails on v5.13-rc5 and old kernels. Because the second session
> doesn't send RECLAIM_COMPLETE before attempting to do a non-reclaim
> open. So the server returns NFS4ERR_GRACE instead of NFS4_OK.

Thanks.

I suppose the problem here is that we're trying to pretend this is two 
separate clients, but the server sees it as a single client getting a 
new client id?

thanks,
calum.

> 
>      # ./testserver.py ${server_IP}:/nfsroot --rundeps COUR2
>      INFO   :rpc.poll:got connection from ('127.0.0.1', 39206), assigned to
>      fd=5
>      INFO   :rpc.thread:Called connect(('193.168.140.239', 2049))
>      INFO   :rpc.poll:Adding 6 generated by another thread
>      INFO   :test.env:Created client to 193.168.140.239, 2049
>      INFO   :test.env:Called do_readdir()
>      INFO   :test.env:do_readdir() = [entry4(cookie=512,
>      name=b'COUR2_1623055313', attrs={})]
>      fileb'COUR2_1623119443'created by sess1
>      INFO   :test.env:Sleeping for 22 seconds: twice the lease period
>      INFO   :test.env:Woke up
>      session created
>      **************************************************
>      COUR2    st_courtesy.testLockSleepLock                            :
>      FAILURE
>             OP_OPEN should return NFS4_OK, instead got
>                       NFS4ERR_GRACE
>      **************************************************
>      Command line asked for 1 of 255 tests
>        Of those: 0 Skipped, 1 Failed, 0 Warned, 0 Passed
> 
> RFC5661, page 567:
> "Whenever a client establishes a new client ID and before it does the
> first non-reclaim operation that obtains a lock, it MUST send a
> RECLAIM_COMPLETE with rca_one_fs set to FALSE, even if there are no
> locks to reclaim. If non-reclaim locking operations are done before
> the RECLAIM_COMPLETE, an NFS4ERR_GRACE error will be returned."
> 
> Send RECLAIM_COMPLETE before the file open to let the test pass.
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   nfs4.1/server41tests/st_courtesy.py | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
> index dd911a37772d..3478a9d93dbf 100644
> --- a/nfs4.1/server41tests/st_courtesy.py
> +++ b/nfs4.1/server41tests/st_courtesy.py
> @@ -74,6 +74,9 @@ def testLockSleepLock(t, env):
>       c2 = env.c1.new_client(b"%s_2" % env.testname(t))
>       sess2 = c2.create_session()
>   
> +    res = sess2.compound([op.reclaim_complete(FALSE)])
> +    check(res)
> +
>       res = open_file(sess2, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
>       check(res)
>   
> 

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

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[Calum Mackay]

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

On 10/06/2021 2:01 am, suy.fnst@fujitsu.com wrote:
> The test fails on v5.13-rc5 and old kernels. Because the second session
> doesn't send RECLAIM_COMPLETE before attempting to do a non-reclaim
> open. So the server returns NFS4ERR_GRACE instead of NFS4_OK.
> 
>      # ./testserver.py ${server_IP}:/nfsroot --rundeps COUR2
>      INFO   :rpc.poll:got connection from ('127.0.0.1', 39206), assigned to
>      fd=5
>      INFO   :rpc.thread:Called connect(('193.168.140.239', 2049))
>      INFO   :rpc.poll:Adding 6 generated by another thread
>      INFO   :test.env:Created client to 193.168.140.239, 2049
>      INFO   :test.env:Called do_readdir()
>      INFO   :test.env:do_readdir() = [entry4(cookie=512,
>      name=b'COUR2_1623055313', attrs={})]
>      fileb'COUR2_1623119443'created by sess1
>      INFO   :test.env:Sleeping for 22 seconds: twice the lease period
>      INFO   :test.env:Woke up
>      session created
>      **************************************************
>      COUR2    st_courtesy.testLockSleepLock                            :
>      FAILURE
>             OP_OPEN should return NFS4_OK, instead got
>                       NFS4ERR_GRACE
>      **************************************************
>      Command line asked for 1 of 255 tests
>        Of those: 0 Skipped, 1 Failed, 0 Warned, 0 Passed
> 
> RFC5661, page 567:
> "Whenever a client establishes a new client ID and before it does the
> first non-reclaim operation that obtains a lock, it MUST send a
> RECLAIM_COMPLETE with rca_one_fs set to FALSE, even if there are no
> locks to reclaim. If non-reclaim locking operations are done before
> the RECLAIM_COMPLETE, an NFS4ERR_GRACE error will be returned."
> 
> Send RECLAIM_COMPLETE before the file open to let the test pass.
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>   nfs4.1/server41tests/st_courtesy.py | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
> index dd911a37772d..3478a9d93dbf 100644
> --- a/nfs4.1/server41tests/st_courtesy.py
> +++ b/nfs4.1/server41tests/st_courtesy.py
> @@ -74,6 +74,9 @@ def testLockSleepLock(t, env):
>       c2 = env.c1.new_client(b"%s_2" % env.testname(t))
>       sess2 = c2.create_session()
>   
> +    res = sess2.compound([op.reclaim_complete(FALSE)])
> +    check(res)
> +
>       res = open_file(sess2, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
>       check(res)
>   
> 

I'd still like to check whether this is the right place to fix this.

Initially, I was confused as to why the first client "c1" doesn't face 
the same issue. A network trace shows that a RECLAIM_COMPLETE is indeed 
sent for c1, despite not appearing explicitly in testLockSleepLock(). 
Whereas one isn't sent for c2, hence the problem.

This is probably because c1 is initialised with:

   61     sess1 = env.c1.new_client_session(env.testname(t))


and c2 with:

   74     c2 = env.c1.new_client(b"%s_2" % env.testname(t))
   75     sess2 = c2.create_session()


The c1 case results in a RECLAIM_COMPLETE, but the c2 case does not.

I'm not yet sure whether that ought to be done in 
new_client()/create_session(). If so, then there would be no need to add 
it explicitly here.


[I suspect this was missed in my testing, since the Solaris server I 
used may be less strict about requiring the RECLAIM_COMPLETE]


I'll look into this more and report asap.

thanks again,
calum.

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

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[J. Bruce Fields]

On Mon, Jun 14, 2021 at 09:50:34PM +0100, Calum Mackay wrote:
> On 10/06/2021 2:01 am, suy.fnst@fujitsu.com wrote:
> >The test fails on v5.13-rc5 and old kernels. Because the second session
> >doesn't send RECLAIM_COMPLETE before attempting to do a non-reclaim
> >open. So the server returns NFS4ERR_GRACE instead of NFS4_OK.
> >
> >     # ./testserver.py ${server_IP}:/nfsroot --rundeps COUR2
> >     INFO   :rpc.poll:got connection from ('127.0.0.1', 39206), assigned to
> >     fd=5
> >     INFO   :rpc.thread:Called connect(('193.168.140.239', 2049))
> >     INFO   :rpc.poll:Adding 6 generated by another thread
> >     INFO   :test.env:Created client to 193.168.140.239, 2049
> >     INFO   :test.env:Called do_readdir()
> >     INFO   :test.env:do_readdir() = [entry4(cookie=512,
> >     name=b'COUR2_1623055313', attrs={})]
> >     fileb'COUR2_1623119443'created by sess1
> >     INFO   :test.env:Sleeping for 22 seconds: twice the lease period
> >     INFO   :test.env:Woke up
> >     session created
> >     **************************************************
> >     COUR2    st_courtesy.testLockSleepLock                            :
> >     FAILURE
> >            OP_OPEN should return NFS4_OK, instead got
> >                      NFS4ERR_GRACE
> >     **************************************************
> >     Command line asked for 1 of 255 tests
> >       Of those: 0 Skipped, 1 Failed, 0 Warned, 0 Passed
> >
> >RFC5661, page 567:
> >"Whenever a client establishes a new client ID and before it does the
> >first non-reclaim operation that obtains a lock, it MUST send a
> >RECLAIM_COMPLETE with rca_one_fs set to FALSE, even if there are no
> >locks to reclaim. If non-reclaim locking operations are done before
> >the RECLAIM_COMPLETE, an NFS4ERR_GRACE error will be returned."
> >
> >Send RECLAIM_COMPLETE before the file open to let the test pass.
> >Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> >---
> >  nfs4.1/server41tests/st_courtesy.py | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> >diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
> >index dd911a37772d..3478a9d93dbf 100644
> >--- a/nfs4.1/server41tests/st_courtesy.py
> >+++ b/nfs4.1/server41tests/st_courtesy.py
> >@@ -74,6 +74,9 @@ def testLockSleepLock(t, env):
> >      c2 = env.c1.new_client(b"%s_2" % env.testname(t))
> >      sess2 = c2.create_session()
> >+    res = sess2.compound([op.reclaim_complete(FALSE)])
> >+    check(res)
> >+
> >      res = open_file(sess2, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
> >      check(res)
> >
> 
> I'd still like to check whether this is the right place to fix this.
> 
> Initially, I was confused as to why the first client "c1" doesn't
> face the same issue. A network trace shows that a RECLAIM_COMPLETE
> is indeed sent for c1, despite not appearing explicitly in
> testLockSleepLock(). Whereas one isn't sent for c2, hence the
> problem.
> 
> This is probably because c1 is initialised with:
> 
>   61     sess1 = env.c1.new_client_session(env.testname(t))
> 
> 
> and c2 with:
> 
>   74     c2 = env.c1.new_client(b"%s_2" % env.testname(t))
>   75     sess2 = c2.create_session()
> 
> 
> The c1 case results in a RECLAIM_COMPLETE, but the c2 case does not.
> 
> I'm not yet sure whether that ought to be done in
> new_client()/create_session(). If so, then there would be no need to
> add it explicitly here.

There's definitely cases where clients want to be able to create a new
session without sending a new RECLAIM_COMPLETE.

Any reason we can't replace those two lines by a single
new_client_session()?  I'd do either that or just add the explicit
RECLAIM_COMPLETE.

> [I suspect this was missed in my testing, since the Solaris server I
> used may be less strict about requiring the RECLAIM_COMPLETE]

That's a server bug:

	https://datatracker.ietf.org/doc/html/rfc5661#page-173

	... NFS4ERR_GRACE must always be returned to clients attempting
	a non-reclaim lock request before doing their own global
	RECLAIM_COMPLETE.

I've complained about it before.  I had some idea it'd been fixed, maybe
not.

--b.

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[Calum Mackay]

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

On 15/06/2021 3:47 pm, J. Bruce Fields wrote:
> On Mon, Jun 14, 2021 at 09:50:34PM +0100, Calum Mackay wrote:
>> On 10/06/2021 2:01 am, suy.fnst@fujitsu.com wrote:
>>> The test fails on v5.13-rc5 and old kernels. Because the second session
>>> doesn't send RECLAIM_COMPLETE before attempting to do a non-reclaim
>>> open. So the server returns NFS4ERR_GRACE instead of NFS4_OK.
>>>
>>>      # ./testserver.py ${server_IP}:/nfsroot --rundeps COUR2
>>>      INFO   :rpc.poll:got connection from ('127.0.0.1', 39206), assigned to
>>>      fd=5
>>>      INFO   :rpc.thread:Called connect(('193.168.140.239', 2049))
>>>      INFO   :rpc.poll:Adding 6 generated by another thread
>>>      INFO   :test.env:Created client to 193.168.140.239, 2049
>>>      INFO   :test.env:Called do_readdir()
>>>      INFO   :test.env:do_readdir() = [entry4(cookie=512,
>>>      name=b'COUR2_1623055313', attrs={})]
>>>      fileb'COUR2_1623119443'created by sess1
>>>      INFO   :test.env:Sleeping for 22 seconds: twice the lease period
>>>      INFO   :test.env:Woke up
>>>      session created
>>>      **************************************************
>>>      COUR2    st_courtesy.testLockSleepLock                            :
>>>      FAILURE
>>>             OP_OPEN should return NFS4_OK, instead got
>>>                       NFS4ERR_GRACE
>>>      **************************************************
>>>      Command line asked for 1 of 255 tests
>>>        Of those: 0 Skipped, 1 Failed, 0 Warned, 0 Passed
>>>
>>> RFC5661, page 567:
>>> "Whenever a client establishes a new client ID and before it does the
>>> first non-reclaim operation that obtains a lock, it MUST send a
>>> RECLAIM_COMPLETE with rca_one_fs set to FALSE, even if there are no
>>> locks to reclaim. If non-reclaim locking operations are done before
>>> the RECLAIM_COMPLETE, an NFS4ERR_GRACE error will be returned."
>>>
>>> Send RECLAIM_COMPLETE before the file open to let the test pass.
>>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>>> ---
>>>   nfs4.1/server41tests/st_courtesy.py | 3 +++
>>>   1 file changed, 3 insertions(+)
>>>
>>> diff --git a/nfs4.1/server41tests/st_courtesy.py b/nfs4.1/server41tests/st_courtesy.py
>>> index dd911a37772d..3478a9d93dbf 100644
>>> --- a/nfs4.1/server41tests/st_courtesy.py
>>> +++ b/nfs4.1/server41tests/st_courtesy.py
>>> @@ -74,6 +74,9 @@ def testLockSleepLock(t, env):
>>>       c2 = env.c1.new_client(b"%s_2" % env.testname(t))
>>>       sess2 = c2.create_session()
>>> +    res = sess2.compound([op.reclaim_complete(FALSE)])
>>> +    check(res)
>>> +
>>>       res = open_file(sess2, env.testname(t), access=OPEN4_SHARE_ACCESS_WRITE)
>>>       check(res)
>>>
>>
>> I'd still like to check whether this is the right place to fix this.
>>
>> Initially, I was confused as to why the first client "c1" doesn't
>> face the same issue. A network trace shows that a RECLAIM_COMPLETE
>> is indeed sent for c1, despite not appearing explicitly in
>> testLockSleepLock(). Whereas one isn't sent for c2, hence the
>> problem.
>>
>> This is probably because c1 is initialised with:
>>
>>    61     sess1 = env.c1.new_client_session(env.testname(t))
>>
>>
>> and c2 with:
>>
>>    74     c2 = env.c1.new_client(b"%s_2" % env.testname(t))
>>    75     sess2 = c2.create_session()
>>
>>
>> The c1 case results in a RECLAIM_COMPLETE, but the c2 case does not.
>>
>> I'm not yet sure whether that ought to be done in
>> new_client()/create_session(). If so, then there would be no need to
>> add it explicitly here.
> 
> There's definitely cases where clients want to be able to create a new
> session without sending a new RECLAIM_COMPLETE.

thanks Bruce; yes, I wondered.


> Any reason we can't replace those two lines by a single
> new_client_session()?

I wasn't quite sure on the semantics of those calls.

We want what appears to the server to be a new client c2, not a new 
session from an existing client c1. I wasn't sure whether 
new_client_session() would give us that?

Put alternatively, what is the difference between new_client_session and 
new_client/new_session? Is it just a different granularity, and to give 
the option e.g. of not using RECLAIM_COMPLETE?


I'll have a further look…

thanks very much,
calum.



> I'd do either that or just add the explicit
> RECLAIM_COMPLETE.
> 
>> [I suspect this was missed in my testing, since the Solaris server I
>> used may be less strict about requiring the RECLAIM_COMPLETE]
> 
> That's a server bug:
> 
> 	https://datatracker.ietf.org/doc/html/rfc5661#page-173
> 
> 	... NFS4ERR_GRACE must always be returned to clients attempting
> 	a non-reclaim lock request before doing their own global
> 	RECLAIM_COMPLETE.
> 
> I've complained about it before.  I had some idea it'd been fixed, maybe
> not.
> 
> --b.
> 

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

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[J. Bruce Fields]

On Tue, Jun 15, 2021 at 04:38:15PM +0100, Calum Mackay wrote:
> I wasn't quite sure on the semantics of those calls.
> 
> We want what appears to the server to be a new client c2, not a new
> session from an existing client c1. I wasn't sure whether
> new_client_session() would give us that?

Yes, it gets you both a new client and a new session for that client.
It does all the stuff you need to get a new client that you can actually
use for normal operations, so it should be the default unless you need
finer control.

(Also, *eventually*, I want to port all the 4.0 tests to the 4.1 code
and eliminate the separate 4.0/4.1 directories.  new_client_session will
then do either exchange_id+create_session+reclaim_complete or
setclientid+setclient_confirm depending on minor version.)

Anyway, so the names are totally unhelpful.  Maybe we should reanme
new_client to exchange_id and new_client_session to just new_client.

--b.

Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2 opening the file

[Calum Mackay]

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

On 15/06/2021 4:50 pm, J. Bruce Fields wrote:
> On Tue, Jun 15, 2021 at 04:38:15PM +0100, Calum Mackay wrote:
>> I wasn't quite sure on the semantics of those calls.
>>
>> We want what appears to the server to be a new client c2, not a new
>> session from an existing client c1. I wasn't sure whether
>> new_client_session() would give us that?
> 
> Yes, it gets you both a new client and a new session for that client.
> It does all the stuff you need to get a new client that you can actually
> use for normal operations, so it should be the default unless you need
> finer control.

Perfect, thanks Bruce.

I'll test that, and submit a new patch for this issue


> (Also, *eventually*, I want to port all the 4.0 tests to the 4.1 code
> and eliminate the separate 4.0/4.1 directories.  new_client_session will
> then do either exchange_id+create_session+reclaim_complete or
> setclientid+setclient_confirm depending on minor version.)
> 
> Anyway, so the names are totally unhelpful.  Maybe we should reanme
> new_client to exchange_id and new_client_session to just new_client.

sounds good.


cheers,
calum.

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