* [PATCH] ksmbd: smbd: call rdma_accept() under CM handler
@ 2022-01-03 0:02 Hyunchul Lee
2022-01-04 0:45 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Hyunchul Lee @ 2022-01-03 0:02 UTC (permalink / raw)
To: linux-cifs; +Cc: Namjae Jeon, Sergey Senozhatsky, Steve French, Hyunchul Lee
if CONFIG_LOCKDEP is enabled, the following
kernel warning message is generated because
rdma_accept() is called not under CM(Connection
Manager) handler.
[ 63.211405 ] WARNING: CPU: 1 PID: 345 at drivers/infiniband/core/cma.c:4405 rdma_accept+0x17a/0x350
[ 63.212080 ] RIP: 0010:rdma_accept+0x17a/0x350
...
[ 63.214036 ] Call Trace:
[ 63.214098 ] <TASK>
[ 63.214185 ] smb_direct_accept_client+0xb4/0x170 [ksmbd]
[ 63.214412 ] smb_direct_prepare+0x322/0x8c0 [ksmbd]
[ 63.214555 ] ? rcu_read_lock_sched_held+0x3a/0x70
[ 63.214700 ] ksmbd_conn_handler_loop+0x63/0x270 [ksmbd]
[ 63.214826 ] ? ksmbd_conn_alive+0x80/0x80 [ksmbd]
[ 63.214952 ] kthread+0x171/0x1a0
[ 63.215039 ] ? set_kthread_struct+0x40/0x40
[ 63.215128 ] ret_from_fork+0x22/0x30
To avoid this, move creating a queue pair and accepting
a client from transport_ops->prepare() to
smb_direct_handle_connect_request().
Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
fs/ksmbd/transport_rdma.c | 97 +++++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 40 deletions(-)
diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
index f89b64e27836..b37e4b9580ae 100644
--- a/fs/ksmbd/transport_rdma.c
+++ b/fs/ksmbd/transport_rdma.c
@@ -568,6 +568,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc *wc)
}
t->negotiation_requested = true;
t->full_packet_received = true;
+ enqueue_reassembly(t, recvmsg, 0);
wake_up_interruptible(&t->wait_status);
break;
case SMB_DIRECT_MSG_DATA_TRANSFER: {
@@ -1594,19 +1595,13 @@ static int smb_direct_accept_client(struct smb_direct_transport *t)
pr_err("error at rdma_accept: %d\n", ret);
return ret;
}
-
- wait_event_interruptible(t->wait_status,
- t->status != SMB_DIRECT_CS_NEW);
- if (t->status != SMB_DIRECT_CS_CONNECTED)
- return -ENOTCONN;
return 0;
}
-static int smb_direct_negotiate(struct smb_direct_transport *t)
+static int smb_direct_prepare_negotiation(struct smb_direct_transport *t)
{
int ret;
struct smb_direct_recvmsg *recvmsg;
- struct smb_direct_negotiate_req *req;
recvmsg = get_free_recvmsg(t);
if (!recvmsg)
@@ -1616,44 +1611,20 @@ static int smb_direct_negotiate(struct smb_direct_transport *t)
ret = smb_direct_post_recv(t, recvmsg);
if (ret) {
pr_err("Can't post recv: %d\n", ret);
- goto out;
+ goto out_err;
}
t->negotiation_requested = false;
ret = smb_direct_accept_client(t);
if (ret) {
pr_err("Can't accept client\n");
- goto out;
+ goto out_err;
}
smb_direct_post_recv_credits(&t->post_recv_credits_work.work);
-
- ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
- ret = wait_event_interruptible_timeout(t->wait_status,
- t->negotiation_requested ||
- t->status == SMB_DIRECT_CS_DISCONNECTED,
- SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
- if (ret <= 0 || t->status == SMB_DIRECT_CS_DISCONNECTED) {
- ret = ret < 0 ? ret : -ETIMEDOUT;
- goto out;
- }
-
- ret = smb_direct_check_recvmsg(recvmsg);
- if (ret == -ECONNABORTED)
- goto out;
-
- req = (struct smb_direct_negotiate_req *)recvmsg->packet;
- t->max_recv_size = min_t(int, t->max_recv_size,
- le32_to_cpu(req->preferred_send_size));
- t->max_send_size = min_t(int, t->max_send_size,
- le32_to_cpu(req->max_receive_size));
- t->max_fragmented_send_size =
- le32_to_cpu(req->max_fragmented_size);
-
- ret = smb_direct_send_negotiate_response(t, ret);
-out:
- if (recvmsg)
- put_recvmsg(t, recvmsg);
+ return 0;
+out_err:
+ put_recvmsg(t, recvmsg);
return ret;
}
@@ -1890,6 +1861,47 @@ static int smb_direct_create_qpair(struct smb_direct_transport *t,
static int smb_direct_prepare(struct ksmbd_transport *t)
{
struct smb_direct_transport *st = smb_trans_direct_transfort(t);
+ struct smb_direct_recvmsg *recvmsg;
+ struct smb_direct_negotiate_req *req;
+ int ret;
+
+ ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
+ ret = wait_event_interruptible_timeout(st->wait_status,
+ st->negotiation_requested ||
+ st->status == SMB_DIRECT_CS_DISCONNECTED,
+ SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
+ if (ret <= 0 || st->status == SMB_DIRECT_CS_DISCONNECTED)
+ return ret < 0 ? ret : -ETIMEDOUT;
+
+ recvmsg = get_first_reassembly(st);
+ if (!recvmsg)
+ return -ECONNABORTED;
+
+ ret = smb_direct_check_recvmsg(recvmsg);
+ if (ret == -ECONNABORTED)
+ goto out;
+
+ req = (struct smb_direct_negotiate_req *)recvmsg->packet;
+ st->max_recv_size = min_t(int, st->max_recv_size,
+ le32_to_cpu(req->preferred_send_size));
+ st->max_send_size = min_t(int, st->max_send_size,
+ le32_to_cpu(req->max_receive_size));
+ st->max_fragmented_send_size =
+ le32_to_cpu(req->max_fragmented_size);
+
+ ret = smb_direct_send_negotiate_response(st, ret);
+out:
+ spin_lock_irq(&st->reassembly_queue_lock);
+ st->reassembly_queue_length--;
+ list_del(&recvmsg->list);
+ spin_unlock_irq(&st->reassembly_queue_lock);
+ put_recvmsg(st, recvmsg);
+
+ return ret;
+}
+
+static int smb_direct_connect(struct smb_direct_transport *st)
+{
int ret;
struct ib_qp_cap qp_cap;
@@ -1911,13 +1923,11 @@ static int smb_direct_prepare(struct ksmbd_transport *t)
return ret;
}
- ret = smb_direct_negotiate(st);
+ ret = smb_direct_prepare_negotiation(st);
if (ret) {
pr_err("Can't negotiate: %d\n", ret);
return ret;
}
-
- st->status = SMB_DIRECT_CS_CONNECTED;
return 0;
}
@@ -1933,6 +1943,7 @@ static bool rdma_frwr_is_supported(struct ib_device_attr *attrs)
static int smb_direct_handle_connect_request(struct rdma_cm_id *new_cm_id)
{
struct smb_direct_transport *t;
+ int ret;
if (!rdma_frwr_is_supported(&new_cm_id->device->attrs)) {
ksmbd_debug(RDMA,
@@ -1945,11 +1956,17 @@ static int smb_direct_handle_connect_request(struct rdma_cm_id *new_cm_id)
if (!t)
return -ENOMEM;
+ ret = smb_direct_connect(t);
+ if (ret) {
+ free_transport(t);
+ return ret;
+ }
+
KSMBD_TRANS(t)->handler = kthread_run(ksmbd_conn_handler_loop,
KSMBD_TRANS(t)->conn, "ksmbd:r%u",
smb_direct_port);
if (IS_ERR(KSMBD_TRANS(t)->handler)) {
- int ret = PTR_ERR(KSMBD_TRANS(t)->handler);
+ ret = PTR_ERR(KSMBD_TRANS(t)->handler);
pr_err("Can't start thread\n");
free_transport(t);
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] ksmbd: smbd: call rdma_accept() under CM handler
2022-01-03 0:02 [PATCH] ksmbd: smbd: call rdma_accept() under CM handler Hyunchul Lee
@ 2022-01-04 0:45 ` Namjae Jeon
2022-01-04 1:10 ` Hyunchul Lee
0 siblings, 1 reply; 4+ messages in thread
From: Namjae Jeon @ 2022-01-04 0:45 UTC (permalink / raw)
To: Hyunchul Lee; +Cc: linux-cifs, Sergey Senozhatsky, Steve French
2022-01-03 9:02 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> if CONFIG_LOCKDEP is enabled, the following
> kernel warning message is generated because
> rdma_accept() is called not under CM(Connection
> Manager) handler.
>
> [ 63.211405 ] WARNING: CPU: 1 PID: 345 at
> drivers/infiniband/core/cma.c:4405 rdma_accept+0x17a/0x350
> [ 63.212080 ] RIP: 0010:rdma_accept+0x17a/0x350
> ...
> [ 63.214036 ] Call Trace:
> [ 63.214098 ] <TASK>
> [ 63.214185 ] smb_direct_accept_client+0xb4/0x170 [ksmbd]
> [ 63.214412 ] smb_direct_prepare+0x322/0x8c0 [ksmbd]
> [ 63.214555 ] ? rcu_read_lock_sched_held+0x3a/0x70
> [ 63.214700 ] ksmbd_conn_handler_loop+0x63/0x270 [ksmbd]
> [ 63.214826 ] ? ksmbd_conn_alive+0x80/0x80 [ksmbd]
> [ 63.214952 ] kthread+0x171/0x1a0
> [ 63.215039 ] ? set_kthread_struct+0x40/0x40
> [ 63.215128 ] ret_from_fork+0x22/0x30
I could not understand why lockdep trigger this warning.
Could you elaborate more ?
>
> To avoid this, move creating a queue pair and accepting
> a client from transport_ops->prepare() to
> smb_direct_handle_connect_request().
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> ---
> fs/ksmbd/transport_rdma.c | 97 +++++++++++++++++++++++----------------
> 1 file changed, 57 insertions(+), 40 deletions(-)
>
> diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
> index f89b64e27836..b37e4b9580ae 100644
> --- a/fs/ksmbd/transport_rdma.c
> +++ b/fs/ksmbd/transport_rdma.c
> @@ -568,6 +568,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc
> *wc)
> }
> t->negotiation_requested = true;
> t->full_packet_received = true;
> + enqueue_reassembly(t, recvmsg, 0);
Is this a fix related to this patch?
> wake_up_interruptible(&t->wait_status);
> break;
> case SMB_DIRECT_MSG_DATA_TRANSFER: {
> @@ -1594,19 +1595,13 @@ static int smb_direct_accept_client(struct
> smb_direct_transport *t)
> pr_err("error at rdma_accept: %d\n", ret);
> return ret;
> }
> -
> - wait_event_interruptible(t->wait_status,
> - t->status != SMB_DIRECT_CS_NEW);
> - if (t->status != SMB_DIRECT_CS_CONNECTED)
> - return -ENOTCONN;
> return 0;
> }
>
> -static int smb_direct_negotiate(struct smb_direct_transport *t)
> +static int smb_direct_prepare_negotiation(struct smb_direct_transport *t)
> {
> int ret;
> struct smb_direct_recvmsg *recvmsg;
> - struct smb_direct_negotiate_req *req;
>
> recvmsg = get_free_recvmsg(t);
> if (!recvmsg)
> @@ -1616,44 +1611,20 @@ static int smb_direct_negotiate(struct
> smb_direct_transport *t)
> ret = smb_direct_post_recv(t, recvmsg);
> if (ret) {
> pr_err("Can't post recv: %d\n", ret);
> - goto out;
> + goto out_err;
> }
>
> t->negotiation_requested = false;
> ret = smb_direct_accept_client(t);
> if (ret) {
> pr_err("Can't accept client\n");
> - goto out;
> + goto out_err;
> }
>
> smb_direct_post_recv_credits(&t->post_recv_credits_work.work);
> -
> - ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
> - ret = wait_event_interruptible_timeout(t->wait_status,
> - t->negotiation_requested ||
> - t->status == SMB_DIRECT_CS_DISCONNECTED,
> - SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
> - if (ret <= 0 || t->status == SMB_DIRECT_CS_DISCONNECTED) {
> - ret = ret < 0 ? ret : -ETIMEDOUT;
> - goto out;
> - }
> -
> - ret = smb_direct_check_recvmsg(recvmsg);
> - if (ret == -ECONNABORTED)
> - goto out;
> -
> - req = (struct smb_direct_negotiate_req *)recvmsg->packet;
> - t->max_recv_size = min_t(int, t->max_recv_size,
> - le32_to_cpu(req->preferred_send_size));
> - t->max_send_size = min_t(int, t->max_send_size,
> - le32_to_cpu(req->max_receive_size));
> - t->max_fragmented_send_size =
> - le32_to_cpu(req->max_fragmented_size);
> -
> - ret = smb_direct_send_negotiate_response(t, ret);
> -out:
> - if (recvmsg)
> - put_recvmsg(t, recvmsg);
> + return 0;
> +out_err:
> + put_recvmsg(t, recvmsg);
> return ret;
> }
>
> @@ -1890,6 +1861,47 @@ static int smb_direct_create_qpair(struct
> smb_direct_transport *t,
> static int smb_direct_prepare(struct ksmbd_transport *t)
> {
> struct smb_direct_transport *st = smb_trans_direct_transfort(t);
> + struct smb_direct_recvmsg *recvmsg;
> + struct smb_direct_negotiate_req *req;
> + int ret;
> +
> + ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
> + ret = wait_event_interruptible_timeout(st->wait_status,
> + st->negotiation_requested ||
> + st->status == SMB_DIRECT_CS_DISCONNECTED,
> + SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
> + if (ret <= 0 || st->status == SMB_DIRECT_CS_DISCONNECTED)
> + return ret < 0 ? ret : -ETIMEDOUT;
> +
> + recvmsg = get_first_reassembly(st);
> + if (!recvmsg)
> + return -ECONNABORTED;
> +
> + ret = smb_direct_check_recvmsg(recvmsg);
> + if (ret == -ECONNABORTED)
> + goto out;
> +
> + req = (struct smb_direct_negotiate_req *)recvmsg->packet;
> + st->max_recv_size = min_t(int, st->max_recv_size,
> + le32_to_cpu(req->preferred_send_size));
> + st->max_send_size = min_t(int, st->max_send_size,
> + le32_to_cpu(req->max_receive_size));
> + st->max_fragmented_send_size =
> + le32_to_cpu(req->max_fragmented_size);
> +
> + ret = smb_direct_send_negotiate_response(st, ret);
> +out:
> + spin_lock_irq(&st->reassembly_queue_lock);
> + st->reassembly_queue_length--;
> + list_del(&recvmsg->list);
> + spin_unlock_irq(&st->reassembly_queue_lock);
> + put_recvmsg(st, recvmsg);
> +
> + return ret;
> +}
> +
> +static int smb_direct_connect(struct smb_direct_transport *st)
> +{
> int ret;
> struct ib_qp_cap qp_cap;
>
> @@ -1911,13 +1923,11 @@ static int smb_direct_prepare(struct ksmbd_transport
> *t)
> return ret;
> }
>
> - ret = smb_direct_negotiate(st);
> + ret = smb_direct_prepare_negotiation(st);
> if (ret) {
> pr_err("Can't negotiate: %d\n", ret);
> return ret;
> }
> -
> - st->status = SMB_DIRECT_CS_CONNECTED;
> return 0;
> }
>
> @@ -1933,6 +1943,7 @@ static bool rdma_frwr_is_supported(struct
> ib_device_attr *attrs)
> static int smb_direct_handle_connect_request(struct rdma_cm_id *new_cm_id)
> {
> struct smb_direct_transport *t;
> + int ret;
>
> if (!rdma_frwr_is_supported(&new_cm_id->device->attrs)) {
> ksmbd_debug(RDMA,
> @@ -1945,11 +1956,17 @@ static int smb_direct_handle_connect_request(struct
> rdma_cm_id *new_cm_id)
> if (!t)
> return -ENOMEM;
>
> + ret = smb_direct_connect(t);
> + if (ret) {
> + free_transport(t);
> + return ret;
I think that it is better to use goto statement to out after freeing
transport structure.
> + }
> +
> KSMBD_TRANS(t)->handler = kthread_run(ksmbd_conn_handler_loop,
> KSMBD_TRANS(t)->conn, "ksmbd:r%u",
> smb_direct_port);
> if (IS_ERR(KSMBD_TRANS(t)->handler)) {
> - int ret = PTR_ERR(KSMBD_TRANS(t)->handler);
> + ret = PTR_ERR(KSMBD_TRANS(t)->handler);
>
> pr_err("Can't start thread\n");
> free_transport(t);
> --
> 2.25.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ksmbd: smbd: call rdma_accept() under CM handler
2022-01-04 0:45 ` Namjae Jeon
@ 2022-01-04 1:10 ` Hyunchul Lee
2022-01-04 1:19 ` Namjae Jeon
0 siblings, 1 reply; 4+ messages in thread
From: Hyunchul Lee @ 2022-01-04 1:10 UTC (permalink / raw)
To: Namjae Jeon; +Cc: linux-cifs, Sergey Senozhatsky, Steve French
2022년 1월 4일 (화) 오전 9:45, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2022-01-03 9:02 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > if CONFIG_LOCKDEP is enabled, the following
> > kernel warning message is generated because
> > rdma_accept() is called not under CM(Connection
> > Manager) handler.
> >
> > [ 63.211405 ] WARNING: CPU: 1 PID: 345 at
> > drivers/infiniband/core/cma.c:4405 rdma_accept+0x17a/0x350
> > [ 63.212080 ] RIP: 0010:rdma_accept+0x17a/0x350
> > ...
> > [ 63.214036 ] Call Trace:
> > [ 63.214098 ] <TASK>
> > [ 63.214185 ] smb_direct_accept_client+0xb4/0x170 [ksmbd]
> > [ 63.214412 ] smb_direct_prepare+0x322/0x8c0 [ksmbd]
> > [ 63.214555 ] ? rcu_read_lock_sched_held+0x3a/0x70
> > [ 63.214700 ] ksmbd_conn_handler_loop+0x63/0x270 [ksmbd]
> > [ 63.214826 ] ? ksmbd_conn_alive+0x80/0x80 [ksmbd]
> > [ 63.214952 ] kthread+0x171/0x1a0
> > [ 63.215039 ] ? set_kthread_struct+0x40/0x40
> > [ 63.215128 ] ret_from_fork+0x22/0x30
> I could not understand why lockdep trigger this warning.
> Could you elaborate more ?
>
rdma_accept checks whether the handler_mutex is held. if not,
this warning is generated. And CM holds the handler_mutex before
ksmbd's handler callback is called.
> >
> > To avoid this, move creating a queue pair and accepting
> > a client from transport_ops->prepare() to
> > smb_direct_handle_connect_request().
> >
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > ---
> > fs/ksmbd/transport_rdma.c | 97 +++++++++++++++++++++++----------------
> > 1 file changed, 57 insertions(+), 40 deletions(-)
> >
> > diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
> > index f89b64e27836..b37e4b9580ae 100644
> > --- a/fs/ksmbd/transport_rdma.c
> > +++ b/fs/ksmbd/transport_rdma.c
> > @@ -568,6 +568,7 @@ static void recv_done(struct ib_cq *cq, struct ib_wc
> > *wc)
> > }
> > t->negotiation_requested = true;
> > t->full_packet_received = true;
> > + enqueue_reassembly(t, recvmsg, 0);
> Is this a fix related to this patch?
Yes, this is required to receive the negotiation request
from a client.
Before this patch, posting a buffer and waiting for
the request is done in a function. Because
we have the reference of the buffer, we don't need to
append it into the queue.
> > wake_up_interruptible(&t->wait_status);
> > break;
> > case SMB_DIRECT_MSG_DATA_TRANSFER: {
> > @@ -1594,19 +1595,13 @@ static int smb_direct_accept_client(struct
> > smb_direct_transport *t)
> > pr_err("error at rdma_accept: %d\n", ret);
> > return ret;
> > }
> > -
> > - wait_event_interruptible(t->wait_status,
> > - t->status != SMB_DIRECT_CS_NEW);
> > - if (t->status != SMB_DIRECT_CS_CONNECTED)
> > - return -ENOTCONN;
> > return 0;
> > }
> >
> > -static int smb_direct_negotiate(struct smb_direct_transport *t)
> > +static int smb_direct_prepare_negotiation(struct smb_direct_transport *t)
> > {
> > int ret;
> > struct smb_direct_recvmsg *recvmsg;
> > - struct smb_direct_negotiate_req *req;
> >
> > recvmsg = get_free_recvmsg(t);
> > if (!recvmsg)
> > @@ -1616,44 +1611,20 @@ static int smb_direct_negotiate(struct
> > smb_direct_transport *t)
> > ret = smb_direct_post_recv(t, recvmsg);
> > if (ret) {
> > pr_err("Can't post recv: %d\n", ret);
> > - goto out;
> > + goto out_err;
> > }
> >
> > t->negotiation_requested = false;
> > ret = smb_direct_accept_client(t);
> > if (ret) {
> > pr_err("Can't accept client\n");
> > - goto out;
> > + goto out_err;
> > }
> >
> > smb_direct_post_recv_credits(&t->post_recv_credits_work.work);
> > -
> > - ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
> > - ret = wait_event_interruptible_timeout(t->wait_status,
> > - t->negotiation_requested ||
> > - t->status == SMB_DIRECT_CS_DISCONNECTED,
> > - SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
> > - if (ret <= 0 || t->status == SMB_DIRECT_CS_DISCONNECTED) {
> > - ret = ret < 0 ? ret : -ETIMEDOUT;
> > - goto out;
> > - }
> > -
> > - ret = smb_direct_check_recvmsg(recvmsg);
> > - if (ret == -ECONNABORTED)
> > - goto out;
> > -
> > - req = (struct smb_direct_negotiate_req *)recvmsg->packet;
> > - t->max_recv_size = min_t(int, t->max_recv_size,
> > - le32_to_cpu(req->preferred_send_size));
> > - t->max_send_size = min_t(int, t->max_send_size,
> > - le32_to_cpu(req->max_receive_size));
> > - t->max_fragmented_send_size =
> > - le32_to_cpu(req->max_fragmented_size);
> > -
> > - ret = smb_direct_send_negotiate_response(t, ret);
> > -out:
> > - if (recvmsg)
> > - put_recvmsg(t, recvmsg);
> > + return 0;
> > +out_err:
> > + put_recvmsg(t, recvmsg);
> > return ret;
> > }
> >
> > @@ -1890,6 +1861,47 @@ static int smb_direct_create_qpair(struct
> > smb_direct_transport *t,
> > static int smb_direct_prepare(struct ksmbd_transport *t)
> > {
> > struct smb_direct_transport *st = smb_trans_direct_transfort(t);
> > + struct smb_direct_recvmsg *recvmsg;
> > + struct smb_direct_negotiate_req *req;
> > + int ret;
> > +
> > + ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
> > + ret = wait_event_interruptible_timeout(st->wait_status,
> > + st->negotiation_requested ||
> > + st->status == SMB_DIRECT_CS_DISCONNECTED,
> > + SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
> > + if (ret <= 0 || st->status == SMB_DIRECT_CS_DISCONNECTED)
> > + return ret < 0 ? ret : -ETIMEDOUT;
> > +
> > + recvmsg = get_first_reassembly(st);
> > + if (!recvmsg)
> > + return -ECONNABORTED;
> > +
> > + ret = smb_direct_check_recvmsg(recvmsg);
> > + if (ret == -ECONNABORTED)
> > + goto out;
> > +
> > + req = (struct smb_direct_negotiate_req *)recvmsg->packet;
> > + st->max_recv_size = min_t(int, st->max_recv_size,
> > + le32_to_cpu(req->preferred_send_size));
> > + st->max_send_size = min_t(int, st->max_send_size,
> > + le32_to_cpu(req->max_receive_size));
> > + st->max_fragmented_send_size =
> > + le32_to_cpu(req->max_fragmented_size);
> > +
> > + ret = smb_direct_send_negotiate_response(st, ret);
> > +out:
> > + spin_lock_irq(&st->reassembly_queue_lock);
> > + st->reassembly_queue_length--;
> > + list_del(&recvmsg->list);
> > + spin_unlock_irq(&st->reassembly_queue_lock);
> > + put_recvmsg(st, recvmsg);
> > +
> > + return ret;
> > +}
> > +
> > +static int smb_direct_connect(struct smb_direct_transport *st)
> > +{
> > int ret;
> > struct ib_qp_cap qp_cap;
> >
> > @@ -1911,13 +1923,11 @@ static int smb_direct_prepare(struct ksmbd_transport
> > *t)
> > return ret;
> > }
> >
> > - ret = smb_direct_negotiate(st);
> > + ret = smb_direct_prepare_negotiation(st);
> > if (ret) {
> > pr_err("Can't negotiate: %d\n", ret);
> > return ret;
> > }
> > -
> > - st->status = SMB_DIRECT_CS_CONNECTED;
> > return 0;
> > }
> >
> > @@ -1933,6 +1943,7 @@ static bool rdma_frwr_is_supported(struct
> > ib_device_attr *attrs)
> > static int smb_direct_handle_connect_request(struct rdma_cm_id *new_cm_id)
> > {
> > struct smb_direct_transport *t;
> > + int ret;
> >
> > if (!rdma_frwr_is_supported(&new_cm_id->device->attrs)) {
> > ksmbd_debug(RDMA,
> > @@ -1945,11 +1956,17 @@ static int smb_direct_handle_connect_request(struct
> > rdma_cm_id *new_cm_id)
> > if (!t)
> > return -ENOMEM;
> >
> > + ret = smb_direct_connect(t);
> > + if (ret) {
> > + free_transport(t);
> > + return ret;
> I think that it is better to use goto statement to out after freeing
> transport structure.
Okay, I will change it.
> > + }
> > +
> > KSMBD_TRANS(t)->handler = kthread_run(ksmbd_conn_handler_loop,
> > KSMBD_TRANS(t)->conn, "ksmbd:r%u",
> > smb_direct_port);
> > if (IS_ERR(KSMBD_TRANS(t)->handler)) {
> > - int ret = PTR_ERR(KSMBD_TRANS(t)->handler);
> > + ret = PTR_ERR(KSMBD_TRANS(t)->handler);
> >
> > pr_err("Can't start thread\n");
> > free_transport(t);
> > --
> > 2.25.1
> >
> >
--
Thanks,
Hyunchul
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] ksmbd: smbd: call rdma_accept() under CM handler
2022-01-04 1:10 ` Hyunchul Lee
@ 2022-01-04 1:19 ` Namjae Jeon
0 siblings, 0 replies; 4+ messages in thread
From: Namjae Jeon @ 2022-01-04 1:19 UTC (permalink / raw)
To: Hyunchul Lee; +Cc: linux-cifs, Sergey Senozhatsky, Steve French
2022-01-04 10:10 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> 2022년 1월 4일 (화) 오전 9:45, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>>
>> 2022-01-03 9:02 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
>> > if CONFIG_LOCKDEP is enabled, the following
>> > kernel warning message is generated because
>> > rdma_accept() is called not under CM(Connection
>> > Manager) handler.
>> >
>> > [ 63.211405 ] WARNING: CPU: 1 PID: 345 at
>> > drivers/infiniband/core/cma.c:4405 rdma_accept+0x17a/0x350
>> > [ 63.212080 ] RIP: 0010:rdma_accept+0x17a/0x350
>> > ...
>> > [ 63.214036 ] Call Trace:
>> > [ 63.214098 ] <TASK>
>> > [ 63.214185 ] smb_direct_accept_client+0xb4/0x170 [ksmbd]
>> > [ 63.214412 ] smb_direct_prepare+0x322/0x8c0 [ksmbd]
>> > [ 63.214555 ] ? rcu_read_lock_sched_held+0x3a/0x70
>> > [ 63.214700 ] ksmbd_conn_handler_loop+0x63/0x270 [ksmbd]
>> > [ 63.214826 ] ? ksmbd_conn_alive+0x80/0x80 [ksmbd]
>> > [ 63.214952 ] kthread+0x171/0x1a0
>> > [ 63.215039 ] ? set_kthread_struct+0x40/0x40
>> > [ 63.215128 ] ret_from_fork+0x22/0x30
>> I could not understand why lockdep trigger this warning.
>> Could you elaborate more ?
>>
>
> rdma_accept checks whether the handler_mutex is held. if not,
> this warning is generated. And CM holds the handler_mutex before
> ksmbd's handler callback is called.
Okay, please update patch description with this.
>
>> >
>> > To avoid this, move creating a queue pair and accepting
>> > a client from transport_ops->prepare() to
>> > smb_direct_handle_connect_request().
>> >
>> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>> > ---
>> > fs/ksmbd/transport_rdma.c | 97 +++++++++++++++++++++++----------------
>> > 1 file changed, 57 insertions(+), 40 deletions(-)
>> >
>> > diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
>> > index f89b64e27836..b37e4b9580ae 100644
>> > --- a/fs/ksmbd/transport_rdma.c
>> > +++ b/fs/ksmbd/transport_rdma.c
>> > @@ -568,6 +568,7 @@ static void recv_done(struct ib_cq *cq, struct
>> > ib_wc
>> > *wc)
>> > }
>> > t->negotiation_requested = true;
>> > t->full_packet_received = true;
>> > + enqueue_reassembly(t, recvmsg, 0);
>> Is this a fix related to this patch?
>
> Yes, this is required to receive the negotiation request
> from a client.
> Before this patch, posting a buffer and waiting for
> the request is done in a function. Because
> we have the reference of the buffer, we don't need to
> append it into the queue.
Okay.
>
>
>> > wake_up_interruptible(&t->wait_status);
>> > break;
>> > case SMB_DIRECT_MSG_DATA_TRANSFER: {
>> > @@ -1594,19 +1595,13 @@ static int smb_direct_accept_client(struct
>> > smb_direct_transport *t)
>> > pr_err("error at rdma_accept: %d\n", ret);
>> > return ret;
>> > }
>> > -
>> > - wait_event_interruptible(t->wait_status,
>> > - t->status != SMB_DIRECT_CS_NEW);
>> > - if (t->status != SMB_DIRECT_CS_CONNECTED)
>> > - return -ENOTCONN;
>> > return 0;
>> > }
>> >
>> > -static int smb_direct_negotiate(struct smb_direct_transport *t)
>> > +static int smb_direct_prepare_negotiation(struct smb_direct_transport
>> > *t)
>> > {
>> > int ret;
>> > struct smb_direct_recvmsg *recvmsg;
>> > - struct smb_direct_negotiate_req *req;
>> >
>> > recvmsg = get_free_recvmsg(t);
>> > if (!recvmsg)
>> > @@ -1616,44 +1611,20 @@ static int smb_direct_negotiate(struct
>> > smb_direct_transport *t)
>> > ret = smb_direct_post_recv(t, recvmsg);
>> > if (ret) {
>> > pr_err("Can't post recv: %d\n", ret);
>> > - goto out;
>> > + goto out_err;
>> > }
>> >
>> > t->negotiation_requested = false;
>> > ret = smb_direct_accept_client(t);
>> > if (ret) {
>> > pr_err("Can't accept client\n");
>> > - goto out;
>> > + goto out_err;
>> > }
>> >
>> > smb_direct_post_recv_credits(&t->post_recv_credits_work.work);
>> > -
>> > - ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
>> > - ret = wait_event_interruptible_timeout(t->wait_status,
>> > - t->negotiation_requested
>> > ||
>> > - t->status ==
>> > SMB_DIRECT_CS_DISCONNECTED,
>> > -
>> > SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
>> > - if (ret <= 0 || t->status == SMB_DIRECT_CS_DISCONNECTED) {
>> > - ret = ret < 0 ? ret : -ETIMEDOUT;
>> > - goto out;
>> > - }
>> > -
>> > - ret = smb_direct_check_recvmsg(recvmsg);
>> > - if (ret == -ECONNABORTED)
>> > - goto out;
>> > -
>> > - req = (struct smb_direct_negotiate_req *)recvmsg->packet;
>> > - t->max_recv_size = min_t(int, t->max_recv_size,
>> > - le32_to_cpu(req->preferred_send_size));
>> > - t->max_send_size = min_t(int, t->max_send_size,
>> > - le32_to_cpu(req->max_receive_size));
>> > - t->max_fragmented_send_size =
>> > - le32_to_cpu(req->max_fragmented_size);
>> > -
>> > - ret = smb_direct_send_negotiate_response(t, ret);
>> > -out:
>> > - if (recvmsg)
>> > - put_recvmsg(t, recvmsg);
>> > + return 0;
>> > +out_err:
>> > + put_recvmsg(t, recvmsg);
>> > return ret;
>> > }
>> >
>> > @@ -1890,6 +1861,47 @@ static int smb_direct_create_qpair(struct
>> > smb_direct_transport *t,
>> > static int smb_direct_prepare(struct ksmbd_transport *t)
>> > {
>> > struct smb_direct_transport *st = smb_trans_direct_transfort(t);
>> > + struct smb_direct_recvmsg *recvmsg;
>> > + struct smb_direct_negotiate_req *req;
>> > + int ret;
>> > +
>> > + ksmbd_debug(RDMA, "Waiting for SMB_DIRECT negotiate request\n");
>> > + ret = wait_event_interruptible_timeout(st->wait_status,
>> > + st->negotiation_requested
>> > ||
>> > + st->status ==
>> > SMB_DIRECT_CS_DISCONNECTED,
>> > +
>> > SMB_DIRECT_NEGOTIATE_TIMEOUT * HZ);
>> > + if (ret <= 0 || st->status == SMB_DIRECT_CS_DISCONNECTED)
>> > + return ret < 0 ? ret : -ETIMEDOUT;
>> > +
>> > + recvmsg = get_first_reassembly(st);
>> > + if (!recvmsg)
>> > + return -ECONNABORTED;
>> > +
>> > + ret = smb_direct_check_recvmsg(recvmsg);
>> > + if (ret == -ECONNABORTED)
>> > + goto out;
>> > +
>> > + req = (struct smb_direct_negotiate_req *)recvmsg->packet;
>> > + st->max_recv_size = min_t(int, st->max_recv_size,
>> > + le32_to_cpu(req->preferred_send_size));
>> > + st->max_send_size = min_t(int, st->max_send_size,
>> > + le32_to_cpu(req->max_receive_size));
>> > + st->max_fragmented_send_size =
>> > + le32_to_cpu(req->max_fragmented_size);
>> > +
>> > + ret = smb_direct_send_negotiate_response(st, ret);
>> > +out:
>> > + spin_lock_irq(&st->reassembly_queue_lock);
>> > + st->reassembly_queue_length--;
>> > + list_del(&recvmsg->list);
>> > + spin_unlock_irq(&st->reassembly_queue_lock);
>> > + put_recvmsg(st, recvmsg);
>> > +
>> > + return ret;
>> > +}
>> > +
>> > +static int smb_direct_connect(struct smb_direct_transport *st)
>> > +{
>> > int ret;
>> > struct ib_qp_cap qp_cap;
>> >
>> > @@ -1911,13 +1923,11 @@ static int smb_direct_prepare(struct
>> > ksmbd_transport
>> > *t)
>> > return ret;
>> > }
>> >
>> > - ret = smb_direct_negotiate(st);
>> > + ret = smb_direct_prepare_negotiation(st);
>> > if (ret) {
>> > pr_err("Can't negotiate: %d\n", ret);
>> > return ret;
>> > }
>> > -
>> > - st->status = SMB_DIRECT_CS_CONNECTED;
>> > return 0;
>> > }
>> >
>> > @@ -1933,6 +1943,7 @@ static bool rdma_frwr_is_supported(struct
>> > ib_device_attr *attrs)
>> > static int smb_direct_handle_connect_request(struct rdma_cm_id
>> > *new_cm_id)
>> > {
>> > struct smb_direct_transport *t;
>> > + int ret;
>> >
>> > if (!rdma_frwr_is_supported(&new_cm_id->device->attrs)) {
>> > ksmbd_debug(RDMA,
>> > @@ -1945,11 +1956,17 @@ static int
>> > smb_direct_handle_connect_request(struct
>> > rdma_cm_id *new_cm_id)
>> > if (!t)
>> > return -ENOMEM;
>> >
>> > + ret = smb_direct_connect(t);
>> > + if (ret) {
>> > + free_transport(t);
>> > + return ret;
>> I think that it is better to use goto statement to out after freeing
>> transport structure.
>
> Okay, I will change it.
OK.
Thanks.
>
>> > + }
>> > +
>> > KSMBD_TRANS(t)->handler = kthread_run(ksmbd_conn_handler_loop,
>> > KSMBD_TRANS(t)->conn,
>> > "ksmbd:r%u",
>> > smb_direct_port);
>> > if (IS_ERR(KSMBD_TRANS(t)->handler)) {
>> > - int ret = PTR_ERR(KSMBD_TRANS(t)->handler);
>> > + ret = PTR_ERR(KSMBD_TRANS(t)->handler);
>> >
>> > pr_err("Can't start thread\n");
>> > free_transport(t);
>> > --
>> > 2.25.1
>> >
>> >
>
>
>
> --
> Thanks,
> Hyunchul
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-04 1:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-03 0:02 [PATCH] ksmbd: smbd: call rdma_accept() under CM handler Hyunchul Lee
2022-01-04 0:45 ` Namjae Jeon
2022-01-04 1:10 ` Hyunchul Lee
2022-01-04 1:19 ` Namjae Jeon
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.