diff mbox

IB/srp: add change_queue_depth and change_queue_type support

Message ID 521B5DD5.3020804@profitbricks.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Jinpu Wang Aug. 26, 2013, 1:53 p.m. UTC 
Attached patch add change_queue_depth/change_queue_type function support
for srp driver, as what most modern scsi host driver does.

From 10445c9fd9e24d03269e43680bcd2504c713b622 Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang@profitbricks.com>
Date: Mon, 26 Aug 2013 15:50:03 +0200
Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support

Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   53
+++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

 			     u64 req_tag, unsigned int lun, u8 func)
@@ -1961,6 +2012,8 @@ static struct scsi_host_template srp_template = {
 	.slave_configure		= srp_slave_configure,
 	.info				= srp_target_info,
 	.queuecommand			= srp_queuecommand,
+	.change_queue_depth             = srp_change_queue_depth,
+	.change_queue_type              = srp_change_queue_type,
 	.eh_abort_handler		= srp_abort,
 	.eh_device_reset_handler	= srp_reset_device,
 	.eh_host_reset_handler		= srp_reset_host,

Comments

Bart Van Assche Aug. 27, 2013, 8:31 a.m. UTC | #1 
On 08/26/13 15:53, Jack Wang wrote:
> From: Jack Wang <jinpu.wang@profitbricks.com>
> Date: Mon, 26 Aug 2013 15:50:03 +0200
> Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support
>
> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>

Hello Jack,

When posting a Linux kernel patch it is not only expected that the 
commit message explains what is changed but also why these changes have 
been made. What made you look into adding dynamic queue depth support ? 
Had you perhaps run into a performance issue, an issue that is solved by 
this patch ? If so, did it occur with all storage types or only with 
certain storage types (e.g. hard disks or hard disk arrays) ? How much 
did this patch improve performance ?

> @@ -1697,6 +1698,56 @@ static int srp_cm_handler(struct ib_cm_id *cm_id,
> struct ib_cm_event *event)
>
>   	return 0;
>   }
> +/**
> + * srp_change_queue_type - changing device queue tag type

Please leave a blank line between the end of one function and the header 
of the next function.

> +
> +/**
> + * srp_change_queue_depth - setting device queue depth
> + * @sdev: scsi device struct
> + * @qdepth: requested queue depth
> + * @reason: SCSI_QDEPTH_DEFAULT/SCSI_QDEPTH_QFULL/SCSI_QDEPTH_RAMP_UP
> + * (see include/scsi/scsi_host.h for definition)
> + *
> + * Returns queue depth.
> + */
> +static int
> +srp_change_queue_depth(struct scsi_device *sdev, int qdepth, int reason)
> +{
> +	if (reason == SCSI_QDEPTH_DEFAULT || reason == SCSI_QDEPTH_RAMP_UP) {
> +		struct Scsi_Host *shost = sdev->host;
> +		int max_depth;

Nothing important, but I think in the Linux kernel there is a preference 
to declare variables at the outermost scope.

 > +		if (!sdev->tagged_supported)
 > +			max_depth = 1;

This code seems incorrect to me for the SRP protocol. In the SRP 
protocol, although there is no TCQ support, queue depths above one are 
supported.

I also have a more general remark. There is no TCQ support in the SRP 
protocol, which means that sdev->tagged_supported is always 0 (false). 
So my recommendation is to leave out all code that depends on "if 
(sdev->tagged_supported)" and to remove the "if 
(!sdev->tagged_supported)" tests.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jinpu Wang Aug. 27, 2013, 9:06 a.m. UTC | #2 
On 08/27/2013 10:31 AM, Bart Van Assche wrote:
> On 08/26/13 15:53, Jack Wang wrote:
>> From: Jack Wang <jinpu.wang@profitbricks.com>
>> Date: Mon, 26 Aug 2013 15:50:03 +0200
>> Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support
>>
>> Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
> 
> Hello Jack,
> 
> When posting a Linux kernel patch it is not only expected that the
> commit message explains what is changed but also why these changes have
> been made. What made you look into adding dynamic queue depth support ?
> Had you perhaps run into a performance issue, an issue that is solved by
> this patch ? If so, did it occur with all storage types or only with
> certain storage types (e.g. hard disks or hard disk arrays) ? How much
> did this patch improve performance ?
Hello Bart,

Thanks for informative comments.
The intention to add change_queue_type/change_queue_type support is we
saw some times srp run into scsi error handle when storage server(scst)
is busy, we want reduce the queue_depth at this point to avoid such
situation, as I understand about scsi core, it will try to auto change
queue depth according to scsi command results, when success ramp up
queue depth, and when storage return busy or queue full, it reduce the
queue depth.

The README of SCST suggests:
Unfortunately, currently SCST lacks dynamic I/O flow control, when the
queue depth on the target is dynamically decreased/increased based on
how slow/fast the backstorage speed comparing to the target link. So,
there are 6 possible actions, which you can do to workaround or fix this
issue in this case:

1. Ignore incoming task management (TM) commands. It's fine if there are
not too many of them, so average performance isn't hurt and the
corresponding device isn't getting put offline, i.e. if the backstorage
isn't too slow.

2. Decrease /sys/block/sdX/device/queue_depth on the initiator in case
if it's Linux (see below how) or/and SCST_MAX_TGT_DEV_COMMANDS constant
in scst_priv.h file until you stop seeing incoming TM commands.
ISCSI-SCST driver also has its own iSCSI specific parameter for that,
see its README file.

To decrease device queue depth on Linux initiators you can run command:

# echo Y >/sys/block/sdX/device/queue_depth

where Y is the new number of simultaneously queued commands, X - your
imported device letter, like 'a' for sda device. There are no special
limitations for Y value, it can be any value from 1 to possible maximum
(usually, 32), so start from dividing the current value on 2, i.e. set
16, if /sys/block/sdX/device/queue_depth contains 32.

3. Increase the corresponding timeout on the initiator. For Linux it is
located in
/sys/devices/platform/host*/session*/target*:0:0/*:0:0:1/timeout. It can
be done automatically by an udev rule. For instance, the following
rule will increase it to 300 seconds:

SUBSYSTEM=="scsi", KERNEL=="[0-9]*:[0-9]*", ACTION=="add",
ATTR{type}=="0|7|14", ATTR{timeout}="300"

By default, this timeout is 30 or 60 seconds, depending on your
distribution.

4. Try to avoid such seek intensive workloads.

5. Increase speed of the target's backstorage.

6. Implement in SCST dynamic I/O flow control. This will be an ultimate
solution. See "Dynamic I/O flow control" section on
http://scst.sourceforge.net/contributing.html page for possible
implementation idea.

I'm approaching the second one.


> 
>> @@ -1697,6 +1698,56 @@ static int srp_cm_handler(struct ib_cm_id *cm_id,
>> struct ib_cm_event *event)
>>
>>       return 0;
>>   }
>> +/**
>> + * srp_change_queue_type - changing device queue tag type
> 
> Please leave a blank line between the end of one function and the header
> of the next function.
> 
thanks, will do
>> +
>> +/**
>> + * srp_change_queue_depth - setting device queue depth
>> + * @sdev: scsi device struct
>> + * @qdepth: requested queue depth
>> + * @reason: SCSI_QDEPTH_DEFAULT/SCSI_QDEPTH_QFULL/SCSI_QDEPTH_RAMP_UP
>> + * (see include/scsi/scsi_host.h for definition)
>> + *
>> + * Returns queue depth.
>> + */
>> +static int
>> +srp_change_queue_depth(struct scsi_device *sdev, int qdepth, int reason)
>> +{
>> +    if (reason == SCSI_QDEPTH_DEFAULT || reason ==
>> SCSI_QDEPTH_RAMP_UP) {
>> +        struct Scsi_Host *shost = sdev->host;
>> +        int max_depth;
> 
> Nothing important, but I think in the Linux kernel there is a preference
> to declare variables at the outermost scope.
> 
sure, will do.
>> +        if (!sdev->tagged_supported)
>> +            max_depth = 1;
> 
> This code seems incorrect to me for the SRP protocol. In the SRP
> protocol, although there is no TCQ support, queue depths above one are
> supported.
> 
> I also have a more general remark. There is no TCQ support in the SRP
> protocol, which means that sdev->tagged_supported is always 0 (false).
> So my recommendation is to leave out all code that depends on "if
> (sdev->tagged_supported)" and to remove the "if
> (!sdev->tagged_supported)" tests.
> 
Good to know, thanks for share will fix this.

KR
Jack


> Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jinpu Wang Aug. 27, 2013, 4:39 p.m. UTC | #3 
snip
> This code seems incorrect to me for the SRP protocol. In the SRP
> protocol, although there is no TCQ support, queue depths above one are
> supported.
> 
> I also have a more general remark. There is no TCQ support in the SRP
> protocol, which means that sdev->tagged_supported is always 0 (false).
> So my recommendation is to leave out all code that depends on "if
> (sdev->tagged_supported)" and to remove the "if
> (!sdev->tagged_supported)" tests.
> 
> Bart.

Hello Bart,

I look into scsi core about above statement:
In drivers/scsi/scsi_scan.c:865:		

 if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
            !(*bflags & BLIST_NOTQ))
                sdev->tagged_supported = 1;
It check inquiry result byte 7 with cmdque bit.

inquiry result of a disk export through srp/srpt
sg_inq /dev/sdb
standard INQUIRY:
  PQual=0  Device_type=0  RMB=0  version=0x06  [SPC-4]
  [AERC=0]  [TrmTsk=0]  NormACA=0  HiSUP=0  Resp_data_format=2
  SCCS=0  ACC=0  TPGS=0  3PC=0  Protect=0  [BQue=0]
  EncServ=0  MultiP=1 (VS=0)  [MChngr=0]  [ACKREQQ=0]  Addr16=0
  [RelAdr=0]  WBus16=0  Sync=0  Linked=0  [TranDis=0]  CmdQue=1
  [SPI: Clocking=0x0  QAS=0  IUS=0]
    length=66 (0x42)   Peripheral device type: disk
 Vendor identification: SCST_FIO
 Product identification: vol0
 Product revision level:  300
 Unit serial number: 609bbc13

So the sdev_>tagged_supported will be set in this case

In srpr16 p38
The TASK ATTRIBUTE field is defined in table 21, it includes support for:

    head of queue
    ordered
    simple

Do I miss something?

Regards,
Jack

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Aug. 27, 2013, 4:46 p.m. UTC | #4 
On 08/27/13 18:39, Jack Wang wrote:
> I look into scsi core about above statement:
> In drivers/scsi/scsi_scan.c:865:		
>
>   if ((sdev->scsi_level >= SCSI_2) && (inq_result[7] & 2) &&
>              !(*bflags & BLIST_NOTQ))
>                  sdev->tagged_supported = 1;
> It check inquiry result byte 7 with cmdque bit.

Ah, thanks for looking that up. I had overlooked that statement. So it 
should be fine to keep all the code that depends on the value of 
sdev->tagged_supported.

Regards,

Bart.

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

Patch

From 10445c9fd9e24d03269e43680bcd2504c713b622 Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang@profitbricks.com>
Date: Mon, 26 Aug 2013 15:50:03 +0200
Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support

Signed-off-by: Jack Wang <jinpu.wang@profitbricks.com>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f93baf8..e2c2781 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -46,6 +46,7 @@ 
 #include <scsi/scsi.h>
 #include <scsi/scsi_device.h>
 #include <scsi/scsi_dbg.h>
+#include <scsi/scsi_tcq.h>
 #include <scsi/srp.h>
 #include <scsi/scsi_transport_srp.h>
 
@@ -1697,6 +1698,56 @@  static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 
 	return 0;
 }
+/**
+ * srp_change_queue_type - changing device queue tag type
+ * @sdev: scsi device struct
+ * @tag_type: requested tag type
+ *
+ * Returns queue tag type.
+ */
+static int
+srp_change_queue_type(struct scsi_device *sdev, int tag_type)
+{
+	if (sdev->tagged_supported) {
+		scsi_set_tag_type(sdev, tag_type);
+		if (tag_type)
+			scsi_activate_tcq(sdev, sdev->queue_depth);
+		else
+			scsi_deactivate_tcq(sdev, sdev->queue_depth);
+	} else
+		tag_type = 0;
+
+	return tag_type;
+}
+
+/**
+ * srp_change_queue_depth - setting device queue depth
+ * @sdev: scsi device struct
+ * @qdepth: requested queue depth
+ * @reason: SCSI_QDEPTH_DEFAULT/SCSI_QDEPTH_QFULL/SCSI_QDEPTH_RAMP_UP
+ * (see include/scsi/scsi_host.h for definition)
+ *
+ * Returns queue depth.
+ */
+static int
+srp_change_queue_depth(struct scsi_device *sdev, int qdepth, int reason)
+{
+	if (reason == SCSI_QDEPTH_DEFAULT || reason == SCSI_QDEPTH_RAMP_UP) {
+		struct Scsi_Host *shost = sdev->host;
+		int max_depth;
+		max_depth = shost->can_queue;
+		if (!sdev->tagged_supported)
+			max_depth = 1;
+		if (qdepth > max_depth)
+			qdepth = max_depth;
+		scsi_adjust_queue_depth(sdev, scsi_get_tag_type(sdev), qdepth);
+	} else if (reason == SCSI_QDEPTH_QFULL)
+		scsi_track_queue_full(sdev, qdepth);
+	else
+		return -EOPNOTSUPP;
+
+	return sdev->queue_depth;
+}
 
 static int srp_send_tsk_mgmt(struct srp_target_port *target,
 			     u64 req_tag, unsigned int lun, u8 func)
@@ -1961,6 +2012,8 @@  static struct scsi_host_template srp_template = {
 	.slave_configure		= srp_slave_configure,
 	.info				= srp_target_info,
 	.queuecommand			= srp_queuecommand,
+	.change_queue_depth             = srp_change_queue_depth,
+	.change_queue_type              = srp_change_queue_type,
 	.eh_abort_handler		= srp_abort,
 	.eh_device_reset_handler	= srp_reset_device,
 	.eh_host_reset_handler		= srp_reset_host,
-- 
1.7.10.4