IB SRP initiator patches for kernel 3.12

Certain storage configurations, e.g. a sufficiently large array of hard disks in a RAID configuration, need a queue depth above 64 to achieve optimal performance. Hence make the queue depth configurable.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <[email protected]>

Cc Vu Pham <vu-VPRAkNaXOzVWk0Htik3J/[email protected]>

Cc Sebastian Riemer <[email protected]>

Cc Konrad Grzybowski <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c |  125 ++++++++++++++++++++++++++---------
  drivers/infiniband/ulp/srp/ib_srp.h |   17 +++--
  2 files changed, 103 insertions(+), 39 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +95 -30

index ece1f2d..6de2323 100644
@@ -299,16 +299,16 @@ static int srp_create_target_ib(struct srp_target_port *target)
  		return -ENOMEM;
 
  	recv_cq = ib_create_cq(target->srp_host->srp_dev->dev,
-			       srp_recv_completion, NULL, target, SRP_RQ_SIZE,
-			       target->comp_vector);
+			       srp_recv_completion, NULL, target,
+			       target->queue_size, target->comp_vector);
  	if (IS_ERR(recv_cq)) {
  		ret = PTR_ERR(recv_cq);
  		goto err;
  	}
 
  	send_cq = ib_create_cq(target->srp_host->srp_dev->dev,
-			       srp_send_completion, NULL, target, SRP_SQ_SIZE,
-			       target->comp_vector);
+			       srp_send_completion, NULL, target,
+			       target->queue_size, target->comp_vector);
  	if (IS_ERR(send_cq)) {
  		ret = PTR_ERR(send_cq);
  		goto err_recv_cq;

@@ -317,8 +317,8 @@ static int srp_create_target_ib(struct srp_target_port *target)
  	ib_req_notify_cq(recv_cq, IB_CQ_NEXT_COMP);
 
  	init_attr->event_handler       = srp_qp_event;
-	init_attr->cap.max_send_wr     = SRP_SQ_SIZE;
-	init_attr->cap.max_recv_wr     = SRP_RQ_SIZE;
+	init_attr->cap.max_send_wr     = target->queue_size;
+	init_attr->cap.max_recv_wr     = target->queue_size;
  	init_attr->cap.max_recv_sge    = 1;
  	init_attr->cap.max_send_sge    = 1;
  	init_attr->sq_sig_type         = IB_SIGNAL_ALL_WR;

@@ -364,6 +364,10 @@ err:
  	return ret;
  }
 
+/*
+ * Note: this function may be called without srp_alloc_iu_bufs() having been
+ * invoked. Hence the target->[rt]x_ring checks.
+ */
  static void srp_free_target_ib(struct srp_target_port *target)
  {
  	int i;

@@ -375,10 +379,18 @@ static void srp_free_target_ib(struct srp_target_port *target)
  	target->qp = NULL;
  	target->send_cq = target->recv_cq = NULL;
 
-	for (i = 0; i < SRP_RQ_SIZE; ++i)
-		srp_free_iu(target->srp_host, target->rx_ring[i]);
-	for (i = 0; i < SRP_SQ_SIZE; ++i)
-		srp_free_iu(target->srp_host, target->tx_ring[i]);
+	if (target->rx_ring) {
+		for (i = 0; i < target->queue_size; ++i)
+			srp_free_iu(target->srp_host, target->rx_ring[i]);
+		kfree(target->rx_ring);
+		target->rx_ring = NULL;
+	}
+	if (target->tx_ring) {
+		for (i = 0; i < target->queue_size; ++i)
+			srp_free_iu(target->srp_host, target->tx_ring[i]);
+		kfree(target->tx_ring);
+		target->tx_ring = NULL;
+	}
  }
 
  static void srp_path_rec_completion(int status,

@@ -564,7 +576,11 @@ static void srp_free_req_data(struct srp_target_port *target)
  	struct srp_request *req;
  	int i;
 
-	for (i = 0, req = target->req_ring; i < SRP_CMD_SQ_SIZE; ++i, ++req) {
+	if (!target->req_ring)
+		return;
+
+	for (i = 0; i < target->req_ring_size; ++i) {
+		req = &target->req_ring[i];
  		kfree(req->fmr_list);
  		kfree(req->map_page);
  		if (req->indirect_dma_addr) {

@@ -574,6 +590,9 @@ static void srp_free_req_data(struct srp_target_port *target)
  		}
  		kfree(req->indirect_desc);
  	}
+
+	kfree(target->req_ring);
+	target->req_ring = NULL;
  }
 
  static int srp_alloc_req_data(struct srp_target_port *target)

@@ -586,7 +605,12 @@ static int srp_alloc_req_data(struct srp_target_port *target)
 
  	INIT_LIST_HEAD(&target->free_reqs);
 
-	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
+	target->req_ring = kzalloc(target->req_ring_size *
+				   sizeof(*target->req_ring), GFP_KERNEL);
+	if (!target->req_ring)
+		goto out;
+
+	for (i = 0; i < target->req_ring_size; ++i) {
  		req = &target->req_ring[i];
  		req->fmr_list = kmalloc(target->cmd_sg_cnt * sizeof(void *),
  					GFP_KERNEL);

@@ -810,7 +834,7 @@ static void srp_terminate_io(struct srp_rport *rport)
  	struct srp_target_port *target = rport->lld_data;
  	int i;
 
-	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
+	for (i = 0; i < target->req_ring_size; ++i) {
  		struct srp_request *req = &target->req_ring[i];
  		srp_finish_req(target, req, DID_TRANSPORT_FAILFAST << 16);
  	}

@@ -847,13 +871,13 @@ static int srp_rport_reconnect(struct srp_rport *rport)
  	else
  		srp_create_target_ib(target);
 
-	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
+	for (i = 0; i < target->req_ring_size; ++i) {
  		struct srp_request *req = &target->req_ring[i];
  		srp_finish_req(target, req, DID_RESET << 16);
  	}
 
  	INIT_LIST_HEAD(&target->free_tx);
-	for (i = 0; i < SRP_SQ_SIZE; ++i)
+	for (i = 0; i < target->queue_size; ++i)
  		list_add(&target->tx_ring[i]->list, &target->free_tx);
 
  	if (ret == 0)

@@ -1544,11 +1568,24 @@ err_unlock:
  	return SCSI_MLQUEUE_HOST_BUSY;
  }
 
+/*
+ * Note: the resources allocated in this function are freed in
+ * srp_free_target_ib().
+ */
  static int srp_alloc_iu_bufs(struct srp_target_port *target)
  {
  	int i;
 
-	for (i = 0; i < SRP_RQ_SIZE; ++i) {
+	target->rx_ring = kzalloc(target->queue_size * sizeof(*target->rx_ring),
+				  GFP_KERNEL);
+	if (!target->rx_ring)
+		goto err_no_ring;
+	target->tx_ring = kzalloc(target->queue_size * sizeof(*target->tx_ring),
+				  GFP_KERNEL);
+	if (!target->tx_ring)
+		goto err_no_ring;
+
+	for (i = 0; i < target->queue_size; ++i) {
  		target->rx_ring[i] = srp_alloc_iu(target->srp_host,
  						  target->max_ti_iu_len,
  						  GFP_KERNEL, DMA_FROM_DEVICE);

@@ -1556,7 +1593,7 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target)
  			goto err;
  	}
 
-	for (i = 0; i < SRP_SQ_SIZE; ++i) {
+	for (i = 0; i < target->queue_size; ++i) {
  		target->tx_ring[i] = srp_alloc_iu(target->srp_host,
  						  target->max_iu_len,
  						  GFP_KERNEL, DMA_TO_DEVICE);

@@ -1569,16 +1606,18 @@ static int srp_alloc_iu_bufs(struct srp_target_port *target)
  	return 0;
 
  err:
-	for (i = 0; i < SRP_RQ_SIZE; ++i) {
+	for (i = 0; i < target->queue_size; ++i) {
  		srp_free_iu(target->srp_host, target->rx_ring[i]);
-		target->rx_ring[i] = NULL;
-	}
-
-	for (i = 0; i < SRP_SQ_SIZE; ++i) {
  		srp_free_iu(target->srp_host, target->tx_ring[i]);
-		target->tx_ring[i] = NULL;
  	}
 
+
+err_no_ring:
+	kfree(target->tx_ring);
+	target->tx_ring = NULL;
+	kfree(target->rx_ring);
+	target->rx_ring = NULL;
+
  	return -ENOMEM;
  }
 

@@ -1629,6 +1668,9 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
  		target->scsi_host->can_queue
  			= min(target->req_lim - SRP_TSK_MGMT_SQ_SIZE,
  			      target->scsi_host->can_queue);
+		target->scsi_host->cmd_per_lun
+			= min_t(int, target->scsi_host->can_queue,
+				target->scsi_host->cmd_per_lun);
  	} else {
  		shost_printk(KERN_WARNING, target->scsi_host,
  			     PFX "Unhandled RSP opcode %#x\n", lrsp->opcode);

@@ -1636,7 +1678,7 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
  		goto error;
  	}
 
-	if (!target->rx_ring[0]) {
+	if (!target->rx_ring) {
  		ret = srp_alloc_iu_bufs(target);
  		if (ret)
  			goto error;

@@ -1656,7 +1698,7 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
  	if (ret)
  		goto error_free;
 
-	for (i = 0; i < SRP_RQ_SIZE; i++) {
+	for (i = 0; i < target->queue_size; i++) {
  		struct srp_iu *iu = target->rx_ring[i];
  		ret = srp_post_recv(target, iu);
  		if (ret)

@@ -1903,7 +1945,7 @@ static int srp_reset_device(struct scsi_cmnd *scmnd)
  	if (target->tsk_mgmt_status)
  		return FAILED;
 
-	for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {
+	for (i = 0; i < target->req_ring_size; ++i) {
  		struct srp_request *req = &target->req_ring[i];
  		if (req->scmnd && req->scmnd->device == scmnd->device)
  			srp_finish_req(target, req, DID_RESET << 16);

@@ -2096,9 +2138,9 @@ static struct scsi_host_template srp_template = {
  	.eh_host_reset_handler		= srp_reset_host,
  	.skip_settle_delay		= true,
  	.sg_tablesize			= SRP_DEF_SG_TABLESIZE,
-	.can_queue			= SRP_CMD_SQ_SIZE,
+	.can_queue			= SRP_DEFAULT_CMD_SQ_SIZE,
  	.this_id			= -1,
-	.cmd_per_lun			= SRP_CMD_SQ_SIZE,
+	.cmd_per_lun			= SRP_DEFAULT_CMD_SQ_SIZE,
  	.use_clustering			= ENABLE_CLUSTERING,
  	.shost_attrs			= srp_host_attrs
  };

@@ -2205,6 +2247,7 @@ enum {
  	SRP_OPT_SG_TABLESIZE	= 1 << 11,
  	SRP_OPT_COMP_VECTOR	= 1 << 12,
  	SRP_OPT_TL_RETRY_COUNT	= 1 << 13,
+	SRP_OPT_CAN_QUEUE	= 1 << 14,
  	SRP_OPT_ALL		= (SRP_OPT_ID_EXT	|
  				   SRP_OPT_IOC_GUID	|
  				   SRP_OPT_DGID		|

@@ -2227,6 +2270,7 @@ static const match_table_t srp_opt_tokens = {
  	{ SRP_OPT_SG_TABLESIZE,		"sg_tablesize=%u"	},
  	{ SRP_OPT_COMP_VECTOR,		"comp_vector=%u"	},
  	{ SRP_OPT_TL_RETRY_COUNT,	"tl_retry_count=%u"	},
+	{ SRP_OPT_CAN_QUEUE,		"can_queue=%d"		},
  	{ SRP_OPT_ERR,			NULL 			}
  };
 

@@ -2321,13 +2365,25 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
  			target->scsi_host->max_sectors = token;
  			break;
 
+		case SRP_OPT_CAN_QUEUE:
+			if (match_int(args, &token) || token < 1) {
+				pr_warn("bad can_queue parameter '%s'\n", p);
+				goto out;
+			}
+			target->scsi_host->can_queue = token;
+			target->queue_size = token + SRP_RSP_SQ_SIZE +
+					     SRP_TSK_MGMT_SQ_SIZE;
+			if (!(opt_mask & SRP_OPT_MAX_CMD_PER_LUN))
+				target->scsi_host->cmd_per_lun = token;
+			break;
+
  		case SRP_OPT_MAX_CMD_PER_LUN:
-			if (match_int(args, &token)) {
+			if (match_int(args, &token) || token < 1) {
  				pr_warn("bad max cmd_per_lun parameter '%s'\n",
  					p);
  				goto out;
  			}
-			target->scsi_host->cmd_per_lun = min(token, SRP_CMD_SQ_SIZE);
+			target->scsi_host->cmd_per_lun = token;
  			break;
 
  		case SRP_OPT_IO_CLASS:

@@ -2415,6 +2471,12 @@ static int srp_parse_options(const char *buf, struct srp_target_port *target)
  				pr_warn("target creation request is missing parameter '%s'\n",
  					srp_opt_tokens[i].pattern);
 
+	if (target->scsi_host->cmd_per_lun > target->scsi_host->can_queue
+	    && (opt_mask & SRP_OPT_MAX_CMD_PER_LUN))
+		pr_warn("cmd_per_lun = %d > can_queue = %d\n",
+			target->scsi_host->cmd_per_lun,
+			target->scsi_host->can_queue);
+
  out:
  	kfree(options);
  	return ret;

@@ -2453,11 +2515,14 @@ static ssize_t srp_create_target(struct device *dev,
  	target->sg_tablesize	= indirect_sg_entries ? : cmd_sg_entries;
  	target->allow_ext_sg	= allow_ext_sg;
  	target->tl_retry_count	= 7;
+	target->queue_size	= SRP_DEFAULT_QUEUE_SIZE;
 
  	ret = srp_parse_options(buf, target);
  	if (ret)
  		goto err;
 
+	target->req_ring_size = target->queue_size - SRP_TSK_MGMT_SQ_SIZE;
+
  	if (!srp_conn_unique(target->srp_host, target)) {
  		shost_printk(KERN_INFO, target->scsi_host,
  			     PFX "Already connected to target port with id_ext=%016llx;ioc_guid=%016llx;initiator_ext=%016llx\n",

drivers/infiniband/ulp/srp/ib_srp.h +5 -6

index 446b045..5756810 100644
@@ -57,14 +57,11 @@ enum {
  	SRP_MAX_LUN		= 512,
  	SRP_DEF_SG_TABLESIZE	= 12,
 
-	SRP_RQ_SHIFT    	= 6,
-	SRP_RQ_SIZE		= 1 << SRP_RQ_SHIFT,
-
-	SRP_SQ_SIZE		= SRP_RQ_SIZE,
+	SRP_DEFAULT_QUEUE_SIZE	= 1 << 6,
  	SRP_RSP_SQ_SIZE		= 1,
-	SRP_REQ_SQ_SIZE		= SRP_SQ_SIZE - SRP_RSP_SQ_SIZE,
  	SRP_TSK_MGMT_SQ_SIZE	= 1,
-	SRP_CMD_SQ_SIZE		= SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_SQ_SIZE,
+	SRP_DEFAULT_CMD_SQ_SIZE = SRP_DEFAULT_QUEUE_SIZE - SRP_RSP_SQ_SIZE -
+				  SRP_TSK_MGMT_SQ_SIZE,
 
  	SRP_TAG_NO_REQ		= ~0U,
  	SRP_TAG_TSK_MGMT	= 1U << 31,

@@ -156,6 +153,8 @@ struct srp_target_port {
  	char			target_name[32];
  	unsigned int		scsi_id;
  	unsigned int		sg_tablesize;
+	int			queue_size;
+	int			req_ring_size;
  	int			comp_vector;
  	int			tl_retry_count;
 

@@ -173,9 +172,9 @@ struct srp_target_port {
 
  	int			zero_req_lim;

Quoted 4 lines · Bart Van Assche

Certain storage configurations, e.g. a sufficiently large array of hard disks in a RAID configuration, need a queue depth above 64 to achieve optimal performance. Hence make the queue depth configurable. [ ... ]

I noticed this patch in your github and played with it, I agree that this patch is needed for a long time...

Question,
If srp now will allow larger queues while using a single global FMR pool of size 1024, isn't it more likely now that in stress environment srp will run out of FMRs to handle IO commands?
I mean that let's say that you have x scsi hosts with can_queue size of 512 (+-) and all of them are running IO stress, is it possible that all FMRs will be inuse and no FMR is available to register the next IO SG-list? Did you try out such a scenario?

I guess that in such a case IB core will return EAGAIN and SRP will return SCSI_MLQUEUE_HOST_BUSY.
I think it is a good Idea to move FMR pools to be per connection rather than a global pool, what do you think?

Quoted 9 lines · Sagi Grimberg

Question,
If srp now will allow larger queues while using a single global FMR pool of size 1024, isn't it more likely now that in stress environment srp will run out of FMRs to handle IO commands?
I mean that let's say that you have x scsi hosts with can_queue size of 512 (+-) and all of them are running IO stress, is it possible that all FMRs will be inuse and no FMR is available to register the next IO SG-list? Did you try out such a scenario?

Quoted 4 lines

I guess that in such a case IB core will return EAGAIN and SRP will return SCSI_MLQUEUE_HOST_BUSY.
I think it is a good Idea to move FMR pools to be per connection rather than a global pool, what do you think?

That makes sense to me. And as long as the above has not yet been implemented I'm fine with dropping patch 8/8 from this patch set.

Quoted 14 lines · Bart Van Assche

Quoted 13 lines · Sagi Grimberg

Question,
If srp now will allow larger queues while using a single global FMR pool of size 1024, isn't it more likely now that in stress environment srp will run out of FMRs to handle IO commands?
I mean that let's say that you have x scsi hosts with can_queue size of 512 (+-) and all of them are running IO stress, is it possible that all FMRs will be inuse and no FMR is available to register the next IO SG-list? Did you try out such a scenario?

I guess that in such a case IB core will return EAGAIN and SRP will return SCSI_MLQUEUE_HOST_BUSY.
I think it is a good Idea to move FMR pools to be per connection rather than a global pool, what do you think?

Quoted 2 lines

That makes sense to me. And as long as the above has not yet been implemented I'm fine with dropping patch 8/8 from this patch set.

Don't drop it; most configs won't have all that many connections and shouldn't have an issue; even those that do will only see a potential slowdown when running with everything at once.

We can address the FMR/BMME issues on top of this patch.

Changes since the previous patch series are:
- Rewrote the srp_tmo_valid() to improve readability (requested by Dave Dillow).
- The combination (reconnect_delay < 0 && fast_io_fail_tmo < 0 && dev_loss_tmo < 0) is now rejected as requested by Dave Dillow.
- Fixed a race between transport layer failure handling and device removal. This issue was reported by Vu Pham.

drivers/infiniband/ulp/srp/ib_srp.c static const match_table_t srp_opt_tokens = { · line 2273

Quoted 5 lines · Bart Van Assche

drivers/infiniband/ulp/srp/ib_srp.c +1 -0

@@ -2227,6 +2270,7 @@ static const match_table_t srp_opt_tokens = {
         { SRP_OPT_SG_TABLESIZE,         "sg_tablesize=%u"       },
         { SRP_OPT_COMP_VECTOR,          "comp_vector=%u"        },
         { SRP_OPT_TL_RETRY_COUNT,       "tl_retry_count=%u"     },
+       { SRP_OPT_CAN_QUEUE,            "can_queue=%d"          },

I'm pretty much OK with the patch, but since we're stuck with it going forward, I'd like to have a better externally visible name here -- queue_depth? max_queue? queue_size?

Otherwise,

Acked-by David Dillow <[email protected]>

If this name was not yet in use in any interface that is visible in user space, I would agree that we should come up with a better name. However, the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Quoted 8 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user space, I would agree that we should come up with a better name. However, the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

--

Quoted 13 lines · David Dillow

Quoted 8 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user space, I would agree that we should come up with a better name. However, the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

Quoted 2 lines

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

What the allow range for this queue size?
Default cmd_per_lun and can_queue with same value makes no sense to me. Could we bump can_queue to bigger value like 512?

Quoted 16 lines · Jack Wang

Quoted 15 lines · David Dillow

Quoted 10 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user
space, I would agree that we should come up with a better name. However,
the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

Quoted 6 lines

Quoted 2 lines

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

What the allow range for this queue size?
Default cmd_per_lun and can_queue with same value makes no sense to me. Could we bump can_queue to bigger value like 512?

Increasing the default value is only necessary when using a hard disk array at the target side. When using a single hard disk or an SSD at the target side the default value is fine.

Bart.

Quoted 24 lines · Bart Van Assche

Quoted 22 lines · Jack Wang

Quoted 9 lines · David Dillow

Quoted 8 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user
space, I would agree that we should come up with a better name. However,
the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

Quoted 8 lines

Quoted 2 lines

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

What the allow range for this queue size?
Default cmd_per_lun and can_queue with same value makes no sense to me. Could we bump can_queue to bigger value like 512?

Quoted 3 lines

Increasing the default value is only necessary when using a hard disk array at the target side. When using a single hard disk or an SSD at the target side the default value is fine.

Agree, from another side increasing default can_queue value is harmless for single harddisk/SSD, but will boot performance for multiple LUN attached to same host, so why not?

Quoted 29 lines · Jack Wang

Quoted 28 lines · Bart Van Assche

Quoted 26 lines · Jack Wang

Quoted 13 lines · David Dillow

Quoted 11 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user
space, I would agree that we should come up with a better name. However,
the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ?
An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Quoted 8 lines

Yes, I know it has been used before, but I'm torn between not furthering
a bad naming choice and consistency. Foolish consistency and all that...

I really don't like "can_queue", but I'll not complain if Roland decides
to take it as-is.

What the allow range for this queue size?
Default cmd_per_lun and can_queue with same value makes no sense to me. Could we bump can_queue to bigger value like 512?

Quoted 7 lines

Quoted 3 lines

Increasing the default value is only necessary when using a hard disk array at the target side. When using a single hard disk or an SSD at the target side the default value is fine.

Agree, from another side increasing default can_queue value is harmless for single harddisk/SSD, but will boot performance for multiple LUN attached to same host, so why not?

Increasing that parameter increases memory consumption for each path between initiator and target. In a setup where both the initiator and the target have multiple IB ports the number of paths can be large even when only a single LUN is exported by the target. That's why I'm not enthusiast about increasing the default value of the queue size.

Quoted 40 lines · Bart Van Assche

Quoted 30 lines · Jack Wang

Quoted 29 lines · Bart Van Assche

Quoted 27 lines · Jack Wang

Quoted 13 lines · David Dillow

Quoted 11 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user
space, I would agree that we should come up with a better name. However,
the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ?
An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Quoted 8 lines

Yes, I know it has been used before, but I'm torn between not furthering
a bad naming choice and consistency. Foolish consistency and all that...

I really don't like "can_queue", but I'll not complain if Roland decides
to take it as-is.

What the allow range for this queue size?
Default cmd_per_lun and can_queue with same value makes no sense to me.
Could we bump can_queue to bigger value like 512?

Quoted 8 lines

Quoted 4 lines

Increasing the default value is only necessary when using a hard disk array at the target side. When using a single hard disk or an SSD at the
target side the default value is fine.

Agree, from another side increasing default can_queue value is harmless for single harddisk/SSD, but will boot performance for multiple LUN attached to same host, so why not?

Quoted 5 lines

Increasing that parameter increases memory consumption for each path between initiator and target. In a setup where both the initiator and the target have multiple IB ports the number of paths can be large even when only a single LUN is exported by the target. That's why I'm not enthusiast about increasing the default value of the queue size.

(replying to my own e-mail)

Note: with the srp_daemon implementation available on github it's possible to set the can_queue parameter for all logins initiated by srp_daemon by adding something like the following at the end of /etc/srp_daemon.conf:

a can_queue=512

See also http://github.com/bvanassche/srptools.

Bart.

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

I tried your srp-ha branch in github,
echo string

Quoted 1 line

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

to add_target failed with

Quoted 2 lines

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

Do I miss something?

Quoted 9 lines · Jack Wang

I tried your srp-ha branch in github,
echo string

Quoted 2 lines

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

to add_target failed with

Quoted 2 lines

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

Do I miss something?

Hello Jack,

Have you already tried the same command with "echo -n" instead of "echo" ? If I remember correctly the SRP initiator doesn't like a newline at the end of the string written into the sysfs add_target attribute.

Hope this helps,

Bart.

   /*
1870                  * Reserve credits for task management so we don't
1871                  * bounce requests back to the SCSI mid-layer.
1872                  */

1873 target->scsi_host->can_queue
1874 = min(target->req_lim - SRP_TSK_MGMT_SQ_SIZE,
1875 target->scsi_host->can_queue);
1876 target->scsi_host->cmd_per_lun
1877 = min_t(int, target->scsi_host->can_queue,
1878 target->scsi_host->cmd_per_lun);

Quoted 13 lines · Bart Van Assche

Quoted 9 lines · Jack Wang

I tried your srp-ha branch in github,
echo string

Quoted 2 lines

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

to add_target failed with

Quoted 2 lines

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

Do I miss something?

Hello Jack,

Quoted 8 lines

Have you already tried the same command with "echo -n" instead of "echo" ? If I remember correctly the SRP initiator doesn't like a newline at the end of the string written into the sysfs add_target attribute.

Hope this helps,

Bart.

Thanks Bart,

You're right, that is the problem, with -n option I can set can_queue successfully.

But cat /sys/class/scsi_host/hostx/can_queue still show 63

Looks like it cause by the logic below:

Quoted 10 lines

    /*
1870                  * Reserve credits for task management so we don't
1871                  * bounce requests back to the SCSI mid-layer.
1872                  */

1873 target->scsi_host->can_queue
1874 = min(target->req_lim - SRP_TSK_MGMT_SQ_SIZE,
1875 target->scsi_host->can_queue);
1876 target->scsi_host->cmd_per_lun
1877 = min_t(int, target->scsi_host->can_queue,
1878 target->scsi_host->cmd_per_lun);

Could you give some hint on this, why we need to update can_queue and cmd_per_lun here?

Quoted 45 lines · Jack Wang

Quoted 14 lines · Bart Van Assche

Quoted 10 lines · Jack Wang

I tried your srp-ha branch in github,
echo string

Quoted 3 lines

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

to add_target failed with

Quoted 2 lines

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

Do I miss something?

Hello Jack,

Quoted 8 lines

Have you already tried the same command with "echo -n" instead of "echo" ? If I remember correctly the SRP initiator doesn't like a newline at the end of the string written into the sysfs add_target attribute.

Hope this helps,

Bart.

Thanks Bart,

You're right, that is the problem, with -n option I can set can_queue successfully.

But cat /sys/class/scsi_host/hostx/can_queue still show 63

Looks like it cause by the logic below:

Quoted 11 lines

    /*
1870                  * Reserve credits for task management so we don't
1871                  * bounce requests back to the SCSI mid-layer.
1872                  */

1873 target->scsi_host->can_queue
1874 = min(target->req_lim -
SRP_TSK_MGMT_SQ_SIZE,
1875 target->scsi_host->can_queue);
1876 target->scsi_host->cmd_per_lun
1877 = min_t(int, target->scsi_host->can_queue,
1878 target->scsi_host->cmd_per_lun);

Quoted 2 lines

Could you give some hint on this, why we need to update can_queue and cmd_per_lun here?

Hello Jack,

What is the value of target->req_lim on your setup ? The initiator limits the queue size to the target request limit because any attempt to queue more commands would cause the target to reply with "BUSY" anyway.

Bart.

cat /sys/class/scsi_host/host36/req_lim
64

Quoted 49 lines · Bart Van Assche

Quoted 45 lines · Jack Wang

Quoted 14 lines · Bart Van Assche

Quoted 10 lines · Jack Wang

I tried your srp-ha branch in github,
echo string

Quoted 3 lines

SRP2="id_ext=${THCA2_GUID},ioc_guid=${THCA2_GUID},dgid=${TGID_P2},pkey=${PKEY},service_id=${THCA2_GUID},can_queue=512"

to add_target failed with

Quoted 2 lines

ib_srp: unknown parameter or missing value 'can_queue=512

[  122.405385] ' in target creation request

Do I miss something?

Hello Jack,

Quoted 8 lines

Have you already tried the same command with "echo -n" instead of "echo" ? If I remember correctly the SRP initiator doesn't like a newline at the end of the string written into the sysfs add_target attribute.

Hope this helps,

Bart.

Thanks Bart,

You're right, that is the problem, with -n option I can set can_queue successfully.

But cat /sys/class/scsi_host/hostx/can_queue still show 63

Looks like it cause by the logic below:

Quoted 11 lines

    /*
1870                  * Reserve credits for task management so we don't
1871                  * bounce requests back to the SCSI mid-layer.
1872                  */

1873 target->scsi_host->can_queue
1874 = min(target->req_lim -
SRP_TSK_MGMT_SQ_SIZE,
1875 target->scsi_host->can_queue);
1876 target->scsi_host->cmd_per_lun
1877 = min_t(int, target->scsi_host->can_queue,
1878 target->scsi_host->cmd_per_lun);

Quoted 2 lines

Could you give some hint on this, why we need to update can_queue and cmd_per_lun here?

Quoted 9 lines

Hello Jack,

What is the value of target->req_lim on your setup ? The initiator limits the queue size to the target request limit because any attempt to queue more commands would cause the target to reply with "BUSY" anyway.

Bart.

Hello Bart,

Quoted 2 lines

cat /sys/class/scsi_host/host36/req_lim
64

I just checked srp spec, which do define such behaviour, I wonder in SCST/SRPT, how the request limit is chosen, is it report from low level hardware driver?

Thanks
Jack

#define SCST_MAX_TGT_DEV_COMMANDS 64

Hello Bart,

Quoted 2 lines

cat /sys/class/scsi_host/host36/req_lim
64

I just checked srp spec, which do define such behaviour, I wonder in SCST/SRPT, how the request limit is chosen, is it report from low level hardware driver?

Quoted 6 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user space, I would agree that we should come up with a better name. However, the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

Quoted 2 lines

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

Quoted 13 lines · David Dillow

Quoted 8 lines · Bart Van Assche

If this name was not yet in use in any interface that is visible in user space, I would agree that we should come up with a better name. However, the SCSI mid-layer already uses that name today to export the queue size. To me this looks like a good reason to use the name "can_queue" ? An example:

$ cat /sys/class/scsi_host/host93/can_queue
62

Yes, I know it has been used before, but I'm torn between not furthering a bad naming choice and consistency. Foolish consistency and all that...

Quoted 2 lines

I really don't like "can_queue", but I'll not complain if Roland decides to take it as-is.

The merge window has been closed early which means that I'll have to resend this patch series anyway. How about using the name "queue_size" instead ?