IB/srp: add change_queue_depth and change_queue_type support

Attached patch add change_queue_depth/change_queue_type function support for srp driver, as what most modern scsi host driver does.

From: Jack Wang <[email protected]>
Date: Mon, 26 Aug 2013 15:50:03 +0200
Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support

---
 drivers/infiniband/ulp/srp/ib_srp.c |   53
+++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

b/drivers/infiniband/ulp/srp/ib_srp.c
index f93baf8..e2c2781 100644

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

From: Jack Wang <[email protected]>
Date: Mon, 26 Aug 2013 15:50:03 +0200
Subject: [PATCH] IB/srp: add change_queue_depth/change_queue_type support

---
 drivers/infiniband/ulp/srp/ib_srp.c |   53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

1.7.10.4

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.

thanks, will do

sure, will do.

Good to know, thanks for share will fix this.

KR
Jack

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

snip

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html