[RFC PATCH 0/2] nfsd: clean up CB_LAYOUTRECALL handling

[RFC PATCH 0/2] nfsd: clean up CB_LAYOUTRECALL handling

[Jeff Layton]

This is a proposal that should follow the broad outlines of what HCH
was suggesting for CB_LAYOUTRECALL. It changes the code to treat 0
and NFS4ERR_DELAY returns equivalently, and also implements a scheme
to give up on the client after two lease periods.

Note that this is _not_ tested, since I don't have a pnfs test rig on
which to test this.

Jeff Layton (2):
  nfsd: don't hold ls_mutex across a layout recall
  nfsd: give up on CB_LAYOUTRECALLs after two lease periods

 fs/nfsd/nfs4layouts.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

-- 
2.5.0

[RFC PATCH 1/2] nfsd: don't hold ls_mutex across a layout recall

[Jeff Layton]

We do need to serialize layout stateid morphing operations, but we
currently hold the ls_mutex across a layout recall which is pretty
ugly. It's also unnecessary -- once we've bumped the seqid and
copied it, we don't need to serialize the rest of the CB_LAYOUTRECALL
vs. anything else. Just drop the mutex once the copy is done.

Fixes: cc8a55320b5f "nfsd: serialize layout stateid morphing operations"
Cc: stable@vger.kernel.org
Reported-and-Tested-by: Kinglong Mee <kinglongmee@gmail.com>
Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfsd/nfs4layouts.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index 9ffef06b30d5..c9d6c715c0fb 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -616,6 +616,7 @@ nfsd4_cb_layout_prepare(struct nfsd4_callback *cb)
 
 	mutex_lock(&ls->ls_mutex);
 	nfs4_inc_and_copy_stateid(&ls->ls_recall_sid, &ls->ls_stid);
+	mutex_unlock(&ls->ls_mutex);
 }
 
 static int
@@ -659,7 +660,6 @@ nfsd4_cb_layout_release(struct nfsd4_callback *cb)
 
 	trace_layout_recall_release(&ls->ls_stid.sc_stateid);
 
-	mutex_unlock(&ls->ls_mutex);
 	nfsd4_return_all_layouts(ls, &reaplist);
 	nfsd4_free_layouts(&reaplist);
 	nfs4_put_stid(&ls->ls_stid);
-- 
2.5.0

Re: [RFC PATCH 1/2] nfsd: don't hold ls_mutex across a layout recall

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

[RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[Jeff Layton]

Have the CB_LAYOUTRECALL code treat NFS4_OK and NFS4ERR_DELAY returns
equivalently. Change the code to periodically resend CB_LAYOUTRECALLS
until the ls_layouts list is empty or the client returns a different
error code.

If we go for two lease periods without the list being emptied or the
client sending a hard error, then we give up and clean out the list
anyway.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfsd/nfs4layouts.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
index c9d6c715c0fb..f72abc3ba013 100644
--- a/fs/nfsd/nfs4layouts.c
+++ b/fs/nfsd/nfs4layouts.c
@@ -624,24 +624,39 @@ nfsd4_cb_layout_done(struct nfsd4_callback *cb, struct rpc_task *task)
 {
 	struct nfs4_layout_stateid *ls =
 		container_of(cb, struct nfs4_layout_stateid, ls_recall);
+	struct nfsd_net *nn;
+	ktime_t now, cutoff;
 	LIST_HEAD(reaplist);
 
+
 	switch (task->tk_status) {
 	case 0:
-		return 1;
+	case -NFS4ERR_DELAY:
+		/*
+		 * Anything left? If not, then call it done. Note that we don't
+		 * take the spinlock since this is an optimization and nothing
+		 * should get added until the cb counter goes to zero.
+		 */
+		if (list_empty(&ls->ls_layouts))
+			return 1;
+
+		/* Poll the client until it's done with the layout */
+		now = ktime_get();
+		nn = net_generic(ls->ls_stid.sc_client->net, nfsd_net_id);
+
+		/* Client gets 2 lease periods to return it */
+		cutoff = ktime_add_ns(task->tk_start,
+					 nn->nfsd4_lease * NSEC_PER_SEC * 2);
+
+		if (ktime_before(now, cutoff)) {
+			rpc_delay(task, HZ/100); /* 10 mili-seconds */
+			return 0;
+		}
+		/* Fallthrough */
 	case -NFS4ERR_NOMATCHING_LAYOUT:
 		trace_layout_recall_done(&ls->ls_stid.sc_stateid);
 		task->tk_status = 0;
 		return 1;
-	case -NFS4ERR_DELAY:
-		/* Poll the client until it's done with the layout */
-		/* FIXME: cap number of retries.
-		 * The pnfs standard states that we need to only expire
-		 * the client after at-least "lease time" .eg lease-time * 2
-		 * when failing to communicate a recall
-		 */
-		rpc_delay(task, HZ/100); /* 10 mili-seconds */
-		return 0;
 	default:
 		/*
 		 * Unknown error or non-responding client, we'll need to fence.
-- 
2.5.0

Re: [RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 07:23:48AM -0500, Jeff Layton wrote:
> Have the CB_LAYOUTRECALL code treat NFS4_OK and NFS4ERR_DELAY returns
> equivalently. Change the code to periodically resend CB_LAYOUTRECALLS
> until the ls_layouts list is empty or the client returns a different
> error code.
> 
> If we go for two lease periods without the list being emptied or the
> client sending a hard error, then we give up and clean out the list
> anyway.

This looks reasonable to me, but I'll need to actually test it
before giving an ACK.

Btw, it seems like the delegation and layoutrecall code would benefit
from some more code sharing for timeouts.  For example delegation
returns currently don't support NFS4ERR_DELAY at all.

Re: [RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[Jeff Layton]

On Tue, 8 Dec 2015 16:21:23 +0100
Christoph Hellwig <hch@lst.de> wrote:

> On Tue, Dec 08, 2015 at 07:23:48AM -0500, Jeff Layton wrote:
> > Have the CB_LAYOUTRECALL code treat NFS4_OK and NFS4ERR_DELAY returns
> > equivalently. Change the code to periodically resend CB_LAYOUTRECALLS
> > until the ls_layouts list is empty or the client returns a different
> > error code.
> > 
> > If we go for two lease periods without the list being emptied or the
> > client sending a hard error, then we give up and clean out the list
> > anyway.
> 
> This looks reasonable to me, but I'll need to actually test it
> before giving an ACK.
> 

Please do, I don't have a good way to test this at the moment...

> Btw, it seems like the delegation and layoutrecall code would benefit
> from some more code sharing for timeouts.  For example delegation
> returns currently don't support NFS4ERR_DELAY at all.

Yes...

I also wonder -- are we handling revoked layouts correctly? Shouldn't
we be handling revoked layouts like we would a revoked delegation? Stop
allowing the stateid to be used and morph it appropriately so that a
TEST_STATEID against it gives you an error?
 
Right now, it doesn't look like TEST_STATEID is set up to handle layout
stateids at all so they all get back NFS4ERR_BAD_STATEID.

-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[Christoph Hellwig]

On Tue, Dec 08, 2015 at 10:46:10AM -0500, Jeff Layton wrote:
> > This looks reasonable to me, but I'll need to actually test it
> > before giving an ACK.
> > 
> 
> Please do, I don't have a good way to test this at the moment...

Looks like the baseline got broken once again, so I'll need some time
to track that down first.

> > Btw, it seems like the delegation and layoutrecall code would benefit
> > from some more code sharing for timeouts.  For example delegation
> > returns currently don't support NFS4ERR_DELAY at all.
> 
> Yes...
> 
> I also wonder -- are we handling revoked layouts correctly? Shouldn't
> we be handling revoked layouts like we would a revoked delegation? Stop
> allowing the stateid to be used and morph it appropriately so that a
> TEST_STATEID against it gives you an error?

Probably, but I'd need to take a deeper look at this.

Re: [RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[Christoph Hellwig]

Ok, this tests fine for me:

Tested-by: Christoph Hellwig <hch@lst.de>

Re: [RFC PATCH 2/2] nfsd: give up on CB_LAYOUTRECALLs after two lease periods

[J. Bruce Fields]

On Tue, Dec 22, 2015 at 02:25:38PM +0100, Christoph Hellwig wrote:
> Ok, this tests fine for me:
> 
> Tested-by: Christoph Hellwig <hch@lst.de>

Thanks, applying for 4.5.

(Missed your reviewed-by on the other patch when I submitted that for
stable, apologies.)

I did a little work on some pynfs tests to exercise some of the failure
cases here, but didn't get very far; I'll post something if I get a
chance to test them.  I think those tests are what we'd need before
doing something more sophisticated.

--b.