[PATCH v3 0/3] scsi: add attribute to set lun queue depth on all luns on shost

[PATCH v3 0/3] scsi: add attribute to set lun queue depth on all luns on shost

[James Smart]

There has been a desire to set the lun queue depth on all luns on an
shost. Today that is done by an external script looping through
discovered sdevs and set an sdev attribute. The desire is to have a
single shost attribute that performs this work removing the requirement
for scripting.

This patch:
- Refactors the existing sdev change max queue depth attribute so the
  meat of it becomes a service routine.
- Creates a shost helper routine, lldd callable, which cycles though
  all sdevs on the shost and changes their max queue depth. Uses the
  new service routine.
- Adds a new shost attribute which calls the new shost helper routine.

v2:
Reworked the patch with recommendations on error returns and breaking into
smaller patches. I also had a request to make the shost change routine to
be lldd callable. So switched up how that was implemented as well.

v3:
Revert use of -ENOTSUPP. Use -EINVAL or -ENXIO instead.
Clean up spacing and unnecessary parens.
Use kstrtouint
Specify attribute permissions in octl

-- james


James Smart (3):
  scsi: refactor sdev lun queue depth setting via sysfs
  scsi: add shost helper to set max queue depth on all of its devices
  scsi: add shost attribute to set max queue depth on all devices on the
    shost

 drivers/scsi/scsi.c        | 54 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_priv.h   |  1 +
 drivers/scsi/scsi_sysfs.c  | 28 +++++++++++++++++++-----
 include/scsi/scsi_device.h |  1 +
 4 files changed, 79 insertions(+), 5 deletions(-)

-- 
2.13.7

[PATCH v3 1/3] scsi: refactor sdev lun queue depth setting via sysfs

[James Smart]

In preparation for allowing other attributes and routines to change
the current and max lun queue depth on an sdev, refactor the sysfs
sdev attribute change routine. The refactoring creates a new scsi-internal
routine, scsi_change_max_queue_depth(), which changes a devices current
and max queue value.

The new routine is placed next to the routine, scsi_change_queue_depth(),
which is used by most lldds to implement a queue depth change.

Signed-off-by: James Smart <jsmart2021@gmail.com>

---
v3:
  revert -ENOTSUPP status on shost template check back to -EINVAL
---
 drivers/scsi/scsi.c       | 22 ++++++++++++++++++++++
 drivers/scsi/scsi_priv.h  |  1 +
 drivers/scsi/scsi_sysfs.c |  7 ++-----
 3 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 930e4803d888..195c0b11260a 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -249,6 +249,28 @@ int scsi_change_queue_depth(struct scsi_device *sdev, int depth)
 }
 EXPORT_SYMBOL(scsi_change_queue_depth);
 
+/*
+ * scsi_change_max_queue_depth - change the max queue depth for a device.
+ * @sdev: SCSI Device in question
+ * @depth: number of commands allowed to be queued to the driver
+ *
+ * Calls the device's transport to validate and change the queue depth,
+ * then stores the new maximum on the device.
+ */
+int
+scsi_change_max_queue_depth(struct scsi_device *sdev, int depth)
+{
+	int retval;
+
+	retval = sdev->host->hostt->change_queue_depth(sdev, depth);
+	if (retval < 0)
+		return retval;
+
+	sdev->max_queue_depth = sdev->queue_depth;
+
+	return 0;
+}
+
 /**
  * scsi_track_queue_full - track QUEUE_FULL events to adjust queue depth
  * @sdev: SCSI Device in question
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 3bff9f7aa684..5c288cf3ae64 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -41,6 +41,7 @@ static inline void scsi_log_send(struct scsi_cmnd *cmd)
 static inline void scsi_log_completion(struct scsi_cmnd *cmd, int disposition)
 	{ };
 #endif
+int scsi_change_max_queue_depth(struct scsi_device *sdev, int depth);
 
 /* scsi_devinfo.c */
 
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 677b5c5403d2..d4e9ad9a6f18 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -982,9 +982,8 @@ sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
 {
 	int depth, retval;
 	struct scsi_device *sdev = to_scsi_device(dev);
-	struct scsi_host_template *sht = sdev->host->hostt;
 
-	if (!sht->change_queue_depth)
+	if (!sdev->host->hostt->change_queue_depth)
 		return -EINVAL;
 
 	depth = simple_strtoul(buf, NULL, 0);
@@ -992,12 +991,10 @@ sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
 	if (depth < 1 || depth > sdev->host->can_queue)
 		return -EINVAL;
 
-	retval = sht->change_queue_depth(sdev, depth);
+	retval = scsi_change_max_queue_depth(sdev, depth);
 	if (retval < 0)
 		return retval;
 
-	sdev->max_queue_depth = sdev->queue_depth;
-
 	return count;
 }
 sdev_show_function(queue_depth, "%d\n");
-- 
2.13.7

[PATCH v3 2/3] scsi: add shost helper to set max queue depth on all of its devices

[James Smart]

Create a helper routine to loop through all devices on an shost and
change their current and maximum queue depth. The helper is created
such that is lldd callable.

Signed-off-by: James Smart <jsmart2021@gmail.com>

---
v3:
 fix spaces
 change ENOTSUPP to ENXIO
---
 drivers/scsi/scsi.c        | 32 ++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  1 +
 2 files changed, 33 insertions(+)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 195c0b11260a..86db018bacb2 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -272,6 +272,38 @@ scsi_change_max_queue_depth(struct scsi_device *sdev, int depth)
 }
 
 /**
+ * shost_change_max_queue_depths - helper to walk all devices on a
+ *         shost and change their max queue depth.
+ * @shost: shost whose devices we want to iterate over.
+ * @depth: number of commands allowed to be queued to the driver
+ *
+ * Validates the shost allows a change of queue depth, the value is valid,
+ * then traverses over all devices and sets their maximum queue depth.
+ */
+int shost_change_max_queue_depths(struct Scsi_Host *shost, int depth)
+{
+	struct scsi_device *sdev;
+	int retval;
+
+	if (!shost->hostt->change_queue_depth)
+		return -ENXIO;
+
+	if (depth < 1 || depth > shost->can_queue)
+		return -EINVAL;
+
+	shost_for_each_device(sdev, shost) {
+		retval = scsi_change_max_queue_depth(sdev, depth);
+		if (retval != 0)
+			sdev_printk(KERN_INFO, sdev,
+				"failed to set queue depth to %d (err %d)",
+				depth, retval);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(shost_change_max_queue_depths);
+
+/**
  * scsi_track_queue_full - track QUEUE_FULL events to adjust queue depth
  * @sdev: SCSI Device in question
  * @depth: Current number of outstanding SCSI commands on this device,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index f8312a3e5b42..38c4b97fc1b8 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -392,6 +392,7 @@ extern struct scsi_device *__scsi_iterate_devices(struct Scsi_Host *,
 
 extern int scsi_change_queue_depth(struct scsi_device *, int);
 extern int scsi_track_queue_full(struct scsi_device *, int);
+extern int shost_change_max_queue_depths(struct Scsi_Host *shost, int depth);
 
 extern int scsi_set_medium_removal(struct scsi_device *, char);
 
-- 
2.13.7

[PATCH v3 3/3] scsi: add shost attribute to set max queue depth on all devices on the shost

[James Smart]

This patch adds an shost attribute, max_device_queue_depth, that will
cycle through all devices on the shost and change their current and max
queue depth to the new value.

Signed-off-by: James Smart <jsmart2021@gmail.com>

---
v3:
 use kstrtouint
 specify permissions in octl, not by mnemonic
 remove unnecessary parens
---
 drivers/scsi/scsi_sysfs.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index d4e9ad9a6f18..98dbbfb6397b 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -368,6 +368,26 @@ store_shost_eh_deadline(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR(eh_deadline, S_IRUGO | S_IWUSR, show_shost_eh_deadline, store_shost_eh_deadline);
 
+static ssize_t
+store_host_max_device_queue_depth(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct Scsi_Host *shost = class_to_shost(dev);
+	unsigned int depth;
+	int retval;
+
+	retval = kstrtouint(buf, 10, &depth);
+	if (retval)
+		return -EINVAL;
+
+	retval = shost_change_max_queue_depths(shost, depth);
+
+	return retval < 0 ? retval : count;
+}
+
+static DEVICE_ATTR(max_device_queue_depth, 0200, NULL,
+		   store_host_max_device_queue_depth);
+
 shost_rd_attr(unique_id, "%u\n");
 shost_rd_attr(cmd_per_lun, "%hd\n");
 shost_rd_attr(can_queue, "%hd\n");
@@ -411,6 +431,7 @@ static struct attribute *scsi_sysfs_shost_attrs[] = {
 	&dev_attr_prot_guard_type.attr,
 	&dev_attr_host_reset.attr,
 	&dev_attr_eh_deadline.attr,
+	&dev_attr_max_device_queue_depth.attr,
 	NULL
 };
 
-- 
2.13.7

Re: [PATCH v3 3/3] scsi: add shost attribute to set max queue depth on all devices on the shost

[Bart Van Assche]

On 2020-01-24 15:01, James Smart wrote:
> This patch adds an shost attribute, max_device_queue_depth, that will
> cycle through all devices on the shost and change their current and max
> queue depth to the new value.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3 2/3] scsi: add shost helper to set max queue depth on all of its devices

[Bart Van Assche]

On 2020-01-24 15:01, James Smart wrote:
> Create a helper routine to loop through all devices on an shost and
> change their current and maximum queue depth. The helper is created
> such that is lldd callable.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3 1/3] scsi: refactor sdev lun queue depth setting via sysfs

[Bart Van Assche]

On 2020-01-24 15:01, James Smart wrote:
> In preparation for allowing other attributes and routines to change
> the current and max lun queue depth on an sdev, refactor the sysfs
> sdev attribute change routine. The refactoring creates a new scsi-internal
> routine, scsi_change_max_queue_depth(), which changes a devices current
> and max queue value.
> 
> The new routine is placed next to the routine, scsi_change_queue_depth(),
> which is used by most lldds to implement a queue depth change.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v3 0/3] scsi: add attribute to set lun queue depth on all luns on shost

[Martin K. Petersen]

James,

> There has been a desire to set the lun queue depth on all luns on an
> shost. Today that is done by an external script looping through
> discovered sdevs and set an sdev attribute. The desire is to have a
> single shost attribute that performs this work removing the
> requirement for scripting.

I'd like you to elaborate a bit on this.

 - Why is scripting or adding a udev rule inadequate?

 - Why is there a requirement to statically clamp the queue depth
   instead of letting the device manage it?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v3 0/3] scsi: add attribute to set lun queue depth on all luns on shost

[James Smart]

On 2/4/2020 6:56 PM, Martin K. Petersen wrote:
> 
> James,
> 
>> There has been a desire to set the lun queue depth on all luns on an
>> shost. Today that is done by an external script looping through
>> discovered sdevs and set an sdev attribute. The desire is to have a
>> single shost attribute that performs this work removing the
>> requirement for scripting.
> 
> I'd like you to elaborate a bit on this.
> 
>   - Why is scripting or adding a udev rule inadequate?

Simply put, admins don't want to create them, mod the system for them, 
nor concern themselves with finding/changing each of the lun devices 
that connect to a particular port. They have been comfortable changing 
the driver's initial lun queue depth via module parameter. Given that 
was at driver load, it changed everything at time of initial discovery 
so it was fine. But, if the system won't boot for days/weeks, they want 
to do something as simple as the module parameter and with only "1 echo 
command". Thus we wanted to make the parameter be rw rather than ro. 
However, the only interface available to the lldd was 
scsi_change_queue_depth(), which changes the depth but does not change 
the devices max value (which it does do if written via the per-device 
attribute). So although the now writeable attribute allows the driver 
value to change, it would only be applied to storage devices discovered 
after the change. Existing devices would not have their max changed 
unless the per-device attribute were changed.  This new interface gave 
the lldd a new routine, which would find all the devices and apply the 
new max/value to the devices - as if the per-device attributes had been 
called.

> 
>   - Why is there a requirement to statically clamp the queue depth
>     instead of letting the device manage it?

You are misreading it, and perhaps my description led things astray. It 
doesn't "clamp" it at a fixed/unchangable depth. It sets the max to a 
new value and changes the current queue depth to that new value. These 
are the same actions that the per-device attribute does if written to. 
The management of queue depth depth beyond that point is the same as it 
was - meaning queue fulls ramp it down, there is ramp up, and so on. So 
it is the device managing it, just with perhaps a small blip if it 
actually raised vs it's current level, or a pause if its current level 
was higher and it drains down to the new levels.

-- james