Re: [PATCH 1/4] SUNRPC: Ensure rq_bytes_sent is reset before request transmission

[Trond Myklebust <trondmy@hammerspace.com>]

On Thu, 2019-01-03 at 11:17 -0500, Tom Talpey wrote:
> On 1/3/2019 11:05 AM, Trond Myklebust wrote:
> > On Thu, 2019-01-03 at 10:29 -0500, Chuck Lever wrote:
> > > Hi Trond-
> > > 
> > > I was curious about this one because yesterday I saw evidence
> > > (for
> > > other reasons) that rq_bytes_sent wasn't always zeroed when it
> > > should
> > > be.
> > > 
> > > 
> > > > On Jan 2, 2019, at 5:53 PM, Trond Myklebust <trondmy@gmail.com>
> > > > wrote:
> > > > 
> > > > When we resend a request, ensure that the 'rq_bytes_sent' is
> > > > reset
> > > > to zero.
> > > > 
> > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com
> > > > >
> > > > ---
> > > > net/sunrpc/clnt.c | 1 -
> > > > net/sunrpc/xprt.c | 1 +
> > > > 2 files changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
> > > > index 24cbddc44c88..2189fbc4c570 100644
> > > > --- a/net/sunrpc/clnt.c
> > > > +++ b/net/sunrpc/clnt.c
> > > > @@ -1738,7 +1738,6 @@ rpc_xdr_encode(struct rpc_task *task)
> > > > 	xdr_buf_init(&req->rq_rcv_buf,
> > > > 		     req->rq_rbuffer,
> > > > 		     req->rq_rcvsize);
> > > > -	req->rq_bytes_sent = 0;
> > > 
> > > I agree this line is not sufficient, and it should be moved.
> > > Not every retransmission requires a re-encode. However, the
> > > patch description should explain that, and it probably needs
> > > a Fixes: tag.
> > > 
> > > Can you now also remove the same line from xprt_request_init
> > > and xprt_init_bc_request ?
> > > 
> > > Also, I notice that UDP does not touch rq_bytes_sent. Since
> > > RDMA also does not use rq_bytes_sent, maybe the same line
> > > can be removed from xprtrdma/transport.c and
> > > xprtrdma/backchannel.c ?
> > 
> > Sure.
> > 
> > So please note that rq_bytes_sent == 0 no longer means "this
> > request
> > needs to be retransmitted" and we no longer test for it in
> > net/sunrpc/clnt.c. We do still have a couple of tests of
> > rq_bytes_sent
> > in net/sunrpc/xprt.c and net/sunrpc/xprtsock.c, but those are more
> > about checking if a transmission of that request is currently in
> > progress, in which case we don't want to queue anything in front of
> > it
> > on the transmission queue, and we don't want to abort the
> > transmission
> > unless we also close the socket.
> 
> I think rq_bytes_sent is all about managing sends atomically. On
> stream
> transports (which allow buffering partial segments), it would be
> fatal 
> to allow intermingling. On datagram transports, it's a non-issue
> since
> no sends are ever partial.
> 
> IOW, couldn't rq_bytes_sent simply be a boolean?

Sends can be partial for TCP and AF_LOCAL because the stream socket
operations are non-blocking.

Strictly speaking, though, we probably could replace rq_bytes_sent with
a boolean that represents "transmission in progress" since the TCP
layer itself now tracks how many bytes have been transmitted for the
request being transmitted. i.e. the boolean would be set by
xs_local_send_request() and xs_tcp_send_request(), and cleared by those
same functions once the transmission is complete. Meh...


> Tom.
> 
> > The intention now is that if we know the request needs
> > retransmission
> > (due to a transport connection loss or a timeout), then we just add
> > it
> > to the transmission queue.
> > 
> > 
> > > > 	p = rpc_encode_header(task);
> > > > 	if (p == NULL) {
> > > > diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
> > > > index 73547d17d3c6..9075ae150ae5 100644
> > > > --- a/net/sunrpc/xprt.c
> > > > +++ b/net/sunrpc/xprt.c
> > > > @@ -1151,6 +1151,7 @@ xprt_request_enqueue_transmit(struct
> > > > rpc_task
> > > > *task)
> > > > 	struct rpc_xprt *xprt = req->rq_xprt;
> > > > 
> > > > 	if (xprt_request_need_enqueue_transmit(task, req)) {
> > > > +		req->rq_bytes_sent = 0;
> > > > 		spin_lock(&xprt->queue_lock);
> > > > 		/*
> > > > 		 * Requests that carry congestion control
> > > > credits are
> > > > added
> > > 
> > > So I'm not convinced this covers every case. I need some
> > > time to investigate.
> > 
> > It should normally cover all cases. As I said, the only remaining
> > tests
> > are in xprt.c and  xprtsock.c
> > 
-- 
Trond Myklebust
CTO, Hammerspace Inc
4300 El Camino Real, Suite 105
Los Altos, CA 94022
www.hammer.space