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

[PATCH v2 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.

-- 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  | 27 +++++++++++++++++------
 include/scsi/scsi_device.h |  1 +
 4 files changed, 77 insertions(+), 6 deletions(-)

-- 
2.13.7

[PATCH v2 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>
---
 drivers/scsi/scsi.c       | 22 ++++++++++++++++++++++
 drivers/scsi/scsi_priv.h  |  1 +
 drivers/scsi/scsi_sysfs.c |  9 +++------
 3 files changed, 26 insertions(+), 6 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..954f68b002cb 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -982,22 +982,19 @@ 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)
-		return -EINVAL;
+	if (!sdev->host->hostt->change_queue_depth)
+		return -ENOTSUPP;
 
 	depth = simple_strtoul(buf, NULL, 0);
 
 	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 v2 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>
---
 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..f5bb8ce4661f 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 -ENOTSUPP;
+
+	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 v2 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>
---
 drivers/scsi/scsi_sysfs.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 954f68b002cb..85dab4867eed 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -368,6 +368,23 @@ 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);
+	int depth, retval;
+
+	depth = simple_strtoul(buf, NULL, 0);
+
+	retval = shost_change_max_queue_depths(shost, depth);
+
+	return (retval < 0) ? retval : count;
+}
+
+static DEVICE_ATTR(max_device_queue_depth, S_IWUSR, 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 +428,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 v2 1/3] scsi: refactor sdev lun queue depth setting via sysfs

[Bart Van Assche]

On 2020-01-23 14:21, 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 v2 2/3] scsi: add shost helper to set max queue depth on all of its devices

[Bart Van Assche]

On 2020-01-23 14:21, James Smart wrote:
>  /**
> + * shost_change_max_queue_depths -  helper to walk all devices on a
                                     ^^
                  a single space here is probably sufficient?

> +	if (!shost->hostt->change_queue_depth)
> +		return -ENOTSUPP;

ENOTSUPP is defined in include/linux/errno.h so that's a kernel-internal
error code. I think only error codes from
include/uapi/asm-generic/errno*.h should be returned to user space.

> +	if (depth < 1 || depth > shost->can_queue)
> +		return -EINVAL;

Thanks,

Bart.

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

[Bart Van Assche]

On 2020-01-23 14:21, James Smart wrote:
> +	depth = simple_strtoul(buf, NULL, 0);

From Documentation/process/deprecated.rst:

The :c:func:`simple_strtol`, :c:func:`simple_strtoll`,
:c:func:`simple_strtoul`, and :c:func:`simple_strtoull` functions
explicitly ignore overflows, which may lead to unexpected results
in callers. The respective :c:func:`kstrtol`, :c:func:`kstrtoll`,
:c:func:`kstrtoul`, and :c:func:`kstrtoull` functions tend to be the
correct replacements, though note that those require the string to be
NUL or newline terminated.

Did checkpatch recommend to use kstrtoul() instead?

> +	return (retval < 0) ? retval : count;

Are the parentheses necessary in this expression?

Thanks,

Bart.