Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 15 Jun 2021 10:47:24 -0400
To:     Calum Mackay <calum.mackay@oracle.com>
Cc:     "suy.fnst@fujitsu.com" <suy.fnst@fujitsu.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "bfields@redhat.com" <bfields@redhat.com>
Subject: Re: [PATCH] pynfs: courtesy: send RECLAIM_COMPLETE before session2
 opening the file
Message-ID: <20210615144724.GB11877@fieldses.org>
References: <TY2PR01MB2124D8FDDDCA29F5691F3DD089359@TY2PR01MB2124.jpnprd01.prod.outlook.com>
 <91f1d7df-b63c-4aa3-cc03-a8e1cbb2ecb1@oracle.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <91f1d7df-b63c-4aa3-cc03-a8e1cbb2ecb1@oracle.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
From:   bfields@fieldses.org (J. Bruce Fields)
Precedence: bulk

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.