diff mbox series

ksmbd: smbd: call rdma_accept() under CM handler

Message ID 20220103000203.201107-1-hyc.lee@gmail.com
State New
Headers show
Series ksmbd: smbd: call rdma_accept() under CM handler | expand

Commit Message

Hyunchul Lee Jan. 3, 2022, 12:02 a.m. UTC 
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(-)

Comments

Namjae Jeon Jan. 4, 2022, 12:45 a.m. UTC | #1 
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
>
>
Hyunchul Lee Jan. 4, 2022, 1:10 a.m. UTC | #2 
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
> >
> >
Namjae Jeon Jan. 4, 2022, 1:19 a.m. UTC | #3 
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
>
diff mbox series

Patch

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);