[PATCH 0/4] IB/srpt and the percpu_ida conversion

[PATCH 0/4] IB/srpt and the percpu_ida conversion

[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:

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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.
- 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 <bart.vanassche@sandisk.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 55 ++++++++++++++++++++++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 13594db..3b425af 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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);
 }
 
 /**
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index ca288f0..af9b8b5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -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;
-- 
2.7.4

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

[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 <bart.vanassche@sandisk.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 3b425af..07dfc50 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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);
-- 
2.7.4

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

[Christoph Hellwig]

Looks much better,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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 <bart.vanassche@sandisk.com>
Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Sagi Grimberg <sagig@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 64 +++++++++++++++++++++++++++--------
 1 file changed, 49 insertions(+), 15 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 07dfc50..cce6c46 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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);
 }
 
-- 
2.7.4

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

[Christoph Hellwig]

On Wed, Apr 06, 2016 at 11:57:20AM -0700, Bart Van Assche wrote:
> 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 <hch@lst.de>

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

[Bart Van Assche]

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

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 135 ++++++++++++++++++----------------
 drivers/infiniband/ulp/srpt/ib_srpt.h |   6 +-
 2 files changed, 73 insertions(+), 68 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index cce6c46..ae56287 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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));
 }
 
 /**
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index af9b8b5..7018981 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -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;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christoph Hellwig]

> +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.

> +	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 <hch@lst.de>

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

[Christoph Hellwig]

On Wed, Apr 06, 2016 at 11:57:41AM -0700, Bart Van Assche wrote:
> 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.

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

[Nicholas A. Bellinger]

On Fri, 2016-04-08 at 01:03 +0200, Christoph Hellwig wrote:
> On Wed, Apr 06, 2016 at 11:57:41AM -0700, Bart Van Assche wrote:
> > 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.

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

[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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 55 ++++++++++++++++++++++++-----------
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 40 insertions(+), 17 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 13594db..3b425af 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -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);
 }
 
 /**
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index ca288f0..af9b8b5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -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;
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 15:55 -0700, Bart Van Assche wrote:
> 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 <bart.vanassche@sandisk.com>
> Cc: Nicholas Bellinger <nab@linux-iscsi.org>

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.

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

[Bart Van Assche]

On 04/07/2016 04:37 PM, Nicholas A. Bellinger wrote:
> On Thu, 2016-04-07 at 15:55 -0700, Bart Van Assche wrote:
>> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>> Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
>
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 16:44 -0700, Bart Van Assche wrote:
> On 04/07/2016 04:37 PM, Nicholas A. Bellinger wrote:
> > On Thu, 2016-04-07 at 15:55 -0700, Bart Van Assche wrote:
> >> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> >> Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
> >
> > 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Christoph Hellwig]

On Thu, Apr 07, 2016 at 04:52:03PM -0700, Nicholas A. Bellinger wrote:
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 16:54 -0700, Christoph Hellwig wrote:
> On Thu, Apr 07, 2016 at 04:52:03PM -0700, Nicholas A. Bellinger wrote:
> > 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.

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.

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

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 2846 bytes --]

On 4/7/16 4:52 PM, Nicholas A. Bellinger wrote:
> On Thu, 2016-04-07 at 16:44 -0700, Bart Van Assche wrote:
>> On 04/07/2016 04:37 PM, Nicholas A. Bellinger wrote:
>>> On Thu, 2016-04-07 at 15:55 -0700, Bart Van Assche wrote:
>>>> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Nicholas Bellinger <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org>
>>>
>>> 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.

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.

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


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>    GPG Key ID: 0E572FDD
  Red Hat, Inc.
  100 E. Davie St
  Raleigh, NC 27601 USA


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 907 bytes --]

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

[Linus Torvalds]

On Apr 7, 2016 16:37, "Nicholas A. Bellinger" <nab@linux-iscsi.org> wrote:
>
> 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

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

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 16:49 -0700, Linus Torvalds wrote:
> On Apr 7, 2016 16:37, "Nicholas A. Bellinger" <nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
> >
> > 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Linus Torvalds]

On Thu, Apr 7, 2016 at 4:56 PM, Nicholas A. Bellinger
<nab-IzHhD5pYlfBP7FQvKIMDCQ@public.gmane.org> wrote:
>
> 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 <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>

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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Nicholas A. Bellinger]

On Wed, 2016-04-06 at 11:56 -0700, Bart Van Assche wrote:
> 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
> 

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.

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Bart Van Assche]

On 04/07/2016 03:29 PM, Nicholas A. Bellinger wrote:
> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Christoph Hellwig]

On Thu, Apr 07, 2016 at 03:29:41PM -0700, Nicholas A. Bellinger wrote:
> 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.

> 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.

> 
> 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.

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Nicholas A. Bellinger]

On Fri, 2016-04-08 at 01:01 +0200, Christoph Hellwig wrote:
> On Thu, Apr 07, 2016 at 03:29:41PM -0700, Nicholas A. Bellinger wrote:
> > 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.

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

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

> 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Bart Van Assche]

On 04/07/2016 04:24 PM, Nicholas A. Bellinger wrote:
> [ ... ]

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.

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Nicholas A. Bellinger]

On Thu, 2016-04-07 at 16:34 -0700, Bart Van Assche wrote:
> On 04/07/2016 04:24 PM, Nicholas A. Bellinger wrote:
> > [ ... ]
> 
> 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..?

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Bart Van Assche]

On 04/07/2016 04:47 PM, Nicholas A. Bellinger wrote:
> 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.

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Jack Wang]

2016-04-08 1:34 GMT+02:00 Bart Van Assche <bart.vanassche@sandisk.com>:
> On 04/07/2016 04:24 PM, Nicholas A. Bellinger wrote:
>>
>> [ ... ]
>
>
> 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

Re: [PATCH 0/4] IB/srpt and the percpu_ida conversion

[Bart Van Assche]

On 04/07/2016 04:24 PM, Nicholas A. Bellinger wrote:
> [ ... ]  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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html