Return-Path: Received: from us-smtp-delivery-194.mimecast.com ([216.205.24.194]:52868 "EHLO us-smtp-delivery-194.mimecast.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750898AbcIPSEg (ORCPT ); Fri, 16 Sep 2016 14:04:36 -0400 From: Trond Myklebust To: David Vrabel CC: List Linux NFS Mailing , Schumaker Anna , List Linux Network Devel Mailing Subject: Re: [PATCHv1] sunrpc: fix write space race causing stalls Date: Fri, 16 Sep 2016 18:04:25 +0000 Message-ID: <44AD1332-26A4-4C15-99A2-595B292BD6E6@primarydata.com> References: <1474028902-19838-1-git-send-email-david.vrabel@citrix.com> <57DC20C8.5000306@citrix.com> <4BCBEAB3-1AD4-455E-9C0B-CCD3172C2C17@primarydata.com> <57DC2BFE.7040507@citrix.com> In-Reply-To: <57DC2BFE.7040507@citrix.com> MIME-Version: 1.0 Content-Type: text/plain; charset=WINDOWS-1252 Sender: linux-nfs-owner@vger.kernel.org List-ID: > On Sep 16, 2016, at 13:29, David Vrabel wrote: >=20 > On 16/09/16 18:06, Trond Myklebust wrote: >>=20 >>> On Sep 16, 2016, at 12:41, David Vrabel wrote= : >>>=20 >>> On 16/09/16 17:01, Trond Myklebust wrote: >>>>=20 >>>>> On Sep 16, 2016, at 08:28, David Vrabel wro= te: >>>>>=20 >>>>> Write space becoming available may race with putting the task to slee= p >>>>> in xprt_wait_for_buffer_space(). The existing mechanism to avoid the >>>>> race does not work. >>>>>=20 >>>>> This (edited) partial trace illustrates the problem: >>>>>=20 >>>>> [1] rpc_task_run_action: task:43546@5 ... action=3Dcall_transmit >>>>> [2] xs_write_space <-xs_tcp_write_space >>>>> [3] xprt_write_space <-xs_write_space >>>>> [4] rpc_task_sleep: task:43546@5 ... >>>>> [5] xs_write_space <-xs_tcp_write_space >>>>>=20 >>>>> [1] Task 43546 runs but is out of write space. >>>>>=20 >>>>> [2] Space becomes available, xs_write_space() clears the >>>>> SOCKWQ_ASYNC_NOSPACE bit. >>>>>=20 >>>>> [3] xprt_write_space() attemts to wake xprt->snd_task (=3D=3D 43546),= but >>>>> this has not yet been queued and the wake up is lost. >>>>>=20 >>>>> [4] xs_nospace() is called which calls xprt_wait_for_buffer_space() >>>>> which queues task 43546. >>>>>=20 >>>>> [5] The call to sk->sk_write_space() at the end of xs_nospace() (whic= h >>>>> is supposed to handle the above race) does not call >>>>> xprt_write_space() as the SOCKWQ_ASYNC_NOSPACE bit is clear and >>>>> thus the task is not woken. >>>>>=20 >>>>> Fix the race by have xprt_wait_for_buffer_space() check for write >>>>> space after putting the task to sleep. >>>>>=20 >>>>> Signed-off-by: David Vrabel >>>>> --- >>>>> include/linux/sunrpc/xprt.h | 1 + >>>>> net/sunrpc/xprt.c | 4 ++++ >>>>> net/sunrpc/xprtsock.c | 21 +++++++++++++++++++-- >>>>> 3 files changed, 24 insertions(+), 2 deletions(-) >>>>>=20 >>>>> diff --git a/include/linux/sunrpc/xprt.h b/include/linux/sunrpc/xprt.= h >>>>> index a16070d..621e74b 100644 >>>>> --- a/include/linux/sunrpc/xprt.h >>>>> +++ b/include/linux/sunrpc/xprt.h >>>>> @@ -129,6 +129,7 @@ struct rpc_xprt_ops { >>>>> =09void=09=09(*connect)(struct rpc_xprt *xprt, struct rpc_task *task)= ; >>>>> =09void *=09=09(*buf_alloc)(struct rpc_task *task, size_t size); >>>>> =09void=09=09(*buf_free)(void *buffer); >>>>> +=09bool (*have_write_space)(struct rpc_xprt *task); >>>>> =09int=09=09(*send_request)(struct rpc_task *task); >>>>> =09void=09=09(*set_retrans_timeout)(struct rpc_task *task); >>>>> =09void=09=09(*timer)(struct rpc_xprt *xprt, struct rpc_task *task); >>>>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c >>>>> index ea244b2..d3c1b1e 100644 >>>>> --- a/net/sunrpc/xprt.c >>>>> +++ b/net/sunrpc/xprt.c >>>>> @@ -502,6 +502,10 @@ void xprt_wait_for_buffer_space(struct rpc_task = *task, rpc_action action) >>>>>=20 >>>>> =09task->tk_timeout =3D RPC_IS_SOFT(task) ? req->rq_timeout : 0; >>>>> =09rpc_sleep_on(&xprt->pending, task, action); >>>>> + >>>>> +=09/* Write space notification may race with putting task to sleep. = */ >>>>> +=09if (xprt->ops->have_write_space(xprt)) >>>>> +=09=09rpc_wake_up_queued_task(&xprt->pending, task); >>>>> } >>>>> EXPORT_SYMBOL_GPL(xprt_wait_for_buffer_space); >>>>>=20 >>>>> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c >>>>> index bf16883..211de5b 100644 >>>>> --- a/net/sunrpc/xprtsock.c >>>>> +++ b/net/sunrpc/xprtsock.c >>>>> @@ -472,8 +472,6 @@ static int xs_nospace(struct rpc_task *task) >>>>>=20 >>>>> =09spin_unlock_bh(&xprt->transport_lock); >>>>>=20 >>>>> -=09/* Race breaker in case memory is freed before above code is call= ed */ >>>>> -=09sk->sk_write_space(sk); >>>>> =09return ret; >>>>> } >>>>=20 >>>> Instead of these callbacks, why not just add a call to >>>> sk_set_bit(SOCKWQ_ASYNC_WAITDATA, sk) after queueing the task in >>>> xs_nospace()? Won=92t that fix the existing race breaker? >>>=20 >>> I don't see how that would help. If sk->sk_write_space was already >>> called, SOCKWQ_ASYNC_NOSPACE will still be clear and the next call to >>> sk->sk_write_space will still be a nop. >>=20 >> Sorry. Copy+paste error. I meant SOCKWQ_ASYNC_NOSPACE. >>=20 >>>=20 >>> Or did you mean SOCKWQ_ASYNC_NOSPACE here? It doesn't seem right to se= t >>> this bit when we don't know if there's space or not. >>=20 >> Why not? >=20 > I prefer my solution because: >=20 > a) It obviously fixes the race (games with bits are less understandable). >=20 > b) It requires fewer atomic ops. >=20 > c) It doesn't require me to understand what the behaviour of the > socket-internal SOCKWQ_ASYNC_NOSPACE bit is or should be. >=20 > d) I'm not sure I understand the objection to the additional > have_write_space method -- it has simple, clear behaviour. >=20 I don=92t see the point of adding 24 lines of code over 3 different files i= f the problem can be solved with 1 line of code.