| Message ID | 20190102225313.2097-2-trond.myklebust@hammerspace.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bugfixes for RPCSEC_GSS client support | expand |
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 ? > 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. -- Chuck Lever
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
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? 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 >
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 > >
> On Jan 3, 2019, at 11:17 AM, Tom Talpey <tom@talpey.com> 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? I read somewhere recently that a boolean would take up as much space as a u32 in rpc_rqst. Not sure it saves much. I would be interested in removing rq_bytes_sent from generic paths, as a minor optimization. It seems to be something that stream transports need, but the others don't. > 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 -- Chuck Lever
> On Jan 3, 2019, at 11:05 AM, Trond Myklebust <trondmy@hammerspace.com> 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. > > 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
On Thu, 2019-01-03 at 11:41 -0500, Chuck Lever wrote: > > On Jan 3, 2019, at 11:05 AM, Trond Myklebust < > > trondmy@hammerspace.com> 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. > > > > 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. I'm not sure I understand. req->rq_bytes_sent is expected to be zero when we call rpc_encode_header(), which is where we call xprt_skip_transport_header(). If you want to remove tsh_size, then I would suggest replacing xprt_skip_transport_header() with an operation in struct rpc_xprt_ops that returns a pointer to the first word in the send buffer that is not reserved for transport use.
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; 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
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(-)