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 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.

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
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com