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

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

[Jeff Layton]

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@netapp.com>
Signed-off-by: Jeff Layton <jlayton@redhat.com>
---
 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;
-- 
1.7.7.6

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

[Jeff Layton]

On Fri, 18 May 2012 16:16:28 -0400
Jeff Layton <jlayton@redhat.com> 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@netapp.com>
> Signed-off-by: Jeff Layton <jlayton@redhat.com>
> ---
>  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).

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?
-- 
Jeff Layton <jlayton@redhat.com>

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

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTA1LTE4IGF0IDE2OjE2IC0wNDAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
eHBydF9hbGxvY19zbG90IHdpbGwgY2FsbCBycGNfZGVsYXkoKSB0byBtYWtlIHRoZSB0YXNrIHdh
aXQgYSBiaXQgYmVmb3JlDQo+IHJldHJ5aW5nIHdoZW4gaXQgZ2V0cyBiYWNrIGFuIC1FTk9NRU0g
ZXJyb3IgZnJvbSB4cHJ0X2R5bmFtaWNfYWxsb2Nfc2xvdC4NCj4gDQo+IFRoZSBwcm9ibGVtIHRo
ZXJlIGlzIHRoYXQgcnBjX2RlbGF5IGRvZXMgdGhpczoNCj4gDQo+ICAgICBycGNfc2xlZXBfb24o
JmRlbGF5X3F1ZXVlLCB0YXNrLCBfX3JwY19hdHJ1bik7DQo+IA0KPiAuLi5hbmQgX19ycGNfYXRy
dW4gd2lsbCByZXNldCB0aGUgdGFzay0+dGtfc3RhdHVzIGJhY2sgdG8gMCB3aGVuIHRoZQ0KPiB0
YXNrIHdha2VzIGJhY2sgdXAuIFRoaXMgbWFrZXMgY2FsbF9yZXNlcnZlcmVzdWx0IGFib3J0IHRo
ZSB0YXNrIHdpdGgNCj4gLUVJTy4NCj4gDQo+IFRoZSBSSCBRQSBncm91cCBkaXNjb3ZlcmVkIHRo
aXMgcHJvYmxlbSB3aGVuIHRlc3Rpbmcgd2l0aCBsb3ctbWVtb3J5DQo+IGNsaWVudHMgYW5kIHRo
aXMgcGF0Y2ggZml4ZWQgaXQ6DQo+IA0KPiAgICAgaHR0cHM6Ly9idWd6aWxsYS5yZWRoYXQuY29t
L3Nob3dfYnVnLmNnaT9pZD04MjIxODkNCj4gDQo+IEZpeCB0aGlzIGJ5IGFkZGluZyBhIG5ldyBy
cGNfZGVsYXlfYWN0aW9uKCkgdmFyaWFudCB0aGF0IG1ha2VzIGFsbG93cw0KPiB1cyB0byBwYXNz
IGluIGEgcnBjX2FjdGlvbiB0byByZXNldCB0aGUgc3RhdHVzIGJhY2sgdG8gLUVBR0FJTiBpbnN0
ZWFkLg0KDQpIaSBKZWZmLA0KDQpJJ20gbm90IHN1cmUgdGhhdCBJIGxpa2UgdGhlIGlkZWEgb2Yg
YW4gcnBjX2RlbGF5IHZhcmlhbnQgdGhhdCBmYWlscyB0bw0KcmVzZXQgdGhlIHRrX3N0YXR1cy4g
SG93IGFib3V0IHNvbWV0aGluZyBsaWtlIHRoZSBmb2xsb3dpbmcgc29sdXRpb24NCmluc3RlYWQ/
DQoNCkNoZWVycw0KICBUcm9uZA0KODwtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0t
LS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0NCkZyb20gMWFmZWFmNWMyOWFhMDdk
YjI1NzYwZDJmYmVkNWMwOGEzYWVjMzQ5OCBNb24gU2VwIDE3IDAwOjAwOjAwIDIwMDENCkZyb206
IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQpEYXRlOiBTYXQs
IDE5IE1heSAyMDEyIDEyOjEyOjUzIC0wNDAwDQpTdWJqZWN0OiBbUEFUQ0hdIHN1bnJwYzogZml4
IGxvc3Mgb2YgdGFzay0+dGtfc3RhdHVzIGFmdGVyIHJwY19kZWxheSBjYWxsIGluDQogeHBydF9h
bGxvY19zbG90DQoNCnhwcnRfYWxsb2Nfc2xvdCB3aWxsIGNhbGwgcnBjX2RlbGF5KCkgdG8gbWFr
ZSB0aGUgdGFzayB3YWl0IGEgYml0IGJlZm9yZQ0KcmV0cnlpbmcgd2hlbiBpdCBnZXRzIGJhY2sg
YW4gLUVOT01FTSBlcnJvciBmcm9tIHhwcnRfZHluYW1pY19hbGxvY19zbG90Lg0KVGhlIHByb2Js
ZW0gaXMgdGhhdCBycGNfZGVsYXkgd2lsbCBjbGVhciB0aGUgdGFzay0+dGtfc3RhdHVzLCBjYXVz
aW5nDQpjYWxsX3Jlc2VydmVyZXN1bHQgdG8gYWJvcnQgdGhlIHRhc2suDQoNClRoZSBzb2x1dGlv
biBpcyBzaW1wbHkgdG8gbGV0IGNhbGxfcmVzZXJ2ZXJlc3VsdCBoYW5kbGUgdGhlIEVOT01FTSBl
cnJvcg0KZGlyZWN0bHkuDQoNClJlcG9ydGVkLWJ5OiBKZWZmIExheXRvbiA8amxheXRvbkByZWRo
YXQuY29tPg0KQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcgWz49IDMuMV0NClNpZ25lZC1vZmYt
Ynk6IFRyb25kIE15a2xlYnVzdCA8VHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20+DQotLS0NCiBu
ZXQvc3VucnBjL2NsbnQuYyB8ICAgIDIgKysNCiBuZXQvc3VucnBjL3hwcnQuYyB8ICAgIDUgKysr
LS0NCiAyIGZpbGVzIGNoYW5nZWQsIDUgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCg0K
ZGlmZiAtLWdpdCBhL25ldC9zdW5ycGMvY2xudC5jIGIvbmV0L3N1bnJwYy9jbG50LmMNCmluZGV4
IGFkZjI5OTAuLjI1MzAyYzggMTAwNjQ0DQotLS0gYS9uZXQvc3VucnBjL2NsbnQuYw0KKysrIGIv
bmV0L3N1bnJwYy9jbG50LmMNCkBAIC0xMjg4LDYgKzEyODgsOCBAQCBjYWxsX3Jlc2VydmVyZXN1
bHQoc3RydWN0IHJwY190YXNrICp0YXNrKQ0KIAl9DQogDQogCXN3aXRjaCAoc3RhdHVzKSB7DQor
CWNhc2UgLUVOT01FTToNCisJCXJwY19kZWxheSh0YXNrLCBIWiA+PiAyKTsNCiAJY2FzZSAtRUFH
QUlOOgkvKiB3b2tlbiB1cDsgcmV0cnkgKi8NCiAJCXRhc2stPnRrX2FjdGlvbiA9IGNhbGxfcmVz
ZXJ2ZTsNCiAJCXJldHVybjsNCmRpZmYgLS1naXQgYS9uZXQvc3VucnBjL3hwcnQuYyBiL25ldC9z
dW5ycGMveHBydC5jDQppbmRleCBiMjM5ZTc1Li5kN2NjZDc5IDEwMDY0NA0KLS0tIGEvbmV0L3N1
bnJwYy94cHJ0LmMNCisrKyBiL25ldC9zdW5ycGMveHBydC5jDQpAQCAtOTg0LDE1ICs5ODQsMTYg
QEAgc3RhdGljIHZvaWQgeHBydF9hbGxvY19zbG90KHN0cnVjdCBycGNfdGFzayAqdGFzaykNCiAJ
CWdvdG8gb3V0X2luaXRfcmVxOw0KIAlzd2l0Y2ggKFBUUl9FUlIocmVxKSkgew0KIAljYXNlIC1F
Tk9NRU06DQotCQlycGNfZGVsYXkodGFzaywgSFogPj4gMik7DQogCQlkcHJpbnRrKCJSUEM6ICAg
ICAgIGR5bmFtaWMgYWxsb2NhdGlvbiBvZiByZXF1ZXN0IHNsb3QgIg0KIAkJCQkiZmFpbGVkISBS
ZXRyeWluZ1xuIik7DQorCQl0YXNrLT50a19zdGF0dXMgPSAtRU5PTUVNOw0KIAkJYnJlYWs7DQog
CWNhc2UgLUVBR0FJTjoNCiAJCXJwY19zbGVlcF9vbigmeHBydC0+YmFja2xvZywgdGFzaywgTlVM
TCk7DQogCQlkcHJpbnRrKCJSUEM6ICAgICAgIHdhaXRpbmcgZm9yIHJlcXVlc3Qgc2xvdFxuIik7
DQorCWRlZmF1bHQ6DQorCQl0YXNrLT50a19zdGF0dXMgPSAtRUFHQUlOOw0KIAl9DQotCXRhc2st
PnRrX3N0YXR1cyA9IC1FQUdBSU47DQogCXJldHVybjsNCiBvdXRfaW5pdF9yZXE6DQogCXRhc2st
PnRrX3N0YXR1cyA9IDA7DQotLSANCjEuNy43LjYNCg0KDQotLSANClRyb25kIE15a2xlYnVzdA0K
TGludXggTkZTIGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5l
dGFwcC5jb20NCnd3dy5uZXRhcHAuY29tDQoNCg==

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

[Myklebust, Trond]

T24gRnJpLCAyMDEyLTA1LTE4IGF0IDE2OjMzIC0wNDAwLCBKZWZmIExheXRvbiB3cm90ZToNCj4g
T24gRnJpLCAxOCBNYXkgMjAxMiAxNjoxNjoyOCAtMDQwMA0KPiBKZWZmIExheXRvbiA8amxheXRv
bkByZWRoYXQuY29tPiB3cm90ZToNCj4gDQo+ID4geHBydF9hbGxvY19zbG90IHdpbGwgY2FsbCBy
cGNfZGVsYXkoKSB0byBtYWtlIHRoZSB0YXNrIHdhaXQgYSBiaXQgYmVmb3JlDQo+ID4gcmV0cnlp
bmcgd2hlbiBpdCBnZXRzIGJhY2sgYW4gLUVOT01FTSBlcnJvciBmcm9tIHhwcnRfZHluYW1pY19h
bGxvY19zbG90Lg0KPiA+IA0KPiA+IFRoZSBwcm9ibGVtIHRoZXJlIGlzIHRoYXQgcnBjX2RlbGF5
IGRvZXMgdGhpczoNCj4gPiANCj4gPiAgICAgcnBjX3NsZWVwX29uKCZkZWxheV9xdWV1ZSwgdGFz
aywgX19ycGNfYXRydW4pOw0KPiA+IA0KPiA+IC4uLmFuZCBfX3JwY19hdHJ1biB3aWxsIHJlc2V0
IHRoZSB0YXNrLT50a19zdGF0dXMgYmFjayB0byAwIHdoZW4gdGhlDQo+ID4gdGFzayB3YWtlcyBi
YWNrIHVwLiBUaGlzIG1ha2VzIGNhbGxfcmVzZXJ2ZXJlc3VsdCBhYm9ydCB0aGUgdGFzayB3aXRo
DQo+ID4gLUVJTy4NCj4gPiANCj4gPiBUaGUgUkggUUEgZ3JvdXAgZGlzY292ZXJlZCB0aGlzIHBy
b2JsZW0gd2hlbiB0ZXN0aW5nIHdpdGggbG93LW1lbW9yeQ0KPiA+IGNsaWVudHMgYW5kIHRoaXMg
cGF0Y2ggZml4ZWQgaXQ6DQo+ID4gDQo+ID4gICAgIGh0dHBzOi8vYnVnemlsbGEucmVkaGF0LmNv
bS9zaG93X2J1Zy5jZ2k/aWQ9ODIyMTg5DQo+ID4gDQo+ID4gRml4IHRoaXMgYnkgYWRkaW5nIGEg
bmV3IHJwY19kZWxheV9hY3Rpb24oKSB2YXJpYW50IHRoYXQgbWFrZXMgYWxsb3dzDQo+ID4gdXMg
dG8gcGFzcyBpbiBhIHJwY19hY3Rpb24gdG8gcmVzZXQgdGhlIHN0YXR1cyBiYWNrIHRvIC1FQUdB
SU4gaW5zdGVhZC4NCj4gPiANCj4gPiBDYzogQW5keSBBZGFtc29uIDxhbmRyb3NAbmV0YXBwLmNv
bT4NCj4gPiBTaWduZWQtb2ZmLWJ5OiBKZWZmIExheXRvbiA8amxheXRvbkByZWRoYXQuY29tPg0K
PiA+IC0tLQ0KPiA+ICBpbmNsdWRlL2xpbnV4L3N1bnJwYy9zY2hlZC5oIHwgICAgMSArDQo+ID4g
IG5ldC9zdW5ycGMvc2NoZWQuYyAgICAgICAgICAgfCAgICA5ICsrKysrKystLQ0KPiA+ICBuZXQv
c3VucnBjL3hwcnQuYyAgICAgICAgICAgIHwgICAgNyArKysrKystDQo+ID4gIDMgZmlsZXMgY2hh
bmdlZCwgMTQgaW5zZXJ0aW9ucygrKSwgMyBkZWxldGlvbnMoLSkNCj4gPiANCj4gPiBkaWZmIC0t
Z2l0IGEvaW5jbHVkZS9saW51eC9zdW5ycGMvc2NoZWQuaCBiL2luY2x1ZGUvbGludXgvc3VucnBj
L3NjaGVkLmgNCj4gPiBpbmRleCBkYzBjM2NjLi42Y2FmZGJiIDEwMDY0NA0KPiA+IC0tLSBhL2lu
Y2x1ZGUvbGludXgvc3VucnBjL3NjaGVkLmgNCj4gPiArKysgYi9pbmNsdWRlL2xpbnV4L3N1bnJw
Yy9zY2hlZC5oDQo+ID4gQEAgLTI0MSw2ICsyNDEsNyBAQCBzdHJ1Y3QgcnBjX3Rhc2sgKnJwY193
YWtlX3VwX2ZpcnN0KHN0cnVjdCBycGNfd2FpdF9xdWV1ZSAqLA0KPiA+ICAJCQkJCXZvaWQgKik7
DQo+ID4gIHZvaWQJCXJwY193YWtlX3VwX3N0YXR1cyhzdHJ1Y3QgcnBjX3dhaXRfcXVldWUgKiwg
aW50KTsNCj4gPiAgaW50CQlycGNfcXVldWVfZW1wdHkoc3RydWN0IHJwY193YWl0X3F1ZXVlICop
Ow0KPiA+ICt2b2lkCQlycGNfZGVsYXlfYWN0aW9uKHN0cnVjdCBycGNfdGFzayAqLCB1bnNpZ25l
ZCBsb25nLCBycGNfYWN0aW9uKTsNCj4gPiAgdm9pZAkJcnBjX2RlbGF5KHN0cnVjdCBycGNfdGFz
ayAqLCB1bnNpZ25lZCBsb25nKTsNCj4gPiAgdm9pZCAqCQlycGNfbWFsbG9jKHN0cnVjdCBycGNf
dGFzayAqLCBzaXplX3QpOw0KPiA+ICB2b2lkCQlycGNfZnJlZSh2b2lkICopOw0KPiA+IGRpZmYg
LS1naXQgYS9uZXQvc3VucnBjL3NjaGVkLmMgYi9uZXQvc3VucnBjL3NjaGVkLmMNCj4gPiBpbmRl
eCA5OTRjZmVhLi5mMmZmOThkIDEwMDY0NA0KPiA+IC0tLSBhL25ldC9zdW5ycGMvc2NoZWQuYw0K
PiA+ICsrKyBiL25ldC9zdW5ycGMvc2NoZWQuYw0KPiA+IEBAIC02MTMsMTMgKzYxMywxOCBAQCBz
dGF0aWMgdm9pZCBfX3JwY19hdHJ1bihzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2spDQo+ID4gIAl0YXNr
LT50a19zdGF0dXMgPSAwOw0KPiA+ICB9DQo+ID4gIA0KPiA+ICt2b2lkIHJwY19kZWxheV9hY3Rp
b24oc3RydWN0IHJwY190YXNrICp0YXNrLCB1bnNpZ25lZCBsb25nIGRlbGF5LCBycGNfYWN0aW9u
IGFjdGlvbikNCj4gPiArew0KPiA+ICsJdGFzay0+dGtfdGltZW91dCA9IGRlbGF5Ow0KPiA+ICsJ
cnBjX3NsZWVwX29uKCZkZWxheV9xdWV1ZSwgdGFzaywgYWN0aW9uKTsNCj4gPiArfQ0KPiA+ICsN
Cj4gPiAgLyoNCj4gPiAgICogUnVuIGEgdGFzayBhdCBhIGxhdGVyIHRpbWUNCj4gPiAgICovDQo+
ID4gIHZvaWQgcnBjX2RlbGF5KHN0cnVjdCBycGNfdGFzayAqdGFzaywgdW5zaWduZWQgbG9uZyBk
ZWxheSkNCj4gPiAgew0KPiA+IC0JdGFzay0+dGtfdGltZW91dCA9IGRlbGF5Ow0KPiA+IC0JcnBj
X3NsZWVwX29uKCZkZWxheV9xdWV1ZSwgdGFzaywgX19ycGNfYXRydW4pOw0KPiA+ICsJcnBjX2Rl
bGF5X2FjdGlvbih0YXNrLCBkZWxheSwgX19ycGNfYXRydW4pOw0KPiA+ICB9DQo+ID4gIEVYUE9S
VF9TWU1CT0xfR1BMKHJwY19kZWxheSk7DQo+ID4gIA0KPiA+IGRpZmYgLS1naXQgYS9uZXQvc3Vu
cnBjL3hwcnQuYyBiL25ldC9zdW5ycGMveHBydC5jDQo+ID4gaW5kZXggYjIzOWU3NS4uYTVkYzgx
NiAxMDA2NDQNCj4gPiAtLS0gYS9uZXQvc3VucnBjL3hwcnQuYw0KPiA+ICsrKyBiL25ldC9zdW5y
cGMveHBydC5jDQo+ID4gQEAgLTk2OSw2ICs5NjksMTEgQEAgc3RhdGljIGJvb2wgeHBydF9keW5h
bWljX2ZyZWVfc2xvdChzdHJ1Y3QgcnBjX3hwcnQgKnhwcnQsIHN0cnVjdCBycGNfcnFzdCAqcmVx
KQ0KPiA+ICAJcmV0dXJuIGZhbHNlOw0KPiA+ICB9DQo+ID4gIA0KPiA+ICtzdGF0aWMgdm9pZCB4
cHJ0X2FsbG9jX2RlbGF5X3Jlc2V0X3N0YXR1cyhzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2spDQo+ID4g
K3sNCj4gPiArCXRhc2stPnRrX3N0YXR1cyA9IC1FQUdBSU47DQo+ID4gK30NCj4gPiArDQo+ID4g
IHN0YXRpYyB2b2lkIHhwcnRfYWxsb2Nfc2xvdChzdHJ1Y3QgcnBjX3Rhc2sgKnRhc2spDQo+ID4g
IHsNCj4gPiAgCXN0cnVjdCBycGNfeHBydAkqeHBydCA9IHRhc2stPnRrX3hwcnQ7DQo+ID4gQEAg
LTk4NCw3ICs5ODksNyBAQCBzdGF0aWMgdm9pZCB4cHJ0X2FsbG9jX3Nsb3Qoc3RydWN0IHJwY190
YXNrICp0YXNrKQ0KPiA+ICAJCWdvdG8gb3V0X2luaXRfcmVxOw0KPiA+ICAJc3dpdGNoIChQVFJf
RVJSKHJlcSkpIHsNCj4gPiAgCWNhc2UgLUVOT01FTToNCj4gPiAtCQlycGNfZGVsYXkodGFzaywg
SFogPj4gMik7DQo+ID4gKwkJcnBjX2RlbGF5X2FjdGlvbih0YXNrLCBIWiA+PiAyLCB4cHJ0X2Fs
bG9jX2RlbGF5X3Jlc2V0X3N0YXR1cyk7DQo+ID4gIAkJZHByaW50aygiUlBDOiAgICAgICBkeW5h
bWljIGFsbG9jYXRpb24gb2YgcmVxdWVzdCBzbG90ICINCj4gPiAgCQkJCSJmYWlsZWQhIFJldHJ5
aW5nXG4iKTsNCj4gPiAgCQlicmVhazsNCj4gDQo+IA0KPiBOb3cgdGhhdCBJIGxvb2sgYXQgdGhp
cyBtb3JlIGNsb3NlbHkgdGhvdWdoLi4uSSB3b25kZXIgaWYgd2UncmUgZ29pbmcNCj4gdG8gZW5k
IHVwIHdpdGggc2ltaWxhciBwcm9ibGVtcyBpbiB0aGUgRUFHQUlOIGNhc2UuIEJlY2F1c2Ugd2Ug
aGF2ZQ0KPiB0a190aW1lb3V0PT0wIGF0IHRoaXMgcG9pbnQsIEkgdGhpbmsgd2UnbGwgYWx3YXlz
IGVuZCB1cCBoYXZpbmcgdGhlDQo+IHRrX3N0YXR1cyBzZXQgdG8gLUVUSU1FRE9VVCBpbiB0aGUg
ZXZlbnQgdGhhdCB4cHJ0X2R5bmFtaWNfYWxsb2Nfc2xvdA0KPiByZXR1cm5zIEVBR0FJTiAoZHVl
IHRvIF9fcnBjX3F1ZXVlX3RpbWVyX2ZuKS4NCg0KQW0gSSBtaXNzaW5nIHNvbWV0aGluZz8NCg0K
eHBydF9hbGxvY19zbG90IGNhbGxzIHJwY19zbGVlcF9vbiB3aXRoIHRhc2stPnRrX3RpbWVvdXQg
PT0gMCwgd2hpY2gNCm1lYW5zIF9fcnBjX2FkZF90aW1lciBxdWl0cyB3aXRob3V0IGFkZGluZyB0
aGUgdGFzayB0byB0aGUNCnF1ZXVlLT50aW1lcl9saXN0LiBIb3cgd291bGQgX19ycGNfcXVldWVf
dGltZXJfZm4gZmluZCBpdD8NCg0KPiBQZXJoYXBzIHdlIG5lZWQgYSBzaW1pbGFyIGZpeCB0aGVy
ZT8gT3IgbWF5YmUgd2Ugc2hvdWxkIGp1c3QgcHVzaCB0aGUNCj4gZXJyb3IgaGFuZGxpbmcgZm9y
IHhwcnRfYWxsb2Nfc2xvdCBpbnRvIGNhbGxfcmVzZXJ2ZXJlc3VsdCBhbmQgYnlwYXNzDQo+IHRo
ZSB3aG9sZSBwcm9ibGVtIHRoYXQgd2F5Pw0KPiANCj4gVGhvdWdodHM/DQoNClNlZSB0aGUgcGF0
Y2ggSSBzZW50IG91dCBiZWZvcmUgSSBzYXcgdGhpcyBlbWFpbC4gV2UnbGwgc29sdmUgdGhpcyBp
bg0KY2FsbF9yZXNlcnZlcmVzdWx0Lg0KDQotLSANClRyb25kIE15a2xlYnVzdA0KTGludXggTkZT
IGNsaWVudCBtYWludGFpbmVyDQoNCk5ldEFwcA0KVHJvbmQuTXlrbGVidXN0QG5ldGFwcC5jb20N
Cnd3dy5uZXRhcHAuY29tDQoNCg==

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

[Jeff Layton]

On Sat, 19 May 2012 19:33:19 +0000
"Myklebust, Trond" <Trond.Myklebust@netapp.com> 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@redhat.com> 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@netapp.com>
> > > Signed-off-by: Jeff Layton <jlayton@redhat.com>
> > > ---
> > >  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@redhat.com>