Re: [PATCH] sunrpc: fix loss of task->tk_status after rpc_delay call in xprt_alloc_slot

[Jeff Layton <jlayton@xxxxxxxxxx>]

On Sat, 19 May 2012 19:33:19 +0000
"Myklebust, Trond" <Trond.Myklebust@xxxxxxxxxx> wrote:

> On Fri, 2012-05-18 at 16:33 -0400, Jeff Layton wrote:
> > On Fri, 18 May 2012 16:16:28 -0400
> > Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > 
> > > xprt_alloc_slot will call rpc_delay() to make the task wait a bit before
> > > retrying when it gets back an -ENOMEM error from xprt_dynamic_alloc_slot.
> > > 
> > > The problem there is that rpc_delay does this:
> > > 
> > >     rpc_sleep_on(&delay_queue, task, __rpc_atrun);
> > > 
> > > ...and __rpc_atrun will reset the task->tk_status back to 0 when the
> > > task wakes back up. This makes call_reserveresult abort the task with
> > > -EIO.
> > > 
> > > The RH QA group discovered this problem when testing with low-memory
> > > clients and this patch fixed it:
> > > 
> > >     https://bugzilla.redhat.com/show_bug.cgi?id=822189
> > > 
> > > Fix this by adding a new rpc_delay_action() variant that makes allows
> > > us to pass in a rpc_action to reset the status back to -EAGAIN instead.
> > > 
> > > Cc: Andy Adamson <andros@xxxxxxxxxx>
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > >  include/linux/sunrpc/sched.h |    1 +
> > >  net/sunrpc/sched.c           |    9 +++++++--
> > >  net/sunrpc/xprt.c            |    7 ++++++-
> > >  3 files changed, 14 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/include/linux/sunrpc/sched.h b/include/linux/sunrpc/sched.h
> > > index dc0c3cc..6cafdbb 100644
> > > --- a/include/linux/sunrpc/sched.h
> > > +++ b/include/linux/sunrpc/sched.h
> > > @@ -241,6 +241,7 @@ struct rpc_task *rpc_wake_up_first(struct rpc_wait_queue *,
> > >  					void *);
> > >  void		rpc_wake_up_status(struct rpc_wait_queue *, int);
> > >  int		rpc_queue_empty(struct rpc_wait_queue *);
> > > +void		rpc_delay_action(struct rpc_task *, unsigned long, rpc_action);
> > >  void		rpc_delay(struct rpc_task *, unsigned long);
> > >  void *		rpc_malloc(struct rpc_task *, size_t);
> > >  void		rpc_free(void *);
> > > diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c
> > > index 994cfea..f2ff98d 100644
> > > --- a/net/sunrpc/sched.c
> > > +++ b/net/sunrpc/sched.c
> > > @@ -613,13 +613,18 @@ static void __rpc_atrun(struct rpc_task *task)
> > >  	task->tk_status = 0;
> > >  }
> > >  
> > > +void rpc_delay_action(struct rpc_task *task, unsigned long delay, rpc_action action)
> > > +{
> > > +	task->tk_timeout = delay;
> > > +	rpc_sleep_on(&delay_queue, task, action);
> > > +}
> > > +
> > >  /*
> > >   * Run a task at a later time
> > >   */
> > >  void rpc_delay(struct rpc_task *task, unsigned long delay)
> > >  {
> > > -	task->tk_timeout = delay;
> > > -	rpc_sleep_on(&delay_queue, task, __rpc_atrun);
> > > +	rpc_delay_action(task, delay, __rpc_atrun);
> > >  }
> > >  EXPORT_SYMBOL_GPL(rpc_delay);
> > >  
> > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > index b239e75..a5dc816 100644
> > > --- a/net/sunrpc/xprt.c
> > > +++ b/net/sunrpc/xprt.c
> > > @@ -969,6 +969,11 @@ static bool xprt_dynamic_free_slot(struct rpc_xprt *xprt, struct rpc_rqst *req)
> > >  	return false;
> > >  }
> > >  
> > > +static void xprt_alloc_delay_reset_status(struct rpc_task *task)
> > > +{
> > > +	task->tk_status = -EAGAIN;
> > > +}
> > > +
> > >  static void xprt_alloc_slot(struct rpc_task *task)
> > >  {
> > >  	struct rpc_xprt	*xprt = task->tk_xprt;
> > > @@ -984,7 +989,7 @@ static void xprt_alloc_slot(struct rpc_task *task)
> > >  		goto out_init_req;
> > >  	switch (PTR_ERR(req)) {
> > >  	case -ENOMEM:
> > > -		rpc_delay(task, HZ >> 2);
> > > +		rpc_delay_action(task, HZ >> 2, xprt_alloc_delay_reset_status);
> > >  		dprintk("RPC:       dynamic allocation of request slot "
> > >  				"failed! Retrying\n");
> > >  		break;
> > 
> > 
> > Now that I look at this more closely though...I wonder if we're going
> > to end up with similar problems in the EAGAIN case. Because we have
> > tk_timeout==0 at this point, I think we'll always end up having the
> > tk_status set to -ETIMEDOUT in the event that xprt_dynamic_alloc_slot
> > returns EAGAIN (due to __rpc_queue_timer_fn).
> 
> Am I missing something?
> 
> xprt_alloc_slot calls rpc_sleep_on with task->tk_timeout == 0, which
> means __rpc_add_timer quits without adding the task to the
> queue->timer_list. How would __rpc_queue_timer_fn find it?
> 

No, you're correct.

> > Perhaps we need a similar fix there? Or maybe we should just push the
> > error handling for xprt_alloc_slot into call_reserveresult and bypass
> > the whole problem that way?
> > 
> > Thoughts?
> 
> See the patch I sent out before I saw this email. We'll solve this in
> call_reserveresult.
> 

Yes, I think that's a cleaner approach now that I look at it. That
patch should solve the problem.

Should we also move the rpc_sleep_on in the -EAGAIN case to
call_reserveresult while we're at it? It seems a little confusing to
handle the delay in those cases within different functions.

Thanks,
-- 
Jeff Layton <jlayton@xxxxxxxxxx>
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html