linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] cifs: smbd: Properly process errors on ib_post_send
@ 2020-04-02 20:56 longli
  2020-04-03  4:08 ` Steve French
  0 siblings, 1 reply; 3+ messages in thread
From: longli @ 2020-04-02 20:56 UTC (permalink / raw)
  To: Steve French, linux-cifs, samba-technical, linux-kernel; +Cc: Long Li

From: Long Li <longli@microsoft.com>

When processing errors from ib_post_send(), the transport state needs to be
rolled back to the condition before the error.

Refactor the old code to make it easy to roll back on IB errors, and fix this.

Signed-off-by: Long Li <longli@microsoft.com>
---

change in v2: rebased

 fs/cifs/smbdirect.c | 220 +++++++++++++++++++-------------------------
 1 file changed, 97 insertions(+), 123 deletions(-)

diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index fa52bf3e0236..dd3e119da296 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -800,41 +800,91 @@ static int manage_keep_alive_before_sending(struct smbd_connection *info)
 	return 0;
 }
 
-/*
- * Build and prepare the SMBD packet header
- * This function waits for avaialbe send credits and build a SMBD packet
- * header. The caller then optional append payload to the packet after
- * the header
- * intput values
- * size: the size of the payload
- * remaining_data_length: remaining data to send if this is part of a
- * fragmented packet
- * output values
- * request_out: the request allocated from this function
- * return values: 0 on success, otherwise actual error code returned
- */
-static int smbd_create_header(struct smbd_connection *info,
-		int size, int remaining_data_length,
-		struct smbd_request **request_out)
+/* Post the send request */
+static int smbd_post_send(struct smbd_connection *info,
+		struct smbd_request *request)
+{
+	struct ib_send_wr send_wr;
+	int rc, i;
+
+	for (i = 0; i < request->num_sge; i++) {
+		log_rdma_send(INFO,
+			"rdma_request sge[%d] addr=%llu length=%u\n",
+			i, request->sge[i].addr, request->sge[i].length);
+		ib_dma_sync_single_for_device(
+			info->id->device,
+			request->sge[i].addr,
+			request->sge[i].length,
+			DMA_TO_DEVICE);
+	}
+
+	request->cqe.done = send_done;
+
+	send_wr.next = NULL;
+	send_wr.wr_cqe = &request->cqe;
+	send_wr.sg_list = request->sge;
+	send_wr.num_sge = request->num_sge;
+	send_wr.opcode = IB_WR_SEND;
+	send_wr.send_flags = IB_SEND_SIGNALED;
+
+	rc = ib_post_send(info->id->qp, &send_wr, NULL);
+	if (rc) {
+		log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
+		smbd_disconnect_rdma_connection(info);
+		rc = -EAGAIN;
+	} else
+		/* Reset timer for idle connection after packet is sent */
+		mod_delayed_work(info->workqueue, &info->idle_timer_work,
+			info->keep_alive_interval*HZ);
+
+	return rc;
+}
+
+static int smbd_post_send_sgl(struct smbd_connection *info,
+	struct scatterlist *sgl, int data_length, int remaining_data_length)
 {
+	int num_sgs;
+	int i, rc;
+	int header_length;
 	struct smbd_request *request;
 	struct smbd_data_transfer *packet;
-	int header_length;
 	int new_credits;
-	int rc;
+	struct scatterlist *sg;
 
+wait_credit:
 	/* Wait for send credits. A SMBD packet needs one credit */
 	rc = wait_event_interruptible(info->wait_send_queue,
 		atomic_read(&info->send_credits) > 0 ||
 		info->transport_status != SMBD_CONNECTED);
 	if (rc)
-		return rc;
+		goto err_wait_credit;
 
 	if (info->transport_status != SMBD_CONNECTED) {
-		log_outgoing(ERR, "disconnected not sending\n");
-		return -EAGAIN;
+		log_outgoing(ERR, "disconnected not sending on wait_credit\n");
+		rc = -EAGAIN;
+		goto err_wait_credit;
+	}
+	if (unlikely(atomic_dec_return(&info->send_credits) < 0)) {
+		atomic_inc(&info->send_credits);
+		goto wait_credit;
+	}
+
+wait_send_queue:
+	wait_event(info->wait_post_send,
+		atomic_read(&info->send_pending) < info->send_credit_target ||
+		info->transport_status != SMBD_CONNECTED);
+
+	if (info->transport_status != SMBD_CONNECTED) {
+		log_outgoing(ERR, "disconnected not sending on wait_send_queue\n");
+		rc = -EAGAIN;
+		goto err_wait_send_queue;
+	}
+
+	if (unlikely(atomic_inc_return(&info->send_pending) >
+				info->send_credit_target)) {
+		atomic_dec(&info->send_pending);
+		goto wait_send_queue;
 	}
-	atomic_dec(&info->send_credits);
 
 	request = mempool_alloc(info->request_mempool, GFP_KERNEL);
 	if (!request) {
@@ -859,11 +909,11 @@ static int smbd_create_header(struct smbd_connection *info,
 		packet->flags |= cpu_to_le16(SMB_DIRECT_RESPONSE_REQUESTED);
 
 	packet->reserved = 0;
-	if (!size)
+	if (!data_length)
 		packet->data_offset = 0;
 	else
 		packet->data_offset = cpu_to_le32(24);
-	packet->data_length = cpu_to_le32(size);
+	packet->data_length = cpu_to_le32(data_length);
 	packet->remaining_data_length = cpu_to_le32(remaining_data_length);
 	packet->padding = 0;
 
@@ -878,7 +928,7 @@ static int smbd_create_header(struct smbd_connection *info,
 	/* Map the packet to DMA */
 	header_length = sizeof(struct smbd_data_transfer);
 	/* If this is a packet without payload, don't send padding */
-	if (!size)
+	if (!data_length)
 		header_length = offsetof(struct smbd_data_transfer, padding);
 
 	request->num_sge = 1;
@@ -887,107 +937,15 @@ static int smbd_create_header(struct smbd_connection *info,
 						 header_length,
 						 DMA_TO_DEVICE);
 	if (ib_dma_mapping_error(info->id->device, request->sge[0].addr)) {
-		mempool_free(request, info->request_mempool);
 		rc = -EIO;
+		request->sge[0].addr = 0;
 		goto err_dma;
 	}
 
 	request->sge[0].length = header_length;
 	request->sge[0].lkey = info->pd->local_dma_lkey;
 
-	*request_out = request;
-	return 0;
-
-err_dma:
-	/* roll back receive credits */
-	spin_lock(&info->lock_new_credits_offered);
-	info->new_credits_offered += new_credits;
-	spin_unlock(&info->lock_new_credits_offered);
-	atomic_sub(new_credits, &info->receive_credits);
-
-err_alloc:
-	/* roll back send credits */
-	atomic_inc(&info->send_credits);
-
-	return rc;
-}
-
-static void smbd_destroy_header(struct smbd_connection *info,
-		struct smbd_request *request)
-{
-
-	ib_dma_unmap_single(info->id->device,
-			    request->sge[0].addr,
-			    request->sge[0].length,
-			    DMA_TO_DEVICE);
-	mempool_free(request, info->request_mempool);
-	atomic_inc(&info->send_credits);
-}
-
-/* Post the send request */
-static int smbd_post_send(struct smbd_connection *info,
-		struct smbd_request *request)
-{
-	struct ib_send_wr send_wr;
-	int rc, i;
-
-	for (i = 0; i < request->num_sge; i++) {
-		log_rdma_send(INFO,
-			"rdma_request sge[%d] addr=%llu length=%u\n",
-			i, request->sge[i].addr, request->sge[i].length);
-		ib_dma_sync_single_for_device(
-			info->id->device,
-			request->sge[i].addr,
-			request->sge[i].length,
-			DMA_TO_DEVICE);
-	}
-
-	request->cqe.done = send_done;
-
-	send_wr.next = NULL;
-	send_wr.wr_cqe = &request->cqe;
-	send_wr.sg_list = request->sge;
-	send_wr.num_sge = request->num_sge;
-	send_wr.opcode = IB_WR_SEND;
-	send_wr.send_flags = IB_SEND_SIGNALED;
-
-wait_sq:
-	wait_event(info->wait_post_send,
-		atomic_read(&info->send_pending) < info->send_credit_target);
-	if (unlikely(atomic_inc_return(&info->send_pending) >
-				info->send_credit_target)) {
-		atomic_dec(&info->send_pending);
-		goto wait_sq;
-	}
-
-	rc = ib_post_send(info->id->qp, &send_wr, NULL);
-	if (rc) {
-		log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
-		if (atomic_dec_and_test(&info->send_pending))
-			wake_up(&info->wait_send_pending);
-		smbd_disconnect_rdma_connection(info);
-		rc = -EAGAIN;
-	} else
-		/* Reset timer for idle connection after packet is sent */
-		mod_delayed_work(info->workqueue, &info->idle_timer_work,
-			info->keep_alive_interval*HZ);
-
-	return rc;
-}
-
-static int smbd_post_send_sgl(struct smbd_connection *info,
-	struct scatterlist *sgl, int data_length, int remaining_data_length)
-{
-	int num_sgs;
-	int i, rc;
-	struct smbd_request *request;
-	struct scatterlist *sg;
-
-	rc = smbd_create_header(
-		info, data_length, remaining_data_length, &request);
-	if (rc)
-		return rc;
-
+	/* Fill in the packet data payload */
 	num_sgs = sgl ? sg_nents(sgl) : 0;
 	for_each_sg(sgl, sg, num_sgs, i) {
 		request->sge[i+1].addr =
@@ -997,7 +955,7 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
 				info->id->device, request->sge[i+1].addr)) {
 			rc = -EIO;
 			request->sge[i+1].addr = 0;
-			goto dma_mapping_failure;
+			goto err_dma;
 		}
 		request->sge[i+1].length = sg->length;
 		request->sge[i+1].lkey = info->pd->local_dma_lkey;
@@ -1008,14 +966,30 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
 	if (!rc)
 		return 0;
 
-dma_mapping_failure:
-	for (i = 1; i < request->num_sge; i++)
+err_dma:
+	for (i = 0; i < request->num_sge; i++)
 		if (request->sge[i].addr)
 			ib_dma_unmap_single(info->id->device,
 					    request->sge[i].addr,
 					    request->sge[i].length,
 					    DMA_TO_DEVICE);
-	smbd_destroy_header(info, request);
+	mempool_free(request, info->request_mempool);
+
+	/* roll back receive credits and credits to be offered */
+	spin_lock(&info->lock_new_credits_offered);
+	info->new_credits_offered += new_credits;
+	spin_unlock(&info->lock_new_credits_offered);
+	atomic_sub(new_credits, &info->receive_credits);
+
+err_alloc:
+	if (atomic_dec_and_test(&info->send_pending))
+		wake_up(&info->wait_send_pending);
+
+err_wait_send_queue:
+	/* roll back send credits and pending */
+	atomic_inc(&info->send_credits);
+
+err_wait_credit:
 	return rc;
 }
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] cifs: smbd: Properly process errors on ib_post_send
  2020-04-02 20:56 [PATCH v2] cifs: smbd: Properly process errors on ib_post_send longli
@ 2020-04-03  4:08 ` Steve French
  2020-04-03  4:10   ` Steve French
  0 siblings, 1 reply; 3+ messages in thread
From: Steve French @ 2020-04-03  4:08 UTC (permalink / raw)
  To: Long Li; +Cc: Steve French, CIFS, samba-technical, LKML, longli

tentatively merged into cifs-2.6.git for-next pending more testing

On Thu, Apr 2, 2020 at 3:57 PM longli--- via samba-technical
<samba-technical@lists.samba.org> wrote:
>
> From: Long Li <longli@microsoft.com>
>
> When processing errors from ib_post_send(), the transport state needs to be
> rolled back to the condition before the error.
>
> Refactor the old code to make it easy to roll back on IB errors, and fix this.
>
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>
> change in v2: rebased
>
>  fs/cifs/smbdirect.c | 220 +++++++++++++++++++-------------------------
>  1 file changed, 97 insertions(+), 123 deletions(-)
>
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index fa52bf3e0236..dd3e119da296 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -800,41 +800,91 @@ static int manage_keep_alive_before_sending(struct smbd_connection *info)
>         return 0;
>  }
>
> -/*
> - * Build and prepare the SMBD packet header
> - * This function waits for avaialbe send credits and build a SMBD packet
> - * header. The caller then optional append payload to the packet after
> - * the header
> - * intput values
> - * size: the size of the payload
> - * remaining_data_length: remaining data to send if this is part of a
> - * fragmented packet
> - * output values
> - * request_out: the request allocated from this function
> - * return values: 0 on success, otherwise actual error code returned
> - */
> -static int smbd_create_header(struct smbd_connection *info,
> -               int size, int remaining_data_length,
> -               struct smbd_request **request_out)
> +/* Post the send request */
> +static int smbd_post_send(struct smbd_connection *info,
> +               struct smbd_request *request)
> +{
> +       struct ib_send_wr send_wr;
> +       int rc, i;
> +
> +       for (i = 0; i < request->num_sge; i++) {
> +               log_rdma_send(INFO,
> +                       "rdma_request sge[%d] addr=%llu length=%u\n",
> +                       i, request->sge[i].addr, request->sge[i].length);
> +               ib_dma_sync_single_for_device(
> +                       info->id->device,
> +                       request->sge[i].addr,
> +                       request->sge[i].length,
> +                       DMA_TO_DEVICE);
> +       }
> +
> +       request->cqe.done = send_done;
> +
> +       send_wr.next = NULL;
> +       send_wr.wr_cqe = &request->cqe;
> +       send_wr.sg_list = request->sge;
> +       send_wr.num_sge = request->num_sge;
> +       send_wr.opcode = IB_WR_SEND;
> +       send_wr.send_flags = IB_SEND_SIGNALED;
> +
> +       rc = ib_post_send(info->id->qp, &send_wr, NULL);
> +       if (rc) {
> +               log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
> +               smbd_disconnect_rdma_connection(info);
> +               rc = -EAGAIN;
> +       } else
> +               /* Reset timer for idle connection after packet is sent */
> +               mod_delayed_work(info->workqueue, &info->idle_timer_work,
> +                       info->keep_alive_interval*HZ);
> +
> +       return rc;
> +}
> +
> +static int smbd_post_send_sgl(struct smbd_connection *info,
> +       struct scatterlist *sgl, int data_length, int remaining_data_length)
>  {
> +       int num_sgs;
> +       int i, rc;
> +       int header_length;
>         struct smbd_request *request;
>         struct smbd_data_transfer *packet;
> -       int header_length;
>         int new_credits;
> -       int rc;
> +       struct scatterlist *sg;
>
> +wait_credit:
>         /* Wait for send credits. A SMBD packet needs one credit */
>         rc = wait_event_interruptible(info->wait_send_queue,
>                 atomic_read(&info->send_credits) > 0 ||
>                 info->transport_status != SMBD_CONNECTED);
>         if (rc)
> -               return rc;
> +               goto err_wait_credit;
>
>         if (info->transport_status != SMBD_CONNECTED) {
> -               log_outgoing(ERR, "disconnected not sending\n");
> -               return -EAGAIN;
> +               log_outgoing(ERR, "disconnected not sending on wait_credit\n");
> +               rc = -EAGAIN;
> +               goto err_wait_credit;
> +       }
> +       if (unlikely(atomic_dec_return(&info->send_credits) < 0)) {
> +               atomic_inc(&info->send_credits);
> +               goto wait_credit;
> +       }
> +
> +wait_send_queue:
> +       wait_event(info->wait_post_send,
> +               atomic_read(&info->send_pending) < info->send_credit_target ||
> +               info->transport_status != SMBD_CONNECTED);
> +
> +       if (info->transport_status != SMBD_CONNECTED) {
> +               log_outgoing(ERR, "disconnected not sending on wait_send_queue\n");
> +               rc = -EAGAIN;
> +               goto err_wait_send_queue;
> +       }
> +
> +       if (unlikely(atomic_inc_return(&info->send_pending) >
> +                               info->send_credit_target)) {
> +               atomic_dec(&info->send_pending);
> +               goto wait_send_queue;
>         }
> -       atomic_dec(&info->send_credits);
>
>         request = mempool_alloc(info->request_mempool, GFP_KERNEL);
>         if (!request) {
> @@ -859,11 +909,11 @@ static int smbd_create_header(struct smbd_connection *info,
>                 packet->flags |= cpu_to_le16(SMB_DIRECT_RESPONSE_REQUESTED);
>
>         packet->reserved = 0;
> -       if (!size)
> +       if (!data_length)
>                 packet->data_offset = 0;
>         else
>                 packet->data_offset = cpu_to_le32(24);
> -       packet->data_length = cpu_to_le32(size);
> +       packet->data_length = cpu_to_le32(data_length);
>         packet->remaining_data_length = cpu_to_le32(remaining_data_length);
>         packet->padding = 0;
>
> @@ -878,7 +928,7 @@ static int smbd_create_header(struct smbd_connection *info,
>         /* Map the packet to DMA */
>         header_length = sizeof(struct smbd_data_transfer);
>         /* If this is a packet without payload, don't send padding */
> -       if (!size)
> +       if (!data_length)
>                 header_length = offsetof(struct smbd_data_transfer, padding);
>
>         request->num_sge = 1;
> @@ -887,107 +937,15 @@ static int smbd_create_header(struct smbd_connection *info,
>                                                  header_length,
>                                                  DMA_TO_DEVICE);
>         if (ib_dma_mapping_error(info->id->device, request->sge[0].addr)) {
> -               mempool_free(request, info->request_mempool);
>                 rc = -EIO;
> +               request->sge[0].addr = 0;
>                 goto err_dma;
>         }
>
>         request->sge[0].length = header_length;
>         request->sge[0].lkey = info->pd->local_dma_lkey;
>
> -       *request_out = request;
> -       return 0;
> -
> -err_dma:
> -       /* roll back receive credits */
> -       spin_lock(&info->lock_new_credits_offered);
> -       info->new_credits_offered += new_credits;
> -       spin_unlock(&info->lock_new_credits_offered);
> -       atomic_sub(new_credits, &info->receive_credits);
> -
> -err_alloc:
> -       /* roll back send credits */
> -       atomic_inc(&info->send_credits);
> -
> -       return rc;
> -}
> -
> -static void smbd_destroy_header(struct smbd_connection *info,
> -               struct smbd_request *request)
> -{
> -
> -       ib_dma_unmap_single(info->id->device,
> -                           request->sge[0].addr,
> -                           request->sge[0].length,
> -                           DMA_TO_DEVICE);
> -       mempool_free(request, info->request_mempool);
> -       atomic_inc(&info->send_credits);
> -}
> -
> -/* Post the send request */
> -static int smbd_post_send(struct smbd_connection *info,
> -               struct smbd_request *request)
> -{
> -       struct ib_send_wr send_wr;
> -       int rc, i;
> -
> -       for (i = 0; i < request->num_sge; i++) {
> -               log_rdma_send(INFO,
> -                       "rdma_request sge[%d] addr=%llu length=%u\n",
> -                       i, request->sge[i].addr, request->sge[i].length);
> -               ib_dma_sync_single_for_device(
> -                       info->id->device,
> -                       request->sge[i].addr,
> -                       request->sge[i].length,
> -                       DMA_TO_DEVICE);
> -       }
> -
> -       request->cqe.done = send_done;
> -
> -       send_wr.next = NULL;
> -       send_wr.wr_cqe = &request->cqe;
> -       send_wr.sg_list = request->sge;
> -       send_wr.num_sge = request->num_sge;
> -       send_wr.opcode = IB_WR_SEND;
> -       send_wr.send_flags = IB_SEND_SIGNALED;
> -
> -wait_sq:
> -       wait_event(info->wait_post_send,
> -               atomic_read(&info->send_pending) < info->send_credit_target);
> -       if (unlikely(atomic_inc_return(&info->send_pending) >
> -                               info->send_credit_target)) {
> -               atomic_dec(&info->send_pending);
> -               goto wait_sq;
> -       }
> -
> -       rc = ib_post_send(info->id->qp, &send_wr, NULL);
> -       if (rc) {
> -               log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
> -               if (atomic_dec_and_test(&info->send_pending))
> -                       wake_up(&info->wait_send_pending);
> -               smbd_disconnect_rdma_connection(info);
> -               rc = -EAGAIN;
> -       } else
> -               /* Reset timer for idle connection after packet is sent */
> -               mod_delayed_work(info->workqueue, &info->idle_timer_work,
> -                       info->keep_alive_interval*HZ);
> -
> -       return rc;
> -}
> -
> -static int smbd_post_send_sgl(struct smbd_connection *info,
> -       struct scatterlist *sgl, int data_length, int remaining_data_length)
> -{
> -       int num_sgs;
> -       int i, rc;
> -       struct smbd_request *request;
> -       struct scatterlist *sg;
> -
> -       rc = smbd_create_header(
> -               info, data_length, remaining_data_length, &request);
> -       if (rc)
> -               return rc;
> -
> +       /* Fill in the packet data payload */
>         num_sgs = sgl ? sg_nents(sgl) : 0;
>         for_each_sg(sgl, sg, num_sgs, i) {
>                 request->sge[i+1].addr =
> @@ -997,7 +955,7 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
>                                 info->id->device, request->sge[i+1].addr)) {
>                         rc = -EIO;
>                         request->sge[i+1].addr = 0;
> -                       goto dma_mapping_failure;
> +                       goto err_dma;
>                 }
>                 request->sge[i+1].length = sg->length;
>                 request->sge[i+1].lkey = info->pd->local_dma_lkey;
> @@ -1008,14 +966,30 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
>         if (!rc)
>                 return 0;
>
> -dma_mapping_failure:
> -       for (i = 1; i < request->num_sge; i++)
> +err_dma:
> +       for (i = 0; i < request->num_sge; i++)
>                 if (request->sge[i].addr)
>                         ib_dma_unmap_single(info->id->device,
>                                             request->sge[i].addr,
>                                             request->sge[i].length,
>                                             DMA_TO_DEVICE);
> -       smbd_destroy_header(info, request);
> +       mempool_free(request, info->request_mempool);
> +
> +       /* roll back receive credits and credits to be offered */
> +       spin_lock(&info->lock_new_credits_offered);
> +       info->new_credits_offered += new_credits;
> +       spin_unlock(&info->lock_new_credits_offered);
> +       atomic_sub(new_credits, &info->receive_credits);
> +
> +err_alloc:
> +       if (atomic_dec_and_test(&info->send_pending))
> +               wake_up(&info->wait_send_pending);
> +
> +err_wait_send_queue:
> +       /* roll back send credits and pending */
> +       atomic_inc(&info->send_credits);
> +
> +err_wait_credit:
>         return rc;
>  }
>
> --
> 2.17.1
>
>


-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v2] cifs: smbd: Properly process errors on ib_post_send
  2020-04-03  4:08 ` Steve French
@ 2020-04-03  4:10   ` Steve French
  0 siblings, 0 replies; 3+ messages in thread
From: Steve French @ 2020-04-03  4:10 UTC (permalink / raw)
  To: Long Li; +Cc: Steve French, CIFS, samba-technical, LKML, longli

also merged into github tree used by buildbot

http://smb3-test-rhel-75.southcentralus.cloudapp.azure.com/#/builders/2/builds/342

On Thu, Apr 2, 2020 at 11:08 PM Steve French <smfrench@gmail.com> wrote:
>
> tentatively merged into cifs-2.6.git for-next pending more testing
>
> On Thu, Apr 2, 2020 at 3:57 PM longli--- via samba-technical
> <samba-technical@lists.samba.org> wrote:
> >
> > From: Long Li <longli@microsoft.com>
> >
> > When processing errors from ib_post_send(), the transport state needs to be
> > rolled back to the condition before the error.
> >
> > Refactor the old code to make it easy to roll back on IB errors, and fix this.
> >
> > Signed-off-by: Long Li <longli@microsoft.com>
> > ---
> >
> > change in v2: rebased
> >
> >  fs/cifs/smbdirect.c | 220 +++++++++++++++++++-------------------------
> >  1 file changed, 97 insertions(+), 123 deletions(-)
> >
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> > index fa52bf3e0236..dd3e119da296 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -800,41 +800,91 @@ static int manage_keep_alive_before_sending(struct smbd_connection *info)
> >         return 0;
> >  }
> >
> > -/*
> > - * Build and prepare the SMBD packet header
> > - * This function waits for avaialbe send credits and build a SMBD packet
> > - * header. The caller then optional append payload to the packet after
> > - * the header
> > - * intput values
> > - * size: the size of the payload
> > - * remaining_data_length: remaining data to send if this is part of a
> > - * fragmented packet
> > - * output values
> > - * request_out: the request allocated from this function
> > - * return values: 0 on success, otherwise actual error code returned
> > - */
> > -static int smbd_create_header(struct smbd_connection *info,
> > -               int size, int remaining_data_length,
> > -               struct smbd_request **request_out)
> > +/* Post the send request */
> > +static int smbd_post_send(struct smbd_connection *info,
> > +               struct smbd_request *request)
> > +{
> > +       struct ib_send_wr send_wr;
> > +       int rc, i;
> > +
> > +       for (i = 0; i < request->num_sge; i++) {
> > +               log_rdma_send(INFO,
> > +                       "rdma_request sge[%d] addr=%llu length=%u\n",
> > +                       i, request->sge[i].addr, request->sge[i].length);
> > +               ib_dma_sync_single_for_device(
> > +                       info->id->device,
> > +                       request->sge[i].addr,
> > +                       request->sge[i].length,
> > +                       DMA_TO_DEVICE);
> > +       }
> > +
> > +       request->cqe.done = send_done;
> > +
> > +       send_wr.next = NULL;
> > +       send_wr.wr_cqe = &request->cqe;
> > +       send_wr.sg_list = request->sge;
> > +       send_wr.num_sge = request->num_sge;
> > +       send_wr.opcode = IB_WR_SEND;
> > +       send_wr.send_flags = IB_SEND_SIGNALED;
> > +
> > +       rc = ib_post_send(info->id->qp, &send_wr, NULL);
> > +       if (rc) {
> > +               log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
> > +               smbd_disconnect_rdma_connection(info);
> > +               rc = -EAGAIN;
> > +       } else
> > +               /* Reset timer for idle connection after packet is sent */
> > +               mod_delayed_work(info->workqueue, &info->idle_timer_work,
> > +                       info->keep_alive_interval*HZ);
> > +
> > +       return rc;
> > +}
> > +
> > +static int smbd_post_send_sgl(struct smbd_connection *info,
> > +       struct scatterlist *sgl, int data_length, int remaining_data_length)
> >  {
> > +       int num_sgs;
> > +       int i, rc;
> > +       int header_length;
> >         struct smbd_request *request;
> >         struct smbd_data_transfer *packet;
> > -       int header_length;
> >         int new_credits;
> > -       int rc;
> > +       struct scatterlist *sg;
> >
> > +wait_credit:
> >         /* Wait for send credits. A SMBD packet needs one credit */
> >         rc = wait_event_interruptible(info->wait_send_queue,
> >                 atomic_read(&info->send_credits) > 0 ||
> >                 info->transport_status != SMBD_CONNECTED);
> >         if (rc)
> > -               return rc;
> > +               goto err_wait_credit;
> >
> >         if (info->transport_status != SMBD_CONNECTED) {
> > -               log_outgoing(ERR, "disconnected not sending\n");
> > -               return -EAGAIN;
> > +               log_outgoing(ERR, "disconnected not sending on wait_credit\n");
> > +               rc = -EAGAIN;
> > +               goto err_wait_credit;
> > +       }
> > +       if (unlikely(atomic_dec_return(&info->send_credits) < 0)) {
> > +               atomic_inc(&info->send_credits);
> > +               goto wait_credit;
> > +       }
> > +
> > +wait_send_queue:
> > +       wait_event(info->wait_post_send,
> > +               atomic_read(&info->send_pending) < info->send_credit_target ||
> > +               info->transport_status != SMBD_CONNECTED);
> > +
> > +       if (info->transport_status != SMBD_CONNECTED) {
> > +               log_outgoing(ERR, "disconnected not sending on wait_send_queue\n");
> > +               rc = -EAGAIN;
> > +               goto err_wait_send_queue;
> > +       }
> > +
> > +       if (unlikely(atomic_inc_return(&info->send_pending) >
> > +                               info->send_credit_target)) {
> > +               atomic_dec(&info->send_pending);
> > +               goto wait_send_queue;
> >         }
> > -       atomic_dec(&info->send_credits);
> >
> >         request = mempool_alloc(info->request_mempool, GFP_KERNEL);
> >         if (!request) {
> > @@ -859,11 +909,11 @@ static int smbd_create_header(struct smbd_connection *info,
> >                 packet->flags |= cpu_to_le16(SMB_DIRECT_RESPONSE_REQUESTED);
> >
> >         packet->reserved = 0;
> > -       if (!size)
> > +       if (!data_length)
> >                 packet->data_offset = 0;
> >         else
> >                 packet->data_offset = cpu_to_le32(24);
> > -       packet->data_length = cpu_to_le32(size);
> > +       packet->data_length = cpu_to_le32(data_length);
> >         packet->remaining_data_length = cpu_to_le32(remaining_data_length);
> >         packet->padding = 0;
> >
> > @@ -878,7 +928,7 @@ static int smbd_create_header(struct smbd_connection *info,
> >         /* Map the packet to DMA */
> >         header_length = sizeof(struct smbd_data_transfer);
> >         /* If this is a packet without payload, don't send padding */
> > -       if (!size)
> > +       if (!data_length)
> >                 header_length = offsetof(struct smbd_data_transfer, padding);
> >
> >         request->num_sge = 1;
> > @@ -887,107 +937,15 @@ static int smbd_create_header(struct smbd_connection *info,
> >                                                  header_length,
> >                                                  DMA_TO_DEVICE);
> >         if (ib_dma_mapping_error(info->id->device, request->sge[0].addr)) {
> > -               mempool_free(request, info->request_mempool);
> >                 rc = -EIO;
> > +               request->sge[0].addr = 0;
> >                 goto err_dma;
> >         }
> >
> >         request->sge[0].length = header_length;
> >         request->sge[0].lkey = info->pd->local_dma_lkey;
> >
> > -       *request_out = request;
> > -       return 0;
> > -
> > -err_dma:
> > -       /* roll back receive credits */
> > -       spin_lock(&info->lock_new_credits_offered);
> > -       info->new_credits_offered += new_credits;
> > -       spin_unlock(&info->lock_new_credits_offered);
> > -       atomic_sub(new_credits, &info->receive_credits);
> > -
> > -err_alloc:
> > -       /* roll back send credits */
> > -       atomic_inc(&info->send_credits);
> > -
> > -       return rc;
> > -}
> > -
> > -static void smbd_destroy_header(struct smbd_connection *info,
> > -               struct smbd_request *request)
> > -{
> > -
> > -       ib_dma_unmap_single(info->id->device,
> > -                           request->sge[0].addr,
> > -                           request->sge[0].length,
> > -                           DMA_TO_DEVICE);
> > -       mempool_free(request, info->request_mempool);
> > -       atomic_inc(&info->send_credits);
> > -}
> > -
> > -/* Post the send request */
> > -static int smbd_post_send(struct smbd_connection *info,
> > -               struct smbd_request *request)
> > -{
> > -       struct ib_send_wr send_wr;
> > -       int rc, i;
> > -
> > -       for (i = 0; i < request->num_sge; i++) {
> > -               log_rdma_send(INFO,
> > -                       "rdma_request sge[%d] addr=%llu length=%u\n",
> > -                       i, request->sge[i].addr, request->sge[i].length);
> > -               ib_dma_sync_single_for_device(
> > -                       info->id->device,
> > -                       request->sge[i].addr,
> > -                       request->sge[i].length,
> > -                       DMA_TO_DEVICE);
> > -       }
> > -
> > -       request->cqe.done = send_done;
> > -
> > -       send_wr.next = NULL;
> > -       send_wr.wr_cqe = &request->cqe;
> > -       send_wr.sg_list = request->sge;
> > -       send_wr.num_sge = request->num_sge;
> > -       send_wr.opcode = IB_WR_SEND;
> > -       send_wr.send_flags = IB_SEND_SIGNALED;
> > -
> > -wait_sq:
> > -       wait_event(info->wait_post_send,
> > -               atomic_read(&info->send_pending) < info->send_credit_target);
> > -       if (unlikely(atomic_inc_return(&info->send_pending) >
> > -                               info->send_credit_target)) {
> > -               atomic_dec(&info->send_pending);
> > -               goto wait_sq;
> > -       }
> > -
> > -       rc = ib_post_send(info->id->qp, &send_wr, NULL);
> > -       if (rc) {
> > -               log_rdma_send(ERR, "ib_post_send failed rc=%d\n", rc);
> > -               if (atomic_dec_and_test(&info->send_pending))
> > -                       wake_up(&info->wait_send_pending);
> > -               smbd_disconnect_rdma_connection(info);
> > -               rc = -EAGAIN;
> > -       } else
> > -               /* Reset timer for idle connection after packet is sent */
> > -               mod_delayed_work(info->workqueue, &info->idle_timer_work,
> > -                       info->keep_alive_interval*HZ);
> > -
> > -       return rc;
> > -}
> > -
> > -static int smbd_post_send_sgl(struct smbd_connection *info,
> > -       struct scatterlist *sgl, int data_length, int remaining_data_length)
> > -{
> > -       int num_sgs;
> > -       int i, rc;
> > -       struct smbd_request *request;
> > -       struct scatterlist *sg;
> > -
> > -       rc = smbd_create_header(
> > -               info, data_length, remaining_data_length, &request);
> > -       if (rc)
> > -               return rc;
> > -
> > +       /* Fill in the packet data payload */
> >         num_sgs = sgl ? sg_nents(sgl) : 0;
> >         for_each_sg(sgl, sg, num_sgs, i) {
> >                 request->sge[i+1].addr =
> > @@ -997,7 +955,7 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
> >                                 info->id->device, request->sge[i+1].addr)) {
> >                         rc = -EIO;
> >                         request->sge[i+1].addr = 0;
> > -                       goto dma_mapping_failure;
> > +                       goto err_dma;
> >                 }
> >                 request->sge[i+1].length = sg->length;
> >                 request->sge[i+1].lkey = info->pd->local_dma_lkey;
> > @@ -1008,14 +966,30 @@ static int smbd_post_send_sgl(struct smbd_connection *info,
> >         if (!rc)
> >                 return 0;
> >
> > -dma_mapping_failure:
> > -       for (i = 1; i < request->num_sge; i++)
> > +err_dma:
> > +       for (i = 0; i < request->num_sge; i++)
> >                 if (request->sge[i].addr)
> >                         ib_dma_unmap_single(info->id->device,
> >                                             request->sge[i].addr,
> >                                             request->sge[i].length,
> >                                             DMA_TO_DEVICE);
> > -       smbd_destroy_header(info, request);
> > +       mempool_free(request, info->request_mempool);
> > +
> > +       /* roll back receive credits and credits to be offered */
> > +       spin_lock(&info->lock_new_credits_offered);
> > +       info->new_credits_offered += new_credits;
> > +       spin_unlock(&info->lock_new_credits_offered);
> > +       atomic_sub(new_credits, &info->receive_credits);
> > +
> > +err_alloc:
> > +       if (atomic_dec_and_test(&info->send_pending))
> > +               wake_up(&info->wait_send_pending);
> > +
> > +err_wait_send_queue:
> > +       /* roll back send credits and pending */
> > +       atomic_inc(&info->send_credits);
> > +
> > +err_wait_credit:
> >         return rc;
> >  }
> >
> > --
> > 2.17.1
> >
> >
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2020-04-03  4:10 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 20:56 [PATCH v2] cifs: smbd: Properly process errors on ib_post_send longli
2020-04-03  4:08 ` Steve French
2020-04-03  4:10   ` Steve French

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).