On Tue, Nov 07, 2017 at 08:46:14AM -0500, Jeff Layton wrote:
> On Fri, 2017-06-30 at 14:23 +0100, Stefan Hajnoczi wrote:
> > +/**
> > + * xs_vsock_error_report - callback to handle vsock socket state errors
> > + * @sk: socket
> > + *
> > + * Note: we don't call sock_error() since there may be a rpc_task
> > + * using the socket, and so we don't want to clear sk->sk_err.
> > + */
> > +static void xs_vsock_error_report(struct sock *sk)
> > +{
> > + struct rpc_xprt *xprt;
> > + int err;
> > +
> > + read_lock_bh(&sk->sk_callback_lock);
> > + if (!(xprt = xprt_from_sock(sk)))
> > + goto out;
> > +
> > + err = -sk->sk_err;
> > + if (err == 0)
> > + goto out;
> > + /* Is this a reset event? */
> > + if (sk->sk_state == SS_UNCONNECTED)
> > + xs_sock_mark_closed(xprt);
> > + dprintk("RPC: %s client %p, error=%d...\n",
> > + __func__, xprt, -err);
> > + trace_rpc_socket_error(xprt, sk->sk_socket, err);
> > + xprt_wake_pending_tasks(xprt, err);
> > + out:
> > + read_unlock_bh(&sk->sk_callback_lock);
> > +}
>
> Hmm ok...so we have this to avoid some TCP specific stuff in
> xs_error_report, I guess?
>
> I wonder if AF_LOCAL transport should be using the function above,
> rather than xs_error_report? If so, maybe we should rename:
>
> xs_error_report -> xs_tcp_error_report
> xs_vsock_error_report -> xs_stream_error_report
>
> Might be good to do that cleanup first as a preparatory patch.
AF_LOCAL's use of xs_error_report() is fine because AF_UNIX uses TCP_*
constants for sk->sk_state.
VSOCK has now switched to TCP_* sk_state constants in Dave Miller's
net-next tree. I made that change for unrelated reasons, but it means
v4 of this patch series will share the xs_error_report() function with
TCP and AF_LOCAL.
> > +
> > +/**
> > + * xs_vsock_finish_connecting - initialize and connect socket
> > + */
> > +static int xs_vsock_finish_connecting(struct rpc_xprt *xprt, struct socket *sock)
> > +{
> > + struct sock_xprt *transport = container_of(xprt, struct sock_xprt, xprt);
> > + int ret = -ENOTCONN;
> > +
> > + if (!transport->inet) {
> > + struct sock *sk = sock->sk;
> > +
> > + write_lock_bh(&sk->sk_callback_lock);
> > +
> > + xs_save_old_callbacks(transport, sk);
> > +
> > + sk->sk_user_data = xprt;
> > + sk->sk_data_ready = xs_data_ready;
> > + sk->sk_state_change = xs_vsock_state_change;
> > + sk->sk_write_space = xs_tcp_write_space;
>
> Might should rename xs_tcp_write_space to xs_stream_write_space?
Yes, will fix in v4.
> > + sk->sk_error_report = xs_vsock_error_report;
> > + sk->sk_allocation = GFP_ATOMIC;
>
> Why GFP_ATOMIC here? The other finish routines use GFP_NOIO.
I missed the memo on commit c2126157ea3c4f72b315749e0c07a1b162a2fe2b
("SUNRPC: Allow sockets to do GFP_NOIO allocations"). It is now okay to
use GFP_NOIO.
Thank you, will fix in v4.
Attachment:
signature.asc
Description: PGP signature
