Skip to contentSkip to conversation

IB/srpt and the percpu_ida conversion

PATCH In review Bart Van Assche 11 participants 29 messages Apr 6, 2016 – Apr 8, 2016 10 messages not in the archive

Subsystems (2)
InfiniBand SCSI RDMA protocol (SRP) target
lore
Share & tools
lore

Bart Van Assche

author
#0 lore raw

cover IB/srpt and the percpu_ida conversion

Hi Doug,

This patch series addresses the recently reported issue with regard to the percpu_ida conversion of the ib_srpt driver. My proposal is to submit patch 1/4 for inclusion in kernel v4.6 and to submit patches 2..4 for inclusion in kernel v4.7. The actual patches in this series are:

0001-IB-srpt-Revert-Convert-to-percpu_ida-tag-allocation.patch 0002-IB-srpt-Report-login-failures-only-once.patch 0003-IB-srpt-Introduce-two-helper-functions.patch 0004-IB-srpt-Convert-to-percpu_ida-tag-allocation.patch

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#1 lore raw

PATCH 1/4 IB/srpt: Revert "Convert to percpu_ida tag allocation"

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
 [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
 [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
 [<ffffffff81071ea7>] process_one_work+0x197/0x480
 [<ffffffff810721d9>] worker_thread+0x49/0x490
 [<ffffffff8107878a>] kthread+0xea/0x100
 [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Sagi Grimberg <[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 55 ++++++++++++++++++++++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 40 insertions(+), 17 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +38 -17
index 13594db..3b425af 100644
@@ -1264,26 +1264,40 @@ free_mem:
  */
 static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 {
-	struct se_session *se_sess;
 	struct srpt_send_ioctx *ioctx;
-	int tag;
+	unsigned long flags;
 
 	BUG_ON(!ch);
-	se_sess = ch->sess;
 
-	tag = percpu_ida_alloc(&se_sess->sess_tag_pool, TASK_RUNNING);
-	if (tag < 0) {
-		pr_err("Unable to obtain tag for srpt_send_ioctx\n");
-		return NULL;
+	ioctx = NULL;
+	spin_lock_irqsave(&ch->spinlock, flags);
+	if (!list_empty(&ch->free_list)) {
+		ioctx = list_first_entry(&ch->free_list,
+					 struct srpt_send_ioctx, free_list);
+		list_del(&ioctx->free_list);
 	}
-	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
-	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
-	ioctx->ch = ch;
+	spin_unlock_irqrestore(&ch->spinlock, flags);
+
+	if (!ioctx)
+		return ioctx;
+
+	BUG_ON(ioctx->ch != ch);
 	spin_lock_init(&ioctx->spinlock);
 	ioctx->state = SRPT_STATE_NEW;
+	ioctx->n_rbuf = 0;
+	ioctx->rbufs = NULL;
+	ioctx->n_rdma = 0;
+	ioctx->n_rdma_wrs = 0;
+	ioctx->rdma_wrs = NULL;
+	ioctx->mapped_sg_count = 0;
 	init_completion(&ioctx->tx_done);
-
-	ioctx->cmd.map_tag = tag;
+	ioctx->queue_status_only = false;
+	/*
+	 * transport_init_se_cmd() does not initialize all fields, so do it
+	 * here.
+	 */
+	memset(&ioctx->cmd, 0, sizeof(ioctx->cmd));
+	memset(&ioctx->sense_data, 0, sizeof(ioctx->sense_data));
 
 	return ioctx;
 }

@@ -2013,7 +2027,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	struct ib_cm_rep_param *rep_param;
 	struct srpt_rdma_ch *ch, *tmp_ch;
 	u32 it_iu_len;
-	int ret = 0;
+	int i, ret = 0;
 	unsigned char *p;
 
 	WARN_ON_ONCE(irqs_disabled());

@@ -2135,6 +2149,12 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	if (!ch->ioctx_ring)
 		goto free_ch;
 
+	INIT_LIST_HEAD(&ch->free_list);
+	for (i = 0; i < ch->rq_size; i++) {
+		ch->ioctx_ring[i]->ch = ch;
+		list_add_tail(&ch->ioctx_ring[i]->free_list, &ch->free_list);
+	}
+
 	ret = srpt_create_ch_ib(ch);
 	if (ret) {
 		rej->reason = cpu_to_be32(

@@ -2165,8 +2185,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	p = &ch->sess_name[0];
 
 try_again:
-	ch->sess = target_alloc_session(&sport->port_tpg_1, ch->rq_size,
-					sizeof(struct srpt_send_ioctx),
+	ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
 					TARGET_PROT_NORMAL, p, ch, NULL);
 	if (IS_ERR(ch->sess)) {
 		pr_info("Rejected login because no ACL has been"

@@ -2873,7 +2892,7 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 	struct srpt_send_ioctx *ioctx = container_of(se_cmd,
 				struct srpt_send_ioctx, cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
-	struct se_session *se_sess = ch->sess;
+	unsigned long flags;
 
 	WARN_ON(ioctx->state != SRPT_STATE_DONE);
 	WARN_ON(ioctx->mapped_sg_count != 0);

@@ -2884,7 +2903,9 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 		ioctx->n_rbuf = 0;
 	}
 
-	percpu_ida_free(&se_sess->sess_tag_pool, se_cmd->map_tag);
+	spin_lock_irqsave(&ch->spinlock, flags);
+	list_add(&ioctx->free_list, &ch->free_list);
+	spin_unlock_irqrestore(&ch->spinlock, flags);
 }
 
 /**
drivers/infiniband/ulp/srpt/ib_srpt.h +2 -0
index ca288f0..af9b8b5 100644
@@ -179,6 +179,7 @@ struct srpt_recv_ioctx {
  * struct srpt_send_ioctx - SRPT send I/O context.
  * @ioctx:       See above.
  * @ch:          Channel pointer.
+ * @free_list:   Node in srpt_rdma_ch.free_list.
  * @n_rbuf:      Number of data buffers in the received SRP command.
  * @rbufs:       Pointer to SRP data buffer array.
  * @single_rbuf: SRP data buffer if the command has only a single buffer.

@@ -201,6 +202,7 @@ struct srpt_send_ioctx {
 	struct srp_direct_buf	*rbufs;
 	struct srp_direct_buf	single_rbuf;
 	struct scatterlist	*sg;
+	struct list_head	free_list;
 	spinlock_t		spinlock;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
Signature · 1 line
2.7.4
#2 lore raw

PATCH 2/4 IB/srpt: Report login failures only once

In reply to Bart Van Assche

Report the following message only once if no ACL has been configured yet for an initiator port:

"Rejected login because no ACL has been configured yet for initiator %s.\n"

Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Sagi Grimberg <[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +9 -13
index 3b425af..07dfc50 100644
@@ -2028,7 +2028,6 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	struct srpt_rdma_ch *ch, *tmp_ch;
 	u32 it_iu_len;
 	int i, ret = 0;
-	unsigned char *p;
 
 	WARN_ON_ONCE(irqs_disabled());
 

@@ -2182,21 +2181,18 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 			be64_to_cpu(*(__be64 *)(ch->i_port_id + 8)));
 
 	pr_debug("registering session %s\n", ch->sess_name);
-	p = &ch->sess_name[0];
 
-try_again:
 	ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
-					TARGET_PROT_NORMAL, p, ch, NULL);
+					TARGET_PROT_NORMAL, ch->sess_name, ch,
+					NULL);
+	/* Retry without leading "0x" */
+	if (IS_ERR(ch->sess))
+		ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
+						TARGET_PROT_NORMAL,
+						ch->sess_name + 2, ch, NULL);
 	if (IS_ERR(ch->sess)) {
-		pr_info("Rejected login because no ACL has been"
-			" configured yet for initiator %s.\n", p);
-		/*
-		 * XXX: Hack to retry of ch->i_port_id without leading '0x'
-		 */
-		if (p == &ch->sess_name[0]) {
-			p += 2;
-			goto try_again;
-		}
+		pr_info("Rejected login because no ACL has been configured yet for initiator %s.\n",
+			ch->sess_name);
 		rej->reason = cpu_to_be32((PTR_ERR(ch->sess) == -ENOMEM) ?
 				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES :
 				SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
Signature · 1 line
2.7.4

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw

PATCH 3/4 IB/srpt: Introduce two helper functions

In reply to Bart Van Assche

Move the functionality for initializing and cleaning up I/O contexts into two new functions. This patch does not change any functionality.

Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Sagi Grimberg <[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 64 +++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 15 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +49 -15
index 07dfc50..cce6c46 100644
@@ -581,6 +581,31 @@ static void srpt_unregister_mad_agent(struct srpt_device *sdev)
 	}
 }
 
+static int srpt_init_ioctx(struct srpt_device *sdev,
+					  struct srpt_ioctx *ioctx,
+					  int dma_size,
+					  enum dma_data_direction dir)
+{
+	int ret = -ENOMEM;
+
+	ioctx->buf = kmalloc(dma_size, GFP_KERNEL);
+	if (!ioctx->buf)
+		goto out;
+
+	ioctx->dma = ib_dma_map_single(sdev->device, ioctx->buf, dma_size, dir);
+	if (ib_dma_mapping_error(sdev->device, ioctx->dma))
+		goto free_buf;
+
+	ret = 0;
+
+out:
+	return ret;
+
+free_buf:
+	kfree(ioctx->buf);
+	goto out;
+}
+
 /**
  * srpt_alloc_ioctx() - Allocate an SRPT I/O context structure.
  */

@@ -592,24 +617,34 @@ static struct srpt_ioctx *srpt_alloc_ioctx(struct srpt_device *sdev,
 
 	ioctx = kmalloc(ioctx_size, GFP_KERNEL);
 	if (!ioctx)
-		goto err;
-
-	ioctx->buf = kmalloc(dma_size, GFP_KERNEL);
-	if (!ioctx->buf)
-		goto err_free_ioctx;
+		goto out;
 
-	ioctx->dma = ib_dma_map_single(sdev->device, ioctx->buf, dma_size, dir);
-	if (ib_dma_mapping_error(sdev->device, ioctx->dma))
-		goto err_free_buf;
+	if (srpt_init_ioctx(sdev, ioctx, dma_size, dir) < 0)
+		goto free;
 
+out:
 	return ioctx;
 
-err_free_buf:
-	kfree(ioctx->buf);
-err_free_ioctx:
+free:
 	kfree(ioctx);
-err:
-	return NULL;
+	ioctx = NULL;
+	goto out;
+}
+
+/*
+ * Note: it is safe to call this function for a zero-initialized buffer
+ * even if srpt_init_ioctx() has not been called.
+ */
+static void srpt_cleanup_ioctx(struct srpt_device *sdev,
+			       struct srpt_ioctx *ioctx,
+			       int dma_size,
+			       enum dma_data_direction dir)
+{
+	if (!ioctx->buf)
+		return;
+
+	ib_dma_unmap_single(sdev->device, ioctx->dma, dma_size, dir);
+	kfree(ioctx->buf);
 }
 
 /**

@@ -621,8 +656,7 @@ static void srpt_free_ioctx(struct srpt_device *sdev, struct srpt_ioctx *ioctx,
 	if (!ioctx)
 		return;
 
-	ib_dma_unmap_single(sdev->device, ioctx->dma, dma_size, dir);
-	kfree(ioctx->buf);
+	srpt_cleanup_ioctx(sdev, ioctx, dma_size, dir);
 	kfree(ioctx);
 }
Signature · 1 line
2.7.4
Reviewed-by on v1 3/4
#6 lore raw

Re: [PATCH 3/4] IB/srpt: Introduce two helper functions

Quoted 3 lines · Bart Van Assche

Move the functionality for initializing and cleaning up I/O contexts into two new functions. This patch does not change any functionality.

Looks good,

Reviewed-by Christoph Hellwig <[email protected]>

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#8 lore raw

PATCH 4/4 IB/srpt: Convert to percpu_ida tag allocation

Just like other target drivers, use percpu_ida_alloc() and percpu_ida_free() for I/O context management.

Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Cc Christoph Hellwig <[email protected]>
Cc Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 135 ++++++++++++++++++----------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |   6 +-
 2 files changed, 73 insertions(+), 68 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +72 -63
index cce6c46..ae56287 100644
@@ -1293,28 +1293,31 @@ free_mem:
 	return -ENOMEM;
 }
 
+static struct srpt_send_ioctx *srpt_tag_to_ioctx(struct srpt_rdma_ch *ch,
+						 int tag)
+{
+	return &((struct srpt_send_ioctx *)ch->sess->sess_cmd_map)[tag];
+}
+
+static int srpt_ioctx_to_tag(struct srpt_rdma_ch *ch,
+			     struct srpt_send_ioctx *ioctx)
+{
+	return ioctx - (struct srpt_send_ioctx *)ch->sess->sess_cmd_map;
+}
+
 /**
  * srpt_get_send_ioctx() - Obtain an I/O context for sending to the initiator.
  */
 static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 {
+	struct se_session *sess = ch->sess;
 	struct srpt_send_ioctx *ioctx;
-	unsigned long flags;
-
-	BUG_ON(!ch);
-
-	ioctx = NULL;
-	spin_lock_irqsave(&ch->spinlock, flags);
-	if (!list_empty(&ch->free_list)) {
-		ioctx = list_first_entry(&ch->free_list,
-					 struct srpt_send_ioctx, free_list);
-		list_del(&ioctx->free_list);
-	}
-	spin_unlock_irqrestore(&ch->spinlock, flags);
-
-	if (!ioctx)
-		return ioctx;
+	int tag;
 
+	tag = percpu_ida_alloc(&sess->sess_tag_pool, TASK_RUNNING);
+	if (tag < 0)
+		return NULL;
+	ioctx = srpt_tag_to_ioctx(ch, tag);
 	BUG_ON(ioctx->ch != ch);
 	spin_lock_init(&ioctx->spinlock);
 	ioctx->state = SRPT_STATE_NEW;

@@ -2006,6 +2009,7 @@ static void srpt_release_channel_work(struct work_struct *w)
 	struct srpt_rdma_ch *ch;
 	struct srpt_device *sdev;
 	struct se_session *se_sess;
+	int i;
 
 	ch = container_of(w, struct srpt_rdma_ch, release_work);
 	pr_debug("%s: %s-%d; release_done = %p\n", __func__, ch->sess_name,

@@ -2020,6 +2024,14 @@ static void srpt_release_channel_work(struct work_struct *w)
 	target_sess_cmd_list_set_waiting(se_sess);
 	target_wait_for_sess_cmds(se_sess);
 
+	for (i = 0; i < ch->rq_size; i++) {
+		struct srpt_send_ioctx *ioctx = srpt_tag_to_ioctx(ch, i);
+
+		srpt_cleanup_ioctx(sdev, &ioctx->ioctx,
+				   ch->rsp_size,
+				   DMA_TO_DEVICE);
+	}
+
 	transport_deregister_session_configfs(se_sess);
 	transport_deregister_session(se_sess);
 	ch->sess = NULL;

@@ -2028,10 +2040,6 @@ static void srpt_release_channel_work(struct work_struct *w)
 
 	srpt_destroy_ch_ib(ch);
 
-	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
-			     ch->sport->sdev, ch->rq_size,
-			     ch->rsp_size, DMA_TO_DEVICE);
-
 	mutex_lock(&sdev->mutex);
 	list_del_init(&ch->list);
 	if (ch->release_done)

@@ -2175,36 +2183,6 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	INIT_LIST_HEAD(&ch->cmd_wait_list);
 	ch->rsp_size = ch->sport->port_attrib.srp_max_rsp_size;
 
-	ch->ioctx_ring = (struct srpt_send_ioctx **)
-		srpt_alloc_ioctx_ring(ch->sport->sdev, ch->rq_size,
-				      sizeof(*ch->ioctx_ring[0]),
-				      ch->rsp_size, DMA_TO_DEVICE);
-	if (!ch->ioctx_ring)
-		goto free_ch;
-
-	INIT_LIST_HEAD(&ch->free_list);
-	for (i = 0; i < ch->rq_size; i++) {
-		ch->ioctx_ring[i]->ch = ch;
-		list_add_tail(&ch->ioctx_ring[i]->free_list, &ch->free_list);
-	}
-
-	ret = srpt_create_ch_ib(ch);
-	if (ret) {
-		rej->reason = cpu_to_be32(
-			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
-		pr_err("rejected SRP_LOGIN_REQ because creating"
-		       " a new RDMA channel failed.\n");
-		goto free_ring;
-	}
-
-	ret = srpt_ch_qp_rtr(ch, ch->qp);
-	if (ret) {
-		rej->reason = cpu_to_be32(SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
-		pr_err("rejected SRP_LOGIN_REQ because enabling"
-		       " RTR failed (error code = %d)\n", ret);
-		goto destroy_ib;
-	}
-
 	/*
 	 * Use the initator port identifier as the session name, when
 	 * checking against se_node_acl->initiatorname[] this can be

@@ -2216,12 +2194,14 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 
 	pr_debug("registering session %s\n", ch->sess_name);
 
-	ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
+	ch->sess = target_alloc_session(&sport->port_tpg_1, ch->rq_size,
+					sizeof(struct srpt_send_ioctx),
 					TARGET_PROT_NORMAL, ch->sess_name, ch,
 					NULL);
 	/* Retry without leading "0x" */
 	if (IS_ERR(ch->sess))
-		ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
+		ch->sess = target_alloc_session(&sport->port_tpg_1, ch->rq_size,
+						sizeof(struct srpt_send_ioctx),
 						TARGET_PROT_NORMAL,
 						ch->sess_name + 2, ch, NULL);
 	if (IS_ERR(ch->sess)) {

@@ -2230,7 +2210,35 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 		rej->reason = cpu_to_be32((PTR_ERR(ch->sess) == -ENOMEM) ?
 				SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES :
 				SRP_LOGIN_REJ_CHANNEL_LIMIT_REACHED);
-		goto destroy_ib;
+		goto free_ch;
+	}
+
+	for (i = 0; i < ch->rq_size; i++) {
+		struct srpt_send_ioctx *ioctx = srpt_tag_to_ioctx(ch, i);
+
+		if (srpt_init_ioctx(sdev, &ioctx->ioctx, ch->rsp_size,
+				    DMA_TO_DEVICE) < 0) {
+			pr_err("Initialization of I/O context %d/%d failed\n",
+			       i, ch->rq_size);
+			goto deregister_session;
+		}
+		ioctx->ch = ch;
+	}
+
+	ret = srpt_create_ch_ib(ch);
+	if (ret) {
+		rej->reason = cpu_to_be32(
+			      SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
+		pr_err("rejected SRP_LOGIN_REQ because creating a new RDMA channel failed.\n");
+		goto deregister_session;
+	}
+
+	ret = srpt_ch_qp_rtr(ch, ch->qp);
+	if (ret) {
+		rej->reason = cpu_to_be32(SRP_LOGIN_REJ_INSUFFICIENT_RESOURCES);
+		pr_err("rejected SRP_LOGIN_REQ because enabling RTR failed (error code = %d)\n",
+		       ret);
+		goto release_channel;
 	}
 
 	pr_debug("Establish connection sess=%p name=%s cm_id=%p\n", ch->sess,

@@ -2274,17 +2282,21 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 
 release_channel:
 	srpt_disconnect_ch(ch);
+	srpt_destroy_ch_ib(ch);
+
+deregister_session:
+	for (i = 0; i < ch->rq_size; i++) {
+		struct srpt_send_ioctx *ioctx = srpt_tag_to_ioctx(ch, i);
+
+		srpt_cleanup_ioctx(sdev, &ioctx->ioctx,
+				   ch->rsp_size,
+				   DMA_TO_DEVICE);
+	}
+
 	transport_deregister_session_configfs(ch->sess);
 	transport_deregister_session(ch->sess);
 	ch->sess = NULL;
 
-destroy_ib:
-	srpt_destroy_ch_ib(ch);
-
-free_ring:
-	srpt_free_ioctx_ring((struct srpt_ioctx **)ch->ioctx_ring,
-			     ch->sport->sdev, ch->rq_size,
-			     ch->rsp_size, DMA_TO_DEVICE);
 free_ch:
 	kfree(ch);
 

@@ -2922,7 +2934,6 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 	struct srpt_send_ioctx *ioctx = container_of(se_cmd,
 				struct srpt_send_ioctx, cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
-	unsigned long flags;
 
 	WARN_ON(ioctx->state != SRPT_STATE_DONE);
 	WARN_ON(ioctx->mapped_sg_count != 0);

@@ -2933,9 +2944,7 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 		ioctx->n_rbuf = 0;
 	}
 
-	spin_lock_irqsave(&ch->spinlock, flags);
-	list_add(&ioctx->free_list, &ch->free_list);
-	spin_unlock_irqrestore(&ch->spinlock, flags);
+	percpu_ida_free(&ch->sess->sess_tag_pool, srpt_ioctx_to_tag(ch, ioctx));
 }
 
 /**
drivers/infiniband/ulp/srpt/ib_srpt.h +1 -5
index af9b8b5..7018981 100644
@@ -179,7 +179,6 @@ struct srpt_recv_ioctx {
  * struct srpt_send_ioctx - SRPT send I/O context.
  * @ioctx:       See above.
  * @ch:          Channel pointer.
- * @free_list:   Node in srpt_rdma_ch.free_list.
  * @n_rbuf:      Number of data buffers in the received SRP command.
  * @rbufs:       Pointer to SRP data buffer array.
  * @single_rbuf: SRP data buffer if the command has only a single buffer.

@@ -202,7 +201,6 @@ struct srpt_send_ioctx {
 	struct srp_direct_buf	*rbufs;
 	struct srp_direct_buf	single_rbuf;
 	struct scatterlist	*sg;
-	struct list_head	free_list;
 	spinlock_t		spinlock;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;

@@ -250,8 +248,7 @@ enum rdma_ch_state {
  * @req_lim:       request limit: maximum number of requests that may be sent
  *                 by the initiator without having received a response.
  * @req_lim_delta: Number of credits not yet sent back to the initiator.
- * @spinlock:      Protects free_list and state.
- * @free_list:     Head of list with free send I/O contexts.
+ * @spinlock:      Protects modifications of @state.
  * @state:         channel state. See also enum rdma_ch_state.
  * @ioctx_ring:    Send ring.
  * @list:          Node for insertion in the srpt_device.rch_list list.

@@ -279,7 +276,6 @@ struct srpt_rdma_ch {
 	atomic_t		req_lim;
 	atomic_t		req_lim_delta;
 	spinlock_t		spinlock;
-	struct list_head	free_list;
 	enum rdma_ch_state	state;
 	struct srpt_send_ioctx	**ioctx_ring;
 	struct list_head	list;
Signature · 6 lines
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reviewed-by on v1 4/4
#9 lore raw

Re: [PATCH 4/4] IB/srpt: Convert to percpu_ida tag allocation

drivers/infiniband/ulp/srpt/ib_srpt.c free_mem: · line 1306

Quoted 11 lines
drivers/infiniband/ulp/srpt/ib_srpt.c +11 -0
+static struct srpt_send_ioctx *srpt_tag_to_ioctx(struct srpt_rdma_ch *ch,
+						 int tag)
+{
+	return &((struct srpt_send_ioctx *)ch->sess->sess_cmd_map)[tag];
+}
+
+static int srpt_ioctx_to_tag(struct srpt_rdma_ch *ch,
+			     struct srpt_send_ioctx *ioctx)
+{
+	return ioctx - (struct srpt_send_ioctx *)ch->sess->sess_cmd_map;
+}

This is really something the core code should be doing..

I have to admit I really don't understand why the target core is trying to force everyone to use the ida allocator - the 'tag' concept here isn't really useful to most drivers.

drivers/infiniband/ulp/srpt/ib_srpt.c static int srpt_cm_req_recv(struct ib_cm_id *cm_id, · line 2226

Quoted 11 lines
drivers/infiniband/ulp/srpt/ib_srpt.c +11 -0
+	for (i = 0; i < ch->rq_size; i++) {
+		struct srpt_send_ioctx *ioctx = srpt_tag_to_ioctx(ch, i);
+
+		if (srpt_init_ioctx(sdev, &ioctx->ioctx, ch->rsp_size,
+				    DMA_TO_DEVICE) < 0) {
+			pr_err("Initialization of I/O context %d/%d failed\n",
+			       i, ch->rq_size);
+			goto deregister_session;
+		}
+		ioctx->ch = ch;
+	}

E.g. here it would be really nice if the the driver had to just provide init_cmd and cleanup_cmd methods, and the core would iterate over the map, and call them on each cmds without the need for the casts and pointer arithmetics in each driver.

But I guess for now this will do it:

Reviewed-by Christoph Hellwig <[email protected]>
#10 lore raw

Re: [PATCH 4/4] IB/srpt: Convert to percpu_ida tag allocation

In reply to Bart Van Assche
Quoted 2 lines · Bart Van Assche

Just like other target drivers, use percpu_ida_alloc() and percpu_ida_free() for I/O context management.

Can you please resend this patch straight on top of current Linus' tree? I think the two cleanup patches are worthwhile to resubmit later, but let's get the issues with the broken conversion sorted out first with the smallest possible attack vector from Nic.

I need this sorted out before sending the RDMA R/W API, so I'd really appreciate if this was sorted out quickly.

#11 lore raw

Re: [PATCH 4/4] IB/srpt: Convert to percpu_ida tag allocation

Quoted 9 lines · Christoph Hellwig
Quoted 2 lines · Bart Van Assche

Just like other target drivers, use percpu_ida_alloc() and percpu_ida_free() for I/O context management.

Can you please resend this patch straight on top of current Linus' tree? I think the two cleanup patches are worthwhile to resubmit later, but let's get the issues with the broken conversion sorted out first with the smallest possible attack vector from Nic.

Like I said, I'll be resolving this myself in target-pending for the next v4.6-rc fixes PULL.

author
#12 lore raw

PATCH 1/1 IB/srpt: Revert "Convert to percpu_ida tag allocation"

In reply to a message not in the archive

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- It swaps the order of the send ioctx allocation and the transition to RTR mode which is wrong.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
 [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
 [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
 [<ffffffff81071ea7>] process_one_work+0x197/0x480
 [<ffffffff810721d9>] worker_thread+0x49/0x490
 [<ffffffff8107878a>] kthread+0xea/0x100
 [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 55 ++++++++++++++++++++++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 40 insertions(+), 17 deletions(-)
drivers/infiniband/ulp/srpt/ib_srpt.c +38 -17
index 13594db..3b425af 100644
@@ -1264,26 +1264,40 @@ free_mem:
  */
 static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 {
-	struct se_session *se_sess;
 	struct srpt_send_ioctx *ioctx;
-	int tag;
+	unsigned long flags;
 
 	BUG_ON(!ch);
-	se_sess = ch->sess;
 
-	tag = percpu_ida_alloc(&se_sess->sess_tag_pool, TASK_RUNNING);
-	if (tag < 0) {
-		pr_err("Unable to obtain tag for srpt_send_ioctx\n");
-		return NULL;
+	ioctx = NULL;
+	spin_lock_irqsave(&ch->spinlock, flags);
+	if (!list_empty(&ch->free_list)) {
+		ioctx = list_first_entry(&ch->free_list,
+					 struct srpt_send_ioctx, free_list);
+		list_del(&ioctx->free_list);
 	}
-	ioctx = &((struct srpt_send_ioctx *)se_sess->sess_cmd_map)[tag];
-	memset(ioctx, 0, sizeof(struct srpt_send_ioctx));
-	ioctx->ch = ch;
+	spin_unlock_irqrestore(&ch->spinlock, flags);
+
+	if (!ioctx)
+		return ioctx;
+
+	BUG_ON(ioctx->ch != ch);
 	spin_lock_init(&ioctx->spinlock);
 	ioctx->state = SRPT_STATE_NEW;
+	ioctx->n_rbuf = 0;
+	ioctx->rbufs = NULL;
+	ioctx->n_rdma = 0;
+	ioctx->n_rdma_wrs = 0;
+	ioctx->rdma_wrs = NULL;
+	ioctx->mapped_sg_count = 0;
 	init_completion(&ioctx->tx_done);
-
-	ioctx->cmd.map_tag = tag;
+	ioctx->queue_status_only = false;
+	/*
+	 * transport_init_se_cmd() does not initialize all fields, so do it
+	 * here.
+	 */
+	memset(&ioctx->cmd, 0, sizeof(ioctx->cmd));
+	memset(&ioctx->sense_data, 0, sizeof(ioctx->sense_data));
 
 	return ioctx;
 }

@@ -2013,7 +2027,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	struct ib_cm_rep_param *rep_param;
 	struct srpt_rdma_ch *ch, *tmp_ch;
 	u32 it_iu_len;
-	int ret = 0;
+	int i, ret = 0;
 	unsigned char *p;
 
 	WARN_ON_ONCE(irqs_disabled());

@@ -2135,6 +2149,12 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	if (!ch->ioctx_ring)
 		goto free_ch;
 
+	INIT_LIST_HEAD(&ch->free_list);
+	for (i = 0; i < ch->rq_size; i++) {
+		ch->ioctx_ring[i]->ch = ch;
+		list_add_tail(&ch->ioctx_ring[i]->free_list, &ch->free_list);
+	}
+
 	ret = srpt_create_ch_ib(ch);
 	if (ret) {
 		rej->reason = cpu_to_be32(

@@ -2165,8 +2185,7 @@ static int srpt_cm_req_recv(struct ib_cm_id *cm_id,
 	p = &ch->sess_name[0];
 
 try_again:
-	ch->sess = target_alloc_session(&sport->port_tpg_1, ch->rq_size,
-					sizeof(struct srpt_send_ioctx),
+	ch->sess = target_alloc_session(&sport->port_tpg_1, 0, 0,
 					TARGET_PROT_NORMAL, p, ch, NULL);
 	if (IS_ERR(ch->sess)) {
 		pr_info("Rejected login because no ACL has been"

@@ -2873,7 +2892,7 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 	struct srpt_send_ioctx *ioctx = container_of(se_cmd,
 				struct srpt_send_ioctx, cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
-	struct se_session *se_sess = ch->sess;
+	unsigned long flags;
 
 	WARN_ON(ioctx->state != SRPT_STATE_DONE);
 	WARN_ON(ioctx->mapped_sg_count != 0);

@@ -2884,7 +2903,9 @@ static void srpt_release_cmd(struct se_cmd *se_cmd)
 		ioctx->n_rbuf = 0;
 	}
 
-	percpu_ida_free(&se_sess->sess_tag_pool, se_cmd->map_tag);
+	spin_lock_irqsave(&ch->spinlock, flags);
+	list_add(&ioctx->free_list, &ch->free_list);
+	spin_unlock_irqrestore(&ch->spinlock, flags);
 }
 
 /**
drivers/infiniband/ulp/srpt/ib_srpt.h +2 -0
index ca288f0..af9b8b5 100644
@@ -179,6 +179,7 @@ struct srpt_recv_ioctx {
  * struct srpt_send_ioctx - SRPT send I/O context.
  * @ioctx:       See above.
  * @ch:          Channel pointer.
+ * @free_list:   Node in srpt_rdma_ch.free_list.
  * @n_rbuf:      Number of data buffers in the received SRP command.
  * @rbufs:       Pointer to SRP data buffer array.
  * @single_rbuf: SRP data buffer if the command has only a single buffer.

@@ -201,6 +202,7 @@ struct srpt_send_ioctx {
 	struct srp_direct_buf	*rbufs;
 	struct srp_direct_buf	single_rbuf;
 	struct scatterlist	*sg;
+	struct list_head	free_list;
 	spinlock_t		spinlock;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
Signature · 6 lines
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#13 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 26 lines · Bart Van Assche

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- It swaps the order of the send ioctx allocation and the transition to RTR mode which is wrong.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
 [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
 [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
 [<ffffffff81071ea7>] process_one_work+0x197/0x480
 [<ffffffff810721d9>] worker_thread+0x49/0x490
 [<ffffffff8107878a>] kthread+0xea/0x100
 [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Quoted 2 lines
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>

Please stop trying to bypass target-pending for your target related patches.

I've already asked you not to revert the patch, because I'm working on a patch to address the v4.6-rc regression here:

http://www.spinics.net/lists/target-devel/msg12535.html

If you've found a bug in that patch, please comment in that thread, or even better send an incremental diff for what you've found.

A message that is not in the archive (sent off-list or not yet received): 1460072224.18732.67.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org

author
#15 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 27 lines · Nicholas A. Bellinger
Quoted 26 lines · Bart Van Assche

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- It swaps the order of the send ioctx allocation and the transition to RTR mode which is wrong.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
  [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
  [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
  [<ffffffff81071ea7>] process_one_work+0x197/0x480
  [<ffffffff810721d9>] worker_thread+0x49/0x490
  [<ffffffff8107878a>] kthread+0xea/0x100
  [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Quoted 4 lines
Quoted 2 lines
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>

I've already asked you not to revert the patch [ ... ]

But why not to revert that patch? Your patch was completely untested and makes the ib_srpt driver unusable. I think the regular approach for such patches is to revert them and to rework these in a later kernel version. Linus, please let me know if you disagree.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#17 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 31 lines · Bart Van Assche
Quoted 30 lines · Nicholas A. Bellinger
Quoted 28 lines · Bart Van Assche

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- It swaps the order of the send ioctx allocation and the transition to RTR mode which is wrong.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
  [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
  [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
  [<ffffffff81071ea7>] process_one_work+0x197/0x480
  [<ffffffff810721d9>] worker_thread+0x49/0x490
  [<ffffffff8107878a>] kthread+0xea/0x100
  [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>
Quoted 6 lines
Quoted 1 line

I've already asked you not to revert the patch [ ... ]

But why not to revert that patch? Your patch was completely untested and makes the ib_srpt driver unusable. I think the regular approach for such patches is to revert them and to rework these in a later kernel version. Linus, please let me know if you disagree.

Because I'm actively working on a bug-fix for the regression, and it's -rc2 in a target driver that hardly anyone cares about.

If the regression is not addressed before v4.6-rc6, then by all means revert the original patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1460073123.18732.80.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org

#19 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 5 lines · Nicholas A. Bellinger

Because I'm actively working on a bug-fix for the regression, and it's -rc2 in a target driver that hardly anyone cares about.

If the regression is not addressed before v4.6-rc6, then by all means revert the original patch.

Stop it. People do care. At least I do for sure, and it's blocking a mayor feature I want to land in 4.7. Bart has a proper fix, and it's perfectly valid that you ask him to resend with various tweaks towards how your prefer various details to be done, but there is no reason to be agressive and personal.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#20 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 4 lines · Christoph Hellwig
Quoted 3 lines · Nicholas A. Bellinger

Because I'm actively working on a bug-fix for the regression, and it's -rc2 in a target driver that hardly anyone cares about.

Quoted 8 lines
Quoted 2 lines

If the regression is not addressed before v4.6-rc6, then by all means revert the original patch.

Stop it. People do care. At least I do for sure, and it's blocking a mayor feature I want to land in 4.7. Bart has a proper fix, and it's perfectly valid that you ask him to resend with various tweaks towards how your prefer various details to be done, but there is no reason to be agressive and personal.

As mentioned, I'm actively working on a -v2 for this regression.

You'll certainly be the first to hear about it as it's resolved over the next days.

#21 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

In reply to a message not in the archive
Quoted 34 lines · Nicholas A. Bellinger
Quoted 33 lines · Bart Van Assche
Quoted 31 lines · Nicholas A. Bellinger
Quoted 26 lines · Bart Van Assche

That patch causes the ib_srpt driver to crash as soon as the first SCSI command is received. This means that that patch was untested. Hence revert it. The shortcomings of that patch are as follows:
- It makes the ib_srpt driver use I/O contexts allocated by transport_alloc_session_tags() but it does not initialize these I/O contexts properly. All the initializations performed by srpt_alloc_ioctx() are skipped.
- It swaps the order of the send ioctx allocation and the transition to RTR mode which is wrong.
- The amount of memory that is needed for I/O contexts is doubled.
- srpt_rdma_ch.free_list is no longer used but is not removed.

Revert commit 0fd10721fe36 and thereby fix the following kernel crash:

kernel BUG at drivers/infiniband/ulp/srpt/ib_srpt.c:1439! invalid opcode: 0000 [#1] SMP

Workqueue: target_completion target_complete_ok_work [target_core_mod]
RIP: 0010:[<ffffffffa052ef37>]  [<ffffffffa052ef37>] srpt_queue_response+0x437/0x4a0 [ib_srpt]
Call Trace:
  [<ffffffffa052f009>] srpt_queue_data_in+0x9/0x10 [ib_srpt]
  [<ffffffffa04f1ee2>] target_complete_ok_work+0x152/0x2b0 [target_core_mod]
  [<ffffffff81071ea7>] process_one_work+0x197/0x480
  [<ffffffff810721d9>] worker_thread+0x49/0x490
  [<ffffffff8107878a>] kthread+0xea/0x100
  [<ffffffff8159b172>] ret_from_fork+0x22/0x40
Quoted 2 lines
Signed-off-by Bart Van Assche <[email protected]>
Cc Nicholas Bellinger <[email protected]>

I've already asked you not to revert the patch [ ... ]

Quoted 7 lines
Quoted 4 lines

But why not to revert that patch? Your patch was completely untested and makes the ib_srpt driver unusable. I think the regular approach for such patches is to revert them and to rework these in a later kernel version. Linus, please let me know if you disagree.

Because I'm actively working on a bug-fix for the regression, and it's -rc2 in a target driver that hardly anyone cares about.

I care about this driver. It's what we use internally for all of our SRP testing as we don't have and of the big DDN arrays that provide an SRP targets, so we built our own so we could test our clients.

Quoted 5 lines

If the regression is not addressed before v4.6-rc6, then by all means revert the original patch.

Signature · 4 lines
Doug Ledford <[email protected]>    GPG Key ID: 0E572FDD
  Red Hat, Inc.
  100 E. Davie St
  Raleigh, NC 27601 USA
#22 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

In reply to Nicholas A. Bellinger
Quoted 3 lines · Nicholas A. Bellinger

Please stop trying to bypass target-pending for your target related patches.

Nicholas, if you send me totally broken and untested crap, you have lost *all* rights to then complain about "bypass you".

So get off your high horse. Bart did the right thing, and *you* fucked up. Don't try to shift him be the bad guy.

Linus

A message that is not in the archive (sent off-list or not yet received): CA+55aFwOnCeKzg4SuceZm98DgN-tV7aCdtrenAS93LP7GqrZtw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#24 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 11 lines · Linus Torvalds
Quoted 3 lines · Nicholas A. Bellinger

Please stop trying to bypass target-pending for your target related patches.

Nicholas, if you send me totally broken and untested crap, you have lost *all* rights to then complain about "bypass you".

So get off your high horse. Bart did the right thing, and *you* fucked up. Don't try to shift him be the bad guy.

Yes, I fucked up and am actively fixing the regression.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1460073392.18732.83.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org

Tested-by Bart Van Assche
#26 lore raw

Re: [PATCH] IB/srpt: Revert "Convert to percpu_ida tag allocation"

Quoted 2 lines · Nicholas A. Bellinger

Yes, I fucked up and am actively fixing the regression.

.. and then you complained when somebody who relies on that driver wants it *working*.

Really.

I don't know why you and Bart butt heads, but I don't _need_ to know. If people find major bugs that keep their machines from working and want them reverted, that's quite understandable.

So I'm going to apply that revert, and I will *not* pull a fix from you until I hear that the fix actually got tested and has a

Tested-by Bart Van Assche <[email protected]>

or similar from people who actually have the hardware.

Because bugs happen, and that's life.

But it's _not_ acceptable to complain when a used points out that the change had no testing what-so-ever, and asks to have it reverted.

If you have a patch to fix it, reply to a revert request with "Here, try this instead": That's a good approach, and allows the user of that hardware to continue testing.

But if the change really had zero testing, and broke a driver that Bart needs to work, then I certainly see why he'd want to have it reverted.

Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#27 lore raw
In reply to Bart Van Assche
Quoted 7 lines · Bart Van Assche

Hi Doug,

This patch series addresses the recently reported issue with regard to the percpu_ida conversion of the ib_srpt driver. My proposal is to submit patch 1/4 for inclusion in kernel v4.6 and to submit patches 2..4 for inclusion in kernel v4.7. The actual patches in this series are:

Quoted 5 lines

0001-IB-srpt-Revert-Convert-to-percpu_ida-tag-allocation.patch 0002-IB-srpt-Report-login-failures-only-once.patch 0003-IB-srpt-Introduce-two-helper-functions.patch 0004-IB-srpt-Convert-to-percpu_ida-tag-allocation.patch

I asked to not revert the percpu_ida conversion and again you simply ignored subsystem maintainer feedback, and included a different percpu_ida conversion without using alloc_session callback that still does pre-allocation of buffers before session login even completes.

The whole point of the alloc_session callback is so that extra pre-allocation doesn't happen until after se_node_acl lookup has finished, and is what ib_srpt needs to be using given SRP's completely existent spec level security model.

In any event, I'm fixing the regression ahead of the next v4.6-rc PULL in:

http://www.spinics.net/lists/target-devel/msg12535.html

Any issues you find with this patch should be sent out as an incremental patch, and not rolling your own concoction.

A message that is not in the archive (sent off-list or not yet received): 1460068181.18732.23.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org

author
#29 lore raw
Quoted 4 lines · Nicholas A. Bellinger

The whole point of the alloc_session callback is so that extra pre-allocation doesn't happen until after se_node_acl lookup has finished, and is what ib_srpt needs to be using given SRP's completely existent spec level security model.

Sorry but I consider this as hair splitting. Who cares about whether or not command buffer allocation + freeing happens for login attempts that will fail because no ACL has been set up yet? That scenario is not performance critical.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#30 lore raw
In reply to Nicholas A. Bellinger
Quoted 4 lines · Nicholas A. Bellinger

I asked to not revert the percpu_ida conversion and again you simply ignored subsystem maintainer feedback, and included a different percpu_ida conversion without using alloc_session callback that still does pre-allocation of buffers before session login even completes.

He doesn't revert it after the patch series, but reverts it first to then do it properly. If you don't like that it's fine to ask Bart to resend with a different patch ordering, but it would be really helpful to do it politely.

Quoted 4 lines

The whole point of the alloc_session callback is so that extra pre-allocation doesn't happen until after se_node_acl lookup has finished, and is what ib_srpt needs to be using given SRP's completely existent spec level security model.

Your original patch didn't use the alloc_session callback either, so I don't really see the fuzz here. I don't see how it makes things clearer, but if you ask politely, and provide a detailed and valid explanation I'm pretty sure Bart will adopt the series to your taste.

Quoted 8 lines

In any event, I'm fixing the regression ahead of the next v4.6-rc PULL in:

http://www.spinics.net/lists/target-devel/msg12535.html

Any issues you find with this patch should be sent out as an incremental patch, and not rolling your own concoction.

That patch doesn't work. While it's great to have a maintainer that sets overall architectural direction it also really helps to work nicely with contributors that have detailed experience with the transport (or in this case even wrote the original driver).

I think a lot of these disagreements could be sorted out much better if you work with Bart at a technical level rather than having a personal vandetta.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#32 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 4 lines · Nicholas A. Bellinger

I asked to not revert the percpu_ida conversion and again you simply ignored subsystem maintainer feedback, and included a different percpu_ida conversion without using alloc_session callback that still does pre-allocation of buffers before session login even completes.

He doesn't revert it after the patch series, but reverts it first to then do it properly. If you don't like that it's fine to ask Bart to resend with a different patch ordering, but it would be really helpful to do it politely.

I've already taken the time to send out a patch to address the regression for v4.6-rc.

If there is a problem with that patch he should comment with the specifics of the issue he found, and not revert for v4.6 and do his own thing for v4.7.

Quoted 17 lines
Quoted 5 lines

The whole point of the alloc_session callback is so that extra pre-allocation doesn't happen until after se_node_acl lookup has finished, and is what ib_srpt needs to be using given SRP's completely existent spec level security model.

Your original patch didn't use the alloc_session callback either, so I don't really see the fuzz here. I don't see how it makes things clearer, but if you ask politely, and provide a detailed and valid explanation I'm pretty sure Bart will adopt the series to your taste.

Quoted 6 lines

In any event, I'm fixing the regression ahead of the next v4.6-rc PULL in:

http://www.spinics.net/lists/target-devel/msg12535.html

Quoted 8 lines
Quoted 2 lines

Any issues you find with this patch should be sent out as an incremental patch, and not rolling your own concoction.

That patch doesn't work. While it's great to have a maintainer that sets overall architectural direction it also really helps to work nicely with contributors that have detailed experience with the transport (or in this case even wrote the original driver).

Yep, will address for -v2 after verifying with IB ports.

Quoted 3 lines

I think a lot of these disagreements could be sorted out much better if you work with Bart at a technical level rather than having a personal vandetta.

It's nice that Bart has decided to wake up after 5 years of inactivity while I've been maintaining ib_srpt during that time.

I'll let the failed attempt at high-jacking the upstream target drivers be water under the bridge, but if he continues to ignore feedback and constantly mix-up unrelated issues at every opportunity like with the TMR stuff, plus ib_srpt patches never make it on target-devel, don't expect me to be in a particularly friendly mood.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#33 lore raw
Quoted 1 line · Nicholas A. Bellinger

[ ... ]

You are turning this on its head. I do what I can to improve the stability of the ib_srpt driver and the LIO core and you don't provide timely feedback on most of the LIO core patches I post. Nic, this means that you are not doing a proper job as a kernel maintainer. If someone has the right to complain then it's me.

Bart.

#34 lore raw
Quoted 9 lines · Bart Van Assche
Quoted 1 line · Nicholas A. Bellinger

[ ... ]

You are turning this on its head. I do what I can to improve the stability of the ib_srpt driver and the LIO core and you don't provide timely feedback on most of the LIO core patches I post. Nic, this means that you are not doing a proper job as a kernel maintainer. If someone has the right to complain then it's me.

Listen, I've grown tired a long time ago of you repeatably ignoring my feedback on changes of substance to target-core.

It's fine if you only look at target-core through the eyes of ib_srpt, but if you propose significant changes to target-core, and I take the time to explain why there are broken, but you propose them again and again ignoring the original feedback, don't expect a friendly response from me.

Also, none of your ib_srpt patches ever seem make it to target-devel.

Why is that..?

#35 lore raw
Quoted 1 line · Nicholas A. Bellinger

Also, none of your ib_srpt patches ever seem make it to target-devel.

That's not correct. For the two most recent patch submissions I CC-ed target-devel.

Bart.

#36 lore raw
In reply to Bart Van Assche

2016-04-08 1:34 GMT+02:00 Bart Van Assche <[email protected]>:

Quoted 5 lines · Nicholas A. Bellinger
Quoted 2 lines

[ ... ]

Quoted 7 lines

You are turning this on its head. I do what I can to improve the stability of the ib_srpt driver and the LIO core and you don't provide timely feedback on most of the LIO core patches I post. Nic, this means that you are not doing a proper job as a kernel maintainer. If someone has the right to complain then it's me.

Bart.

I'm on your side, Bart. A lot of people can see your passion and diligence about SRP/SRPT and the whole storage stack in Linux kernel. You deserve better.

Have a good day!

Jack

A message that is not in the archive (sent off-list or not yet received): 1460071464.18732.57.camel-XoQW25Eq2zviZyQQd+hFbcojREIfoBdhmpATvIKMPHk@public.gmane.org

author
#38 lore raw
Quoted 1 line · Nicholas A. Bellinger

[ ... ] but if he continues to ignore feedback [ ... ]

You are again turning it on its head. I know the ib_srpt driver much better than you so you should stop ignoring my feedback on your ib_srpt patches.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html