> On Jan 3, 2019, at 11:05 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> 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@xxxxxxxxx> >>> wrote: >>> >>> When we resend a request, ensure that the 'rq_bytes_sent' is reset >>> to zero. >>> >>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> >>> --- >>> 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 In the patch I have that removes xprt::tsh_size, I'm using rq_bytes_sent to figure out when to insert a record marker. Every once in a while, it sticks in a record marker where it shouldn't. -- Chuck Lever