| Message ID | 20230321133312.103789-1-linkinjeon@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ksmbd: don't terminate inactive sessions after a few seconds | expand |
On (23/03/21 22:33), Namjae Jeon wrote: [..] > @@ -335,14 +336,23 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, > } else if (conn->status == KSMBD_SESS_NEED_RECONNECT) { > total_read = -EAGAIN; > break; > - } else if ((length == -ERESTARTSYS || length == -EAGAIN) && > - max_retry) { > + } else if (length == -ERESTARTSYS || length == -EAGAIN) { > + /* > + * If max_retries is negative, Allow unlimited > + * retries to keep connection with inactive sessions. > + */ > + if (max_retries == 0) { > + total_read = length; > + break; > + } else if (max_retries > 0) { > + max_retries--; > + } > + > usleep_range(1000, 2000); > length = 0; > - max_retry--; > continue; > } else if (length <= 0) { > - total_read = -EAGAIN; > + total_read = length; > break; > } > } By the way, ksmbd_tcp_readv() calls kvec_array_init() on each iteration. Shouldn't we call it only if length > 0? That is only if the most recent call to kernel_recvmsg() has read some data.
2023-03-24 12:50 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > On (23/03/21 22:33), Namjae Jeon wrote: > [..] >> @@ -335,14 +336,23 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, >> struct kvec *iov_orig, >> } else if (conn->status == KSMBD_SESS_NEED_RECONNECT) { >> total_read = -EAGAIN; >> break; >> - } else if ((length == -ERESTARTSYS || length == -EAGAIN) && >> - max_retry) { >> + } else if (length == -ERESTARTSYS || length == -EAGAIN) { >> + /* >> + * If max_retries is negative, Allow unlimited >> + * retries to keep connection with inactive sessions. >> + */ >> + if (max_retries == 0) { >> + total_read = length; >> + break; >> + } else if (max_retries > 0) { >> + max_retries--; >> + } >> + >> usleep_range(1000, 2000); >> length = 0; >> - max_retry--; >> continue; >> } else if (length <= 0) { >> - total_read = -EAGAIN; >> + total_read = length; >> break; >> } >> } > > By the way, ksmbd_tcp_readv() calls kvec_array_init() on each iteration. > Shouldn't we call it only if length > 0? That is only if the most recent > call to kernel_recvmsg() has read some data. If length == to_read is equal then it is not called. And in case length < to_read, we have to call it which reinitialize io vec again for reading the rest of the data. >
On (23/03/21 22:33), Namjae Jeon wrote: > Steve reported that inactive sessions are terminated after a few > seconds. ksmbd terminate when receiving -EAGAIN error from > kernel_recvmsg(). -EAGAIN means there is no data available in timeout. > So ksmbd should keep connection with unlimited retries instead of > terminating inactive sessions. > > Reported-by: Steve French <stfrench@microsoft.com> > Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
On (23/03/24 13:28), Namjae Jeon wrote: > > By the way, ksmbd_tcp_readv() calls kvec_array_init() on each iteration. > > Shouldn't we call it only if length > 0? That is only if the most recent > > call to kernel_recvmsg() has read some data. > If length == to_read is equal then it is not called. And in case > length < to_read, we have to call it which reinitialize io vec again > for reading the rest of the data. What I'm saying is: if length == 0 on the previous iteration then we don't need to kvec_array_init(). But maybe I'm missing something.
2023-03-24 14:45 GMT+09:00, Sergey Senozhatsky <senozhatsky@chromium.org>: > On (23/03/24 13:28), Namjae Jeon wrote: >> > By the way, ksmbd_tcp_readv() calls kvec_array_init() on each >> > iteration. >> > Shouldn't we call it only if length > 0? That is only if the most >> > recent >> > call to kernel_recvmsg() has read some data. >> If length == to_read is equal then it is not called. And in case >> length < to_read, we have to call it which reinitialize io vec again >> for reading the rest of the data. > > What I'm saying is: if length == 0 on the previous iteration then > we don't need to kvec_array_init(). But maybe I'm missing something. You're right. We can improve it with another patch. Thanks for your review! >
diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c index 5b10b03800c1..5d914715605f 100644 --- a/fs/ksmbd/connection.c +++ b/fs/ksmbd/connection.c @@ -298,7 +298,7 @@ int ksmbd_conn_handler_loop(void *p) kvfree(conn->request_buf); conn->request_buf = NULL; - size = t->ops->read(t, hdr_buf, sizeof(hdr_buf)); + size = t->ops->read(t, hdr_buf, sizeof(hdr_buf), -1); if (size != sizeof(hdr_buf)) break; @@ -344,7 +344,7 @@ int ksmbd_conn_handler_loop(void *p) * We already read 4 bytes to find out PDU size, now * read in PDU */ - size = t->ops->read(t, conn->request_buf + 4, pdu_size); + size = t->ops->read(t, conn->request_buf + 4, pdu_size, 2); if (size < 0) { pr_err("sock_read failed: %d\n", size); break; diff --git a/fs/ksmbd/connection.h b/fs/ksmbd/connection.h index 3643354a3fa7..0e3a848defaf 100644 --- a/fs/ksmbd/connection.h +++ b/fs/ksmbd/connection.h @@ -114,7 +114,8 @@ struct ksmbd_transport_ops { int (*prepare)(struct ksmbd_transport *t); void (*disconnect)(struct ksmbd_transport *t); void (*shutdown)(struct ksmbd_transport *t); - int (*read)(struct ksmbd_transport *t, char *buf, unsigned int size); + int (*read)(struct ksmbd_transport *t, char *buf, + unsigned int size, int max_retries); int (*writev)(struct ksmbd_transport *t, struct kvec *iovs, int niov, int size, bool need_invalidate_rkey, unsigned int remote_key); diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c index 096eda9ef873..c06efc020bd9 100644 --- a/fs/ksmbd/transport_rdma.c +++ b/fs/ksmbd/transport_rdma.c @@ -670,7 +670,7 @@ static int smb_direct_post_recv(struct smb_direct_transport *t, } static int smb_direct_read(struct ksmbd_transport *t, char *buf, - unsigned int size) + unsigned int size, int unused) { struct smb_direct_recvmsg *recvmsg; struct smb_direct_data_transfer *data_transfer; diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c index 603893fd87f5..20e85e2701f2 100644 --- a/fs/ksmbd/transport_tcp.c +++ b/fs/ksmbd/transport_tcp.c @@ -291,16 +291,18 @@ static int ksmbd_tcp_run_kthread(struct interface *iface) /** * ksmbd_tcp_readv() - read data from socket in given iovec - * @t: TCP transport instance - * @iov_orig: base IO vector - * @nr_segs: number of segments in base iov - * @to_read: number of bytes to read from socket + * @t: TCP transport instance + * @iov_orig: base IO vector + * @nr_segs: number of segments in base iov + * @to_read: number of bytes to read from socket + * @max_retries: maximum retry count * * Return: on success return number of bytes read from socket, * otherwise return error number */ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, - unsigned int nr_segs, unsigned int to_read) + unsigned int nr_segs, unsigned int to_read, + int max_retries) { int length = 0; int total_read; @@ -308,7 +310,6 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, struct msghdr ksmbd_msg; struct kvec *iov; struct ksmbd_conn *conn = KSMBD_TRANS(t)->conn; - int max_retry = 2; iov = get_conn_iovec(t, nr_segs); if (!iov) @@ -335,14 +336,23 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, } else if (conn->status == KSMBD_SESS_NEED_RECONNECT) { total_read = -EAGAIN; break; - } else if ((length == -ERESTARTSYS || length == -EAGAIN) && - max_retry) { + } else if (length == -ERESTARTSYS || length == -EAGAIN) { + /* + * If max_retries is negative, Allow unlimited + * retries to keep connection with inactive sessions. + */ + if (max_retries == 0) { + total_read = length; + break; + } else if (max_retries > 0) { + max_retries--; + } + usleep_range(1000, 2000); length = 0; - max_retry--; continue; } else if (length <= 0) { - total_read = -EAGAIN; + total_read = length; break; } } @@ -358,14 +368,15 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, * Return: on success return number of bytes read from socket, * otherwise return error number */ -static int ksmbd_tcp_read(struct ksmbd_transport *t, char *buf, unsigned int to_read) +static int ksmbd_tcp_read(struct ksmbd_transport *t, char *buf, + unsigned int to_read, int max_retries) { struct kvec iov; iov.iov_base = buf; iov.iov_len = to_read; - return ksmbd_tcp_readv(TCP_TRANS(t), &iov, 1, to_read); + return ksmbd_tcp_readv(TCP_TRANS(t), &iov, 1, to_read, max_retries); } static int ksmbd_tcp_writev(struct ksmbd_transport *t, struct kvec *iov,
Steve reported that inactive sessions are terminated after a few seconds. ksmbd terminate when receiving -EAGAIN error from kernel_recvmsg(). -EAGAIN means there is no data available in timeout. So ksmbd should keep connection with unlimited retries instead of terminating inactive sessions. Reported-by: Steve French <stfrench@microsoft.com> Signed-off-by: Namjae Jeon <linkinjeon@kernel.org> --- fs/ksmbd/connection.c | 4 ++-- fs/ksmbd/connection.h | 3 ++- fs/ksmbd/transport_rdma.c | 2 +- fs/ksmbd/transport_tcp.c | 35 +++++++++++++++++++++++------------ 4 files changed, 28 insertions(+), 16 deletions(-)