Write space becoming available may race with putting the task to sleep
in xprt_wait_for_buffer_space(). The existing mechanism to avoid the
race does not work.
This (edited) partial trace illustrates the problem:
[1] rpc_task_run_action: task:43546@5 ... action=call_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
[1] Task 43546 runs but is out of write space.
[2] Space becomes available, xs_write_space() clears the
SOCKWQ_ASYNC_NOSPACE bit.
[3] xprt_write_space() attemts to wake xprt->snd_task (== 43546), but
this has not yet been queued and the wake up is lost.
[4] xs_nospace() is called which calls xprt_wait_for_buffer_space()
which queues task 43546.
[5] The call to sk->sk_write_space() at the end of xs_nospace() (which
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.
Fix the race by have xprt_wait_for_buffer_space() check for write
space after putting the task to sleep.
Signed-off-by: David Vrabel <[email protected]>
---
include/linux/sunrpc/xprt.h | 1 +
net/sunrpc/xprt.c | 4 ++++
net/sunrpc/xprtsock.c | 21 +++++++++++++++++++--
3 files changed, 24 insertions(+), 2 deletions(-)
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 {
void (*connect)(struct rpc_xprt *xprt, struct rpc_task *task);
void * (*buf_alloc)(struct rpc_task *task, size_t size);
void (*buf_free)(void *buffer);
+ bool (*have_write_space)(struct rpc_xprt *task);
int (*send_request)(struct rpc_task *task);
void (*set_retrans_timeout)(struct rpc_task *task);
void (*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)
task->tk_timeout = RPC_IS_SOFT(task) ? req->rq_timeout : 0;
rpc_sleep_on(&xprt->pending, task, action);
+
+ /* Write space notification may race with putting task to sleep. */
+ if (xprt->ops->have_write_space(xprt))
+ rpc_wake_up_queued_task(&xprt->pending, task);
}
EXPORT_SYMBOL_GPL(xprt_wait_for_buffer_space);
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)
spin_unlock_bh(&xprt->transport_lock);
- /* Race breaker in case memory is freed before above code is called */
- sk->sk_write_space(sk);
return ret;
}
@@ -1679,6 +1677,22 @@ static void xs_tcp_write_space(struct sock *sk)
read_unlock_bh(&sk->sk_callback_lock);
}
+static bool xs_udp_have_write_space(struct rpc_xprt *xprt)
+{
+ struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+ struct sock *sk = transport->inet;
+
+ return sock_writeable(sk);
+}
+
+static bool xs_tcp_have_write_space(struct rpc_xprt *xprt)
+{
+ struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
+ struct sock *sk = transport->inet;
+
+ return sk_stream_is_writeable(sk);
+}
+
static void xs_udp_do_set_buffer_size(struct rpc_xprt *xprt)
{
struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
@@ -2664,6 +2678,7 @@ static struct rpc_xprt_ops xs_local_ops = {
.connect = xs_local_connect,
.buf_alloc = rpc_malloc,
.buf_free = rpc_free,
+ .have_write_space = xs_udp_have_write_space,
.send_request = xs_local_send_request,
.set_retrans_timeout = xprt_set_retrans_timeout_def,
.close = xs_close,
@@ -2683,6 +2698,7 @@ static struct rpc_xprt_ops xs_udp_ops = {
.connect = xs_connect,
.buf_alloc = rpc_malloc,
.buf_free = rpc_free,
+ .have_write_space = xs_udp_have_write_space,
.send_request = xs_udp_send_request,
.set_retrans_timeout = xprt_set_retrans_timeout_rtt,
.timer = xs_udp_timer,
@@ -2704,6 +2720,7 @@ static struct rpc_xprt_ops xs_tcp_ops = {
.connect = xs_connect,
.buf_alloc = rpc_malloc,
.buf_free = rpc_free,
+ .have_write_space = xs_tcp_have_write_space,
.send_request = xs_tcp_send_request,
.set_retrans_timeout = xprt_set_retrans_timeout_def,
.close = xs_tcp_shutdown,
--
2.1.4
> On Sep 16, 2016, at 08:28, David Vrabel <[email protected]> wrote:
>=20
> Write space becoming available may race with putting the task to sleep
> 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() (which
> 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 <[email protected]>
> ---
> 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 *tas=
k, 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 called *=
/
> -=09sk->sk_write_space(sk);
> =09return ret;
> }
Instead of these callbacks, why not just add a call to sk_set_bit(SOCKWQ_AS=
YNC_WAITDATA, sk) after queueing the task in xs_nospace()? Won=92t that fix=
the existing race breaker?
Cheers
Trond
On 16/09/16 17:01, Trond Myklebust wrote:
>
>> On Sep 16, 2016, at 08:28, David Vrabel <[email protected]> wrote:
>>
>> Write space becoming available may race with putting the task to sleep
>> in xprt_wait_for_buffer_space(). The existing mechanism to avoid the
>> race does not work.
>>
>> This (edited) partial trace illustrates the problem:
>>
>> [1] rpc_task_run_action: task:43546@5 ... action=call_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
>>
>> [1] Task 43546 runs but is out of write space.
>>
>> [2] Space becomes available, xs_write_space() clears the
>> SOCKWQ_ASYNC_NOSPACE bit.
>>
>> [3] xprt_write_space() attemts to wake xprt->snd_task (== 43546), but
>> this has not yet been queued and the wake up is lost.
>>
>> [4] xs_nospace() is called which calls xprt_wait_for_buffer_space()
>> which queues task 43546.
>>
>> [5] The call to sk->sk_write_space() at the end of xs_nospace() (which
>> 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.
>>
>> Fix the race by have xprt_wait_for_buffer_space() check for write
>> space after putting the task to sleep.
>>
>> Signed-off-by: David Vrabel <[email protected]>
>> ---
>> include/linux/sunrpc/xprt.h | 1 +
>> net/sunrpc/xprt.c | 4 ++++
>> net/sunrpc/xprtsock.c | 21 +++++++++++++++++++--
>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>
>> 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 {
>> void (*connect)(struct rpc_xprt *xprt, struct rpc_task *task);
>> void * (*buf_alloc)(struct rpc_task *task, size_t size);
>> void (*buf_free)(void *buffer);
>> + bool (*have_write_space)(struct rpc_xprt *task);
>> int (*send_request)(struct rpc_task *task);
>> void (*set_retrans_timeout)(struct rpc_task *task);
>> void (*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)
>>
>> task->tk_timeout = RPC_IS_SOFT(task) ? req->rq_timeout : 0;
>> rpc_sleep_on(&xprt->pending, task, action);
>> +
>> + /* Write space notification may race with putting task to sleep. */
>> + if (xprt->ops->have_write_space(xprt))
>> + rpc_wake_up_queued_task(&xprt->pending, task);
>> }
>> EXPORT_SYMBOL_GPL(xprt_wait_for_buffer_space);
>>
>> 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)
>>
>> spin_unlock_bh(&xprt->transport_lock);
>>
>> - /* Race breaker in case memory is freed before above code is called */
>> - sk->sk_write_space(sk);
>> return ret;
>> }
>
> 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?t that fix the existing race breaker?
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.
Or did you mean SOCKWQ_ASYNC_NOSPACE here? It doesn't seem right to set
this bit when we don't know if there's space or not.
David
> On Sep 16, 2016, at 12:41, David Vrabel <[email protected]> wrote:
>=20
> On 16/09/16 17:01, Trond Myklebust wrote:
>>=20
>>> On Sep 16, 2016, at 08:28, David Vrabel <[email protected]> wrote=
:
>>>=20
>>> Write space becoming available may race with putting the task to sleep
>>> 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), b=
ut
>>> 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() (which
>>> 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 <[email protected]>
>>> ---
>>> 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 *t=
ask, 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 called=
*/
>>> -=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.
Sorry. Copy+paste error. I meant SOCKWQ_ASYNC_NOSPACE.
>=20
> Or did you mean SOCKWQ_ASYNC_NOSPACE here? It doesn't seem right to set
> this bit when we don't know if there's space or not.
Why not?
On 16/09/16 18:06, Trond Myklebust wrote:
>
>> On Sep 16, 2016, at 12:41, David Vrabel <[email protected]> wrote:
>>
>> On 16/09/16 17:01, Trond Myklebust wrote:
>>>
>>>> On Sep 16, 2016, at 08:28, David Vrabel <[email protected]> wrote:
>>>>
>>>> Write space becoming available may race with putting the task to sleep
>>>> in xprt_wait_for_buffer_space(). The existing mechanism to avoid the
>>>> race does not work.
>>>>
>>>> This (edited) partial trace illustrates the problem:
>>>>
>>>> [1] rpc_task_run_action: task:43546@5 ... action=call_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
>>>>
>>>> [1] Task 43546 runs but is out of write space.
>>>>
>>>> [2] Space becomes available, xs_write_space() clears the
>>>> SOCKWQ_ASYNC_NOSPACE bit.
>>>>
>>>> [3] xprt_write_space() attemts to wake xprt->snd_task (== 43546), but
>>>> this has not yet been queued and the wake up is lost.
>>>>
>>>> [4] xs_nospace() is called which calls xprt_wait_for_buffer_space()
>>>> which queues task 43546.
>>>>
>>>> [5] The call to sk->sk_write_space() at the end of xs_nospace() (which
>>>> 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.
>>>>
>>>> Fix the race by have xprt_wait_for_buffer_space() check for write
>>>> space after putting the task to sleep.
>>>>
>>>> Signed-off-by: David Vrabel <[email protected]>
>>>> ---
>>>> include/linux/sunrpc/xprt.h | 1 +
>>>> net/sunrpc/xprt.c | 4 ++++
>>>> net/sunrpc/xprtsock.c | 21 +++++++++++++++++++--
>>>> 3 files changed, 24 insertions(+), 2 deletions(-)
>>>>
>>>> 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 {
>>>> void (*connect)(struct rpc_xprt *xprt, struct rpc_task *task);
>>>> void * (*buf_alloc)(struct rpc_task *task, size_t size);
>>>> void (*buf_free)(void *buffer);
>>>> + bool (*have_write_space)(struct rpc_xprt *task);
>>>> int (*send_request)(struct rpc_task *task);
>>>> void (*set_retrans_timeout)(struct rpc_task *task);
>>>> void (*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)
>>>>
>>>> task->tk_timeout = RPC_IS_SOFT(task) ? req->rq_timeout : 0;
>>>> rpc_sleep_on(&xprt->pending, task, action);
>>>> +
>>>> + /* Write space notification may race with putting task to sleep. */
>>>> + if (xprt->ops->have_write_space(xprt))
>>>> + rpc_wake_up_queued_task(&xprt->pending, task);
>>>> }
>>>> EXPORT_SYMBOL_GPL(xprt_wait_for_buffer_space);
>>>>
>>>> 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)
>>>>
>>>> spin_unlock_bh(&xprt->transport_lock);
>>>>
>>>> - /* Race breaker in case memory is freed before above code is called */
>>>> - sk->sk_write_space(sk);
>>>> return ret;
>>>> }
>>>
>>> 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?t that fix the existing race breaker?
>>
>> 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.
>
> Sorry. Copy+paste error. I meant SOCKWQ_ASYNC_NOSPACE.
>
>>
>> Or did you mean SOCKWQ_ASYNC_NOSPACE here? It doesn't seem right to set
>> this bit when we don't know if there's space or not.
>
> Why not?
I prefer my solution because:
a) It obviously fixes the race (games with bits are less understandable).
b) It requires fewer atomic ops.
c) It doesn't require me to understand what the behaviour of the
socket-internal SOCKWQ_ASYNC_NOSPACE bit is or should be.
d) I'm not sure I understand the objection to the additional
have_write_space method -- it has simple, clear behaviour.
David
> On Sep 16, 2016, at 13:29, David Vrabel <[email protected]> wrote:
>=20
> On 16/09/16 18:06, Trond Myklebust wrote:
>>=20
>>> On Sep 16, 2016, at 12:41, David Vrabel <[email protected]> wrote=
:
>>>=20
>>> On 16/09/16 17:01, Trond Myklebust wrote:
>>>>=20
>>>>> On Sep 16, 2016, at 08:28, David Vrabel <[email protected]> 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 <[email protected]>
>>>>> ---
>>>>> 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.