> On Sep 5, 2018, at 11:28 AM, Trond Myklebust <trondmy@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, 2018-09-05 at 10:30 -0400, Chuck Lever wrote:
>>> On Sep 4, 2018, at 5:05 PM, Trond Myklebust <trondmy@xxxxxxxxx>
>>> wrote:
>>>
>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>> ---
>>> net/sunrpc/clnt.c | 6 ------
>>> net/sunrpc/xprt.c | 13 ++++++-------
>>> 2 files changed, 6 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
>>> index 032b7042adb6..21713bed812a 100644
>>> --- a/net/sunrpc/clnt.c
>>> +++ b/net/sunrpc/clnt.c
>>> @@ -1967,8 +1967,6 @@ call_connect_status(struct rpc_task *task)
>>> static void
>>> call_transmit(struct rpc_task *task)
>>> {
>>> - int is_retrans = RPC_WAS_SENT(task);
>>> -
>>> dprint_status(task);
>>>
>>> task->tk_action = call_transmit_status;
>>> @@ -1979,10 +1977,6 @@ call_transmit(struct rpc_task *task)
>>> if (!xprt_prepare_transmit(task))
>>> return;
>>> xprt_transmit(task);
>>> - if (task->tk_status < 0)
>>> - return;
>>> - if (is_retrans)
>>> - task->tk_client->cl_stats->rpcretrans++;
>>> }
>>>
>>> /*
>>> diff --git a/net/sunrpc/xprt.c b/net/sunrpc/xprt.c
>>> index f1301d391399..e2f5b4668a66 100644
>>> --- a/net/sunrpc/xprt.c
>>> +++ b/net/sunrpc/xprt.c
>>> @@ -191,8 +191,6 @@ int xprt_reserve_xprt(struct rpc_xprt *xprt,
>>> struct rpc_task *task)
>>> goto out_sleep;
>>> }
>>> xprt->snd_task = task;
>>> - if (req != NULL)
>>> - req->rq_ntrans++;
>>>
>>> return 1;
>>>
>>> @@ -247,7 +245,6 @@ int xprt_reserve_xprt_cong(struct rpc_xprt
>>> *xprt, struct rpc_task *task)
>>> }
>>> if (__xprt_get_cong(xprt, task)) {
>>> xprt->snd_task = task;
>>> - req->rq_ntrans++;
>>> return 1;
>>> }
>>> xprt_clear_locked(xprt);
>>> @@ -281,12 +278,8 @@ static inline int xprt_lock_write(struct
>>> rpc_xprt *xprt, struct rpc_task *task)
>>> static bool __xprt_lock_write_func(struct rpc_task *task, void
>>> *data)
>>> {
>>> struct rpc_xprt *xprt = data;
>>> - struct rpc_rqst *req;
>>>
>>> - req = task->tk_rqstp;
>>> xprt->snd_task = task;
>>> - if (req)
>>> - req->rq_ntrans++;
>>> return true;
>>> }
>>>
>>> @@ -1126,6 +1119,7 @@ void xprt_transmit(struct rpc_task *task)
>>> struct rpc_rqst *req = task->tk_rqstp;
>>> struct rpc_xprt *xprt = req->rq_xprt;
>>> unsigned int connect_cookie;
>>> + int is_retrans = RPC_WAS_SENT(task);
>>> int status;
>>>
>>> dprintk("RPC: %5u xprt_transmit(%u)\n", task->tk_pid, req-
>>>> rq_slen);
>>> @@ -1140,6 +1134,8 @@ void xprt_transmit(struct rpc_task *task)
>>> }
>>> }
>>>
>>> + req->rq_ntrans++;
>>> +
>>
>> rq_ntrans is used in two places:
>>
>> 1. rpc_update_rtt
>> 2. rpc_count_iostats_metrics
>>
>> Both of these appear to assume that rq_ntrans is counting
>> full successful transmissions (ie, calls that appear on the
>> wire), not invocations of ->send_request().
>>
>> Can this counter be moved down to where rpcretrans is updated?
>>
>
> The reason why I don't want to update req->rq_ntrans after transmission
> is because that would race with the reply from the server (which is not
> blocked by the XPRT_LOCK). In that race scenario, rpc_update_rtt()
> could update the RTT value before we've updated the req->rq_ntrans,
> meaning that we might be measuring the response time against a reply to
> the original transmission or to the retransmission that was just sent.
> That would end up breaking Van Jacobsen congestion control, since we
> risk significantly underestimating the RTT value.
But for transports that don't use xprt_update_rtt, error
returns from ->send_request() (like -EAGAIN or -ENOBUFS)
means that rq_ntrans is counted twice, and it shows up as
false retransmissions.
What if rq_ntrans was bumped in the transport code instead?
--
Chuck Lever
