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