Re: [PATCH v3 12/13] pnfs: rework LAYOUTGET retry handling

[Jeff Layton <jlayton@poochiereds.net>]

[-- Attachment #1: Type: text/plain, Size: 11320 bytes --]

On Tue, 2016-05-17 at 06:16 -0400, Jeff Layton wrote:
> On Tue, 2016-05-17 at 02:07 +0000, Trond Myklebust wrote:
> > 
> > 
> > On 5/14/16, 21:06, "Jeff Layton" <jlayton@poochiereds.net> wrote:
> > 
> > > There are several problems in the way a stateid is selected for a
> > > LAYOUTGET operation:
> > > 
> > > We pick a stateid to use in the RPC prepare op, but that makes
> > > it difficult to serialize LAYOUTGETs that use the open stateid. That
> > > serialization is done in pnfs_update_layout, which occurs well before
> > > the rpc_prepare operation.
> > > 
> > > Between those two events, the i_lock is dropped and reacquired.
> > > pnfs_update_layout can find that the list has lsegs in it and not do any
> > > serialization, but then later pnfs_choose_layoutget_stateid ends up
> > > choosing the open stateid.
> > > 
> > > This patch changes the client to select the stateid to use in the
> > > LAYOUTGET earlier, when we're searching for a usable layout segment.
> > > This way we can do it all while holding the i_lock the first time, and
> > > ensure that we serialize any LAYOUTGET call that uses a non-layout
> > > stateid.
> > > 
> > > This also means a rework of how LAYOUTGET replies are handled, as we
> > > must now get the latest stateid if we want to retransmit in response
> > > to a retryable error.
> > > 
> > > Most of those errors boil down to the fact that the layout state has
> > > changed in some fashion. Thus, what we really want to do is to re-search
> > > for a layout when it fails with a retryable error, so that we can avoid
> > > reissuing the RPC at all if possible.
> > > 
> > > While the LAYOUTGET RPC is async, the initiating thread always waits for
> > > it to complete, so it's effectively synchronous anyway. Currently, when
> > > we need to retry a LAYOUTGET because of an error, we drive that retry
> > > via the rpc state machine.
> > > 
> > > This means that once the call has been submitted, it runs until it
> > > completes. So, we must move the error handling for this RPC out of the
> > > rpc_call_done operation and into the caller.
> > > 
> > > In order to handle errors like NFS4ERR_DELAY properly, we must also
> > > pass a pointer to the sliding timeout, which is now moved to the stack
> > > in pnfs_update_layout.
> > > 
> > > The complicating errors are -NFS4ERR_RECALLCONFLICT and
> > > -NFS4ERR_LAYOUTTRYLATER, as those involve a timeout after which we give
> > > up and return NULL back to the caller. So, there is some special
> > > handling for those errors to ensure that the layers driving the retries
> > > can handle that appropriately.
> > > 
> > > Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
> > > ---
> > > fs/nfs/nfs4proc.c | 111 +++++++++++++++----------------------
> > > fs/nfs/nfs4trace.h | 10 +++-
> > > fs/nfs/pnfs.c | 142 +++++++++++++++++++++++++-----------------------
> > > fs/nfs/pnfs.h | 6 +-
> > > include/linux/nfs4.h | 2 +
> > > include/linux/nfs_xdr.h | 2 -
> > > 6 files changed, 131 insertions(+), 142 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> > > index c0d75be8cb69..1254ed84c760 100644
> > > --- a/fs/nfs/nfs4proc.c
> > > +++ b/fs/nfs/nfs4proc.c
> > > @@ -416,6 +416,7 @@ static int nfs4_do_handle_exception(struct nfs_server *server,
> > > case -NFS4ERR_DELAY:
> > > nfs_inc_server_stats(server, NFSIOS_DELAY);
> > > case -NFS4ERR_GRACE:
> > > +	 case -NFS4ERR_RECALLCONFLICT:
> > > exception->delay = 1;
> > > return 0;
> > >  
> > > @@ -7824,40 +7825,34 @@ nfs4_layoutget_prepare(struct rpc_task *task, void *calldata)
> > > struct nfs4_layoutget *lgp = calldata;
> > > struct nfs_server *server = NFS_SERVER(lgp->args.inode);
> > > struct nfs4_session *session = nfs4_get_session(server);
> > > -	int ret;
> > >  
> > > dprintk("--> %s\n", __func__);
> > > -	/* Note the is a race here, where a CB_LAYOUTRECALL can come in
> > > -	 * right now covering the LAYOUTGET we are about to send.
> > > -	 * However, that is not so catastrophic, and there seems
> > > -	 * to be no way to prevent it completely.
> > > -	 */
> > > -	if (nfs41_setup_sequence(session, &lgp->args.seq_args,
> > > -	 &lgp->res.seq_res, task))
> > > -	 return;
> > > -	ret = pnfs_choose_layoutget_stateid(&lgp->args.stateid,
> > > -	 NFS_I(lgp->args.inode)->layout,
> > > -	 &lgp->args.range,
> > > -	 lgp->args.ctx->state);
> > > -	if (ret < 0)
> > > -	 rpc_exit(task, ret);
> > > +	nfs41_setup_sequence(session, &lgp->args.seq_args,
> > > +	 &lgp->res.seq_res, task);
> > > +	dprintk("<-- %s\n", __func__);
> > > }
> > >  
> > > static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
> > > {
> > > struct nfs4_layoutget *lgp = calldata;
> > > +
> > > +	dprintk("--> %s\n", __func__);
> > > +	nfs41_sequence_done(task, &lgp->res.seq_res);
> > > +	dprintk("<-- %s\n", __func__);
> > > +}
> > > +
> > > +static int
> > > +nfs4_layoutget_handle_exception(struct rpc_task *task,
> > > +	 struct nfs4_layoutget *lgp, struct nfs4_exception *exception)
> > > +{
> > > struct inode *inode = lgp->args.inode;
> > > struct nfs_server *server = NFS_SERVER(inode);
> > > struct pnfs_layout_hdr *lo;
> > > -	struct nfs4_state *state = NULL;
> > > -	unsigned long timeo, now, giveup;
> > > +	int status = task->tk_status;
> > >  
> > > dprintk("--> %s tk_status => %d\n", __func__, -task->tk_status);
> > >  
> > > -	if (!nfs41_sequence_done(task, &lgp->res.seq_res))
> > > -	 goto out;
> > > -
> > > -	switch (task->tk_status) {
> > > +	switch (status) {
> > > case 0:
> > > goto out;
> > >  
> > > @@ -7867,57 +7862,39 @@ static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
> > > * retry go inband.
> > > */
> > > case -NFS4ERR_LAYOUTUNAVAILABLE:
> > > -	 task->tk_status = -ENODATA;
> > > +	 status = -ENODATA;
> > > goto out;
> > > /*
> > > * NFS4ERR_BADLAYOUT means the MDS cannot return a layout of
> > > * length lgp->args.minlength != 0 (see RFC5661 section 18.43.3).
> > > */
> > > case -NFS4ERR_BADLAYOUT:
> > > -	 goto out_overflow;
> > > +	 status = -EOVERFLOW;
> > > +	 goto out;
> > > /*
> > > * NFS4ERR_LAYOUTTRYLATER is a conflict with another client
> > > * (or clients) writing to the same RAID stripe except when
> > > * the minlength argument is 0 (see RFC5661 section 18.43.3).
> > > +	 *
> > > +	 * Treat it like we would RECALLCONFLICT -- we retry for a little
> > > +	 * while, and then eventually give up.
> > > */
> > > case -NFS4ERR_LAYOUTTRYLATER:
> > > -	 if (lgp->args.minlength == 0)
> > > -	 goto out_overflow;
> > > -	/*
> > > -	 * NFS4ERR_RECALLCONFLICT is when conflict with self (must recall
> > > -	 * existing layout before getting a new one).
> > > -	 */
> > > -	case -NFS4ERR_RECALLCONFLICT:
> > > -	 timeo = rpc_get_timeout(task->tk_client);
> > > -	 giveup = lgp->args.timestamp + timeo;
> > > -	 now = jiffies;
> > > -	 if (time_after(giveup, now)) {
> > > -	 unsigned long delay;
> > > -
> > > -	 /* Delay for:
> > > -	 * - Not less then NFS4_POLL_RETRY_MIN.
> > > -	 * - One last time a jiffie before we give up
> > > -	 * - exponential backoff (time_now minus start_attempt)
> > > -	 */
> > > -	 delay = max_t(unsigned long, NFS4_POLL_RETRY_MIN,
> > > -	 min((giveup - now - 1),
> > > -	 now - lgp->args.timestamp));
> > > -
> > > -	 dprintk("%s: NFS4ERR_RECALLCONFLICT waiting %lu\n",
> > > -	 __func__, delay);
> > > -	 rpc_delay(task, delay);
> > > -	 /* Do not call nfs4_async_handle_error() */
> > > -	 goto out_restart;
> > > +	 if (lgp->args.minlength == 0) {
> > > +	 status = -EOVERFLOW;
> > > +	 goto out;
> > > }
> > > +	 status = -NFS4ERR_RECALLCONFLICT;
> > > break;
> > > case -NFS4ERR_EXPIRED:
> > > case -NFS4ERR_BAD_STATEID:
> > > +	 exception->timeout = 0;
> > > spin_lock(&inode->i_lock);
> > > if (nfs4_stateid_match(&lgp->args.stateid,
> > > &lgp->args.ctx->state->stateid)) {
> > > spin_unlock(&inode->i_lock);
> > > /* If the open stateid was bad, then recover it. */
> > > -	 state = lgp->args.ctx->state;
> > > +	 exception->state = lgp->args.ctx->state;
> > > break;
> > > }
> > > lo = NFS_I(inode)->layout;
> > > @@ -7935,20 +7912,18 @@ static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
> > > pnfs_free_lseg_list(&head);
> > > } else
> > > spin_unlock(&inode->i_lock);
> > > -	 goto out_restart;
> > > +	 status = -EAGAIN;
> > > +	 goto out;
> > > }
> > > -	if (nfs4_async_handle_error(task, server, state, &lgp->timeout) == -EAGAIN)
> > > -	 goto out_restart;
> > > +
> > > +	status = nfs4_handle_exception(server, status, exception);
> > > +	if (status == 0)
> > > +	 status = task->tk_status;
> > > +	if (exception->retry && status != -NFS4ERR_RECALLCONFLICT)
> > > +	 status = -EAGAIN;
> > > out:
> > > dprintk("<-- %s\n", __func__);
> > > -	return;
> > > -out_restart:
> > > -	task->tk_status = 0;
> > > -	rpc_restart_call_prepare(task);
> > > -	return;
> > > -out_overflow:
> > > -	task->tk_status = -EOVERFLOW;
> > > -	goto out;
> > > +	return status;
> > > }
> > >  
> > > static size_t max_response_pages(struct nfs_server *server)
> > > @@ -8017,7 +7992,7 @@ static const struct rpc_call_ops nfs4_layoutget_call_ops = {
> > > };
> > >  
> > > struct pnfs_layout_segment *
> > > -nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
> > > +nfs4_proc_layoutget(struct nfs4_layoutget *lgp, long *timeout, gfp_t gfp_flags)
> > > {
> > > struct inode *inode = lgp->args.inode;
> > > struct nfs_server *server = NFS_SERVER(inode);
> > > @@ -8037,6 +8012,7 @@ nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
> > > .flags = RPC_TASK_ASYNC,
> > > };
> > > struct pnfs_layout_segment *lseg = NULL;
> > > +	struct nfs4_exception exception = { .timeout = *timeout };
> > > int status = 0;
> > >  
> > > dprintk("--> %s\n", __func__);
> > > @@ -8050,7 +8026,6 @@ nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
> > > return ERR_PTR(-ENOMEM);
> > > }
> > > lgp->args.layout.pglen = max_pages * PAGE_SIZE;
> > > -	lgp->args.timestamp = jiffies;
> > >  
> > > lgp->res.layoutp = &lgp->args.layout;
> > > lgp->res.seq_res.sr_slot = NULL;
> > > @@ -8060,13 +8035,17 @@ nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
> > > if (IS_ERR(task))
> > > return ERR_CAST(task);
> > > status = nfs4_wait_for_completion_rpc_task(task);
> > > -	if (status == 0)
> > > -	 status = task->tk_status;
> > > +	if (status == 0) {
> > > +	 status = nfs4_layoutget_handle_exception(task, lgp, &exception);
> > 
> > This is borked… You’re now returning an NFSv4 status as an ERR_PTR(), which is a big no-no!
> > MAX_ERRNO is 4095, while NFS4ERR_RECALLCONFLICT takes the value 10061.
> > 
> 
> Ahh good catch!
> 
> Ok, I think the right fix there is probably to just declare the
> nfs4_exception in pnfs_update_layout, and then just pass a pointer to
> it down to this function. Then it can do the interpretation of the result at the point where we decide to retry.
> 
> I'll respin...
> 
> Thanks,
> Jeff
> 

Ok, incremental patch on top of the series is attached. I ended up not
dealing with the exception at higher layers, but rather just declaring
a new "ERECALLCONFLICT" error code and using that to indicate the
special behavior that those errors require. I'll plan to fold this into
the patch that broke this before I resend.

-- 
Jeff Layton <jlayton@poochiereds.net>

[-- Attachment #2: 0001-pnfs-rework-LAYOUTGET-error-handling.patch --]
[-- Type: text/x-patch, Size: 2414 bytes --]

From fd2cbdeaba1fbc56dbd394c0b232399d256f154e Mon Sep 17 00:00:00 2001
From: Jeff Layton <jeff.layton@primarydata.com>
Date: Tue, 17 May 2016 07:05:21 -0400
Subject: [PATCH] pnfs: rework LAYOUTGET error handling

IS_ERR can't handle errors in the range of -NFS4ERR_* error codes.

Signed-off-by: Jeff Layton <jeff.layton@primarydata.com>
---
 fs/nfs/nfs4proc.c     | 12 +++++++-----
 fs/nfs/pnfs.c         |  4 ++--
 include/linux/errno.h |  1 +
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 1254ed84c760..c2583ca6c8b6 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -7884,8 +7884,12 @@ nfs4_layoutget_handle_exception(struct rpc_task *task,
 			status = -EOVERFLOW;
 			goto out;
 		}
-		status = -NFS4ERR_RECALLCONFLICT;
-		break;
+		/* Fallthrough */
+	case -NFS4ERR_RECALLCONFLICT:
+		nfs4_handle_exception(server, -NFS4ERR_RECALLCONFLICT,
+					exception);
+		status = -ERECALLCONFLICT;
+		goto out;
 	case -NFS4ERR_EXPIRED:
 	case -NFS4ERR_BAD_STATEID:
 		exception->timeout = 0;
@@ -7917,9 +7921,7 @@ nfs4_layoutget_handle_exception(struct rpc_task *task,
 	}
 
 	status = nfs4_handle_exception(server, status, exception);
-	if (status == 0)
-		status = task->tk_status;
-	if (exception->retry && status != -NFS4ERR_RECALLCONFLICT)
+	if (exception->retry)
 		status = -EAGAIN;
 out:
 	dprintk("<-- %s\n", __func__);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index 13e1b57b22bf..deb609c9cd8a 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -1633,9 +1633,9 @@ lookup_again:
 	lseg = send_layoutget(lo, ctx, &stateid, &arg, &timeout, gfp_flags);
 	trace_pnfs_update_layout(ino, pos, count, iomode, lo, lseg,
 				 PNFS_UPDATE_LAYOUT_SEND_LAYOUTGET);
-	if (IS_ERR_OR_NULL(lseg)) {
+	if (IS_ERR(lseg)) {
 		switch(PTR_ERR(lseg)) {
-		case -NFS4ERR_RECALLCONFLICT:
+		case -ERECALLCONFLICT:
 			if (time_after(jiffies, giveup))
 				lseg = NULL;
 			/* Fallthrough */
diff --git a/include/linux/errno.h b/include/linux/errno.h
index 89627b9187f9..7ce9fb1b7d28 100644
--- a/include/linux/errno.h
+++ b/include/linux/errno.h
@@ -28,5 +28,6 @@
 #define EBADTYPE	527	/* Type not supported by server */
 #define EJUKEBOX	528	/* Request initiated, but will not complete before timeout */
 #define EIOCBQUEUED	529	/* iocb queued, will get completion event */
+#define ERECALLCONFLICT	530	/* conflict with recalled state */
 
 #endif
-- 
2.5.5