Fix svc_ioq_flushv send more msg #294
Conversation
|
the more interesting thing is, debian client can not reproduce this client hang, but we can reproduce this on ubuntu client. |
| @@ -257,6 +257,7 @@ svc_ioq_flushv(SVCXPRT *xprt, struct xdr_ioq *xioq) | |||
| xioq->frag_hdr_bytes_sent += result; | |||
| iov[0].iov_base += result; | |||
| iov[0].iov_len -= result; | |||
| frag_hdr_size -= result; | |||
There was a problem hiding this comment.
Is this going to cause a problem down below when frag_hdr_size is subtracted from result?
There was a problem hiding this comment.
Good point. Maybe, instead, use
if (result < (frag_hdr_size - xioq->frag_hdr_bytes_sent))
There was a problem hiding this comment.
Is this going to cause a problem down below when frag_hdr_size is subtracted from result?
sorry, I checked the code, but did not find this will cause a problem. actually, the variable related to frag_hdr_size is remaining, fbytes and xioq->write_start, all need frag_hdr_size is subtracted from result.
There was a problem hiding this comment.
Good point. Maybe, instead, use
if (result < (frag_hdr_size - xioq->frag_hdr_bytes_sent))
if we only use if (result < (frag_hdr_size - xioq->frag_hdr_bytes_sent)) to replace if (result < frag_hdr_size). but without the change of this pull request, xioq->write_start will be wrong and the problem still exist.
There was a problem hiding this comment.
But if the whole frag header isn't sent, we goto again, to try and send the rest of it.
Below, when frag_hdr_size is used, it's after all the attempts to send, so we would only subtract the reduced frag_hdr_size from remaining and fbytes, and add to write_start. But since we've sent the whole frag header, we should change all of those by the original frag_hdr_size
There was a problem hiding this comment.
@ffilz thanks the reply, remaining and fbytes always record the left body length, it should not subtract the frag header. remaining come from remaining = end - xioq->write_start. in the second time we send 5bytes(result=5) including the left 2bytes frag header and 3bytes body, then remaining should subtract 3 bytes, but currently remaining only subtract 1 bytes, this cause send more msg.
For example, the data is ABCDEF, the second send 2bytes header and ABC, remaining should subtract 3, but it subtract 1 which means it only send A, then it will send BCDEF, which cause BC send two times.
There was a problem hiding this comment.
OK, I see it now. Can we have some comments talking about what's going on here because it is really confusing as evidenced by my confusion throughout this discussion. Maybe an example showing the two packets, one with part of the fragment header, and the other with the rest of the fragment header and some portion of the fragment.
Other than that, I approve of this change now that I understand it.
There was a problem hiding this comment.
sure, already add some comments.
| @@ -257,6 +257,7 @@ svc_ioq_flushv(SVCXPRT *xprt, struct xdr_ioq *xioq) | |||
| xioq->frag_hdr_bytes_sent += result; | |||
| iov[0].iov_base += result; | |||
| iov[0].iov_len -= result; | |||
| frag_hdr_size -= result; | |||
|
This probably rates a pullup in the next -dev |
copy from nfs-ganesha/nfs-ganesha#1115
@ffilz @dang we met nfs client hang for rpc length and body mismatch.
Reason
let's see the function svc_ioq_flushv in ntirpc.
step1
svc_ioq_flushv process an xioq with xioq->write_start=0, and end=10, i.e. there exist 10 bytes to send
step2
first into while loop with remaining=10, because xioq->write_start is 0, so frag_hdr_size is 4 bytes, and we need 4 bytes header to save the length of 10 bytes, so need to send 14bytes = 4bytes(header) + 10bytes(body).
step3
sendmsg send 2 bytes of header, and result=2 which less than frag_hdr_size=4bytes, and goto again
step4
in again, sendmsg send 5bytes(result=5) including 2bytes header and 3bytes body, then result-= frag_hdr_size, so result=1, then remaining-=result, so remaining is 9bytes, and xioq->write_start+=result, so xioq->write_start=1. but actually remaining should be 7 and xioq->write_start should be 3, because already sent [0, 3)bytes body.
step5
twice into while loop with remaining=9, and xioq->write_start=1, so send [1, 3)bytes body again. i.e. we send [1, 3)bytes body twice times in step4 and step5. and error happens.