Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.2 \(3445.102.3\))
Subject: Re: [PATCH] SUNRPC: Use poll() to fix up the socket requeue races
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20190219140616.123141-1-trond.myklebust@hammerspace.com>
Date:   Tue, 19 Feb 2019 09:54:55 -0500
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <A2BBC175-F60D-4C61-B70F-479952D2CD21@oracle.com>
References: <20190219140616.123141-1-trond.myklebust@hammerspace.com>
To:     Trond Myklebust <trondmy@gmail.com>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk

Hi Trond-

> On Feb 19, 2019, at 9:06 AM, Trond Myklebust <trondmy@gmail.com> =
wrote:
>=20
> Because we clear XPRT_SOCK_DATA_READY before reading, we can end up
> with a situation where new data arrives, causing xs_data_ready() to
> queue up a second receive worker job for the same socket, which then
> immediately gets stuck waiting on the transport receive mutex.
> The fix is to only clear XPRT_SOCK_DATA_READY once we're done reading,
> and then to use poll() to check if we might need to queue up a new
> job in order to deal with any new data.

Does this fix an application-visible hang, or is it merely a performance
optimization?


> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
> net/sunrpc/xprtsock.c | 17 +++++++++++++++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>=20
> diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
> index f5d7dcd9e8d9..a721c843d5d3 100644
> --- a/net/sunrpc/xprtsock.c
> +++ b/net/sunrpc/xprtsock.c
> @@ -656,13 +656,25 @@ xs_read_stream(struct sock_xprt *transport, int =
flags)
> 	return ret !=3D 0 ? ret : -ESHUTDOWN;
> }
>=20
> +static void xs_poll_check_readable(struct sock_xprt *transport)
> +{
> +	struct socket *sock =3D transport->sock;
> +	__poll_t events;
> +
> +	clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state);
> +	events =3D sock->ops->poll(NULL, sock, NULL);
> +	if (!(events & (EPOLLIN | EPOLLRDNORM)) || events & EPOLLRDHUP)
> +		return;
> +	if (!test_and_set_bit(XPRT_SOCK_DATA_READY, =
&transport->sock_state))
> +		queue_work(xprtiod_workqueue, &transport->recv_worker);
> +}
> +
> static void xs_stream_data_receive(struct sock_xprt *transport)
> {
> 	size_t read =3D 0;
> 	ssize_t ret =3D 0;
>=20
> 	mutex_lock(&transport->recv_mutex);
> -	clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state);
> 	if (transport->sock =3D=3D NULL)
> 		goto out;
> 	for (;;) {
> @@ -672,6 +684,7 @@ static void xs_stream_data_receive(struct =
sock_xprt *transport)
> 		read +=3D ret;
> 		cond_resched();
> 	}
> +	xs_poll_check_readable(transport);
> out:
> 	mutex_unlock(&transport->recv_mutex);
> 	trace_xs_stream_read_data(&transport->xprt, ret, read);
> @@ -1362,7 +1375,6 @@ static void xs_udp_data_receive(struct sock_xprt =
*transport)
> 	int err;
>=20
> 	mutex_lock(&transport->recv_mutex);
> -	clear_bit(XPRT_SOCK_DATA_READY, &transport->sock_state);
> 	sk =3D transport->inet;
> 	if (sk =3D=3D NULL)
> 		goto out;
> @@ -1374,6 +1386,7 @@ static void xs_udp_data_receive(struct sock_xprt =
*transport)
> 		consume_skb(skb);
> 		cond_resched();
> 	}
> +	xs_poll_check_readable(transport);
> out:
> 	mutex_unlock(&transport->recv_mutex);
> }
> --=20
> 2.20.1
>=20

--
Chuck Lever