make sysfs scsi queue_depth read/write [patch]

make sysfs scsi queue_depth read/write [patch]

[Miquel van Smoorenburg]

[please keep me Cc:'ed, I'm not on linux-scsi]

I have been testing with a 3ware controller over the last few weeks.
It turns out that in RAID5 mode, it performs much better if the
queue depth is limited to 64 instead of the default 254.

So I patched the 3ware driver to make this tunable through a kernel/module
command line option, documented my findings, and send all that
stuff to the (very responsive) 3ware folks who are now looking at it
to see if this fixes a problem or just the symptoms of a
bigger problem.

Then it occured to me that it would be much easier to set this
limit (which is per device, not per host anyway) though sysfs.
Since a lot of SCSI drivers have options to set the queue depth,
it probably fills a need.

Below is a patch that makes /sys/block/<dev>/device/queue_depth
a read/write variable. Minimum limit is 1, maximum limit is the
value set in host->cmd_per_lun, so it should be perfectly safe.

Since the patch is trivial and it fixes a real problem (in my case,
tuning this gains 200% performance) please consider this for
2.6.NEXT (or 2.6.mm-next).

Thanks,

Mike.

--- linux-2.6.1/drivers/scsi/scsi_sysfs.c.ORIG	2004-01-09 07:59:10.000000000 +0100
+++ linux-2.6.1/drivers/scsi/scsi_sysfs.c	2004-01-14 12:26:23.000000000 +0100
@@ -259,7 +259,6 @@
  * Create the actual show/store functions and data structures.
  */
 sdev_rd_attr (device_blocked, "%d\n");
-sdev_rd_attr (queue_depth, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
 sdev_rd_attr (vendor, "%.8s\n");
@@ -283,6 +282,22 @@
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
+sdev_show_function(queue_depth, "%d\n")
+static ssize_t sdev_store_queue_depth(struct device *dev, const char *buf,
+				 size_t count)
+{
+	int val;
+	struct scsi_device *sdev;
+	if (sscanf(buf, "%d", &val) != 1)
+		return -EINVAL;
+	sdev = to_scsi_device(dev);
+	if (!sdev || val < 1 || val > sdev->host->cmd_per_lun)
+		return -EINVAL;
+	sdev->queue_depth = val;
+	return count;
+}
+static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth, sdev_store_queue_depth)
+
 /* Default template for device attributes.  May NOT be modified */
 static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
 	&dev_attr_device_blocked,

Re: make sysfs scsi queue_depth read/write [patch]

[Christoph Hellwig]

Patch looks okay, but the implementation would be

> +	struct scsi_device *sdev;
> +	if (sscanf(buf, "%d", &val) != 1)
> +		return -EINVAL;
> +	sdev = to_scsi_device(dev);
> +	if (!sdev

Can't be NULL.

|| val < 1 || val > sdev->host->cmd_per_lun)
> +		return -EINVAL;
> +	sdev->queue_depth = val;

This needs some locking.  Currently we have a single global lock protecting
queue_depth updates, but none for reads.   (which looks a bit racy already,
but no need to make it worse..)

Re: make sysfs scsi queue_depth read/write [patch]

[Miquel van Smoorenburg]

According to Christoph Hellwig:
> Patch looks okay [...]
> This needs some locking.  Currently we have a single global lock protecting
> queue_depth updates, but none for reads.   (which looks a bit racy already,
> but no need to make it worse..)

Well, queue_depth is 32 bits, I think reading/writing to that is
atomic on most/all CPUs. We're not doing read/modify/write, just
an assignment, so it probably doesn't need locking.

Anyway, how about this:

--- linux-2.6.1/drivers/scsi/scsi.c.ORIG	2004-01-14 03:52:53.000000000 +0100
+++ linux-2.6.1/drivers/scsi/scsi.c	2004-01-14 13:12:37.000000000 +0100
@@ -889,6 +892,8 @@
 			sdev->ordered_tags = 0;
 			sdev->simple_tags = 1;
 			break;
+		case -1: /* Just set queue depth */
+			break;
 		default:
 			printk(KERN_WARNING "(scsi%d:%d:%d:%d) "
 				"scsi_adjust_queue_depth, bad queue type, "
--- linux-2.6.1/drivers/scsi/scsi_sysfs.c.ORIG	2004-01-09 07:59:10.000000000 +0100
+++ linux-2.6.1/drivers/scsi/scsi_sysfs.c	2004-01-14 13:13:47.000000000 +0100
@@ -259,7 +259,6 @@
  * Create the actual show/store functions and data structures.
  */
 sdev_rd_attr (device_blocked, "%d\n");
-sdev_rd_attr (queue_depth, "%d\n");
 sdev_rd_attr (type, "%d\n");
 sdev_rd_attr (scsi_level, "%d\n");
 sdev_rd_attr (vendor, "%.8s\n");
@@ -283,6 +282,22 @@
 };
 static DEVICE_ATTR(delete, S_IWUSR, NULL, sdev_store_delete);
 
+sdev_show_function(queue_depth, "%d\n")
+static ssize_t sdev_store_queue_depth(struct device *dev, const char *buf,
+				 size_t count)
+{
+	int val;
+	struct scsi_device *sdev;
+	if (sscanf(buf, "%d", &val) != 1)
+		return -EINVAL;
+	sdev = to_scsi_device(dev);
+	if (val < 1 || val > sdev->host->cmd_per_lun)
+		return -EINVAL;
+	scsi_adjust_queue_depth(sdev, -1, val);
+	return count;
+}
+static DEVICE_ATTR(queue_depth, S_IRUGO | S_IWUSR, sdev_show_queue_depth, sdev_store_queue_depth)
+
 /* Default template for device attributes.  May NOT be modified */
 static struct device_attribute *scsi_sysfs_sdev_attrs[] = {
 	&dev_attr_device_blocked,


Mike.

Re: make sysfs scsi queue_depth read/write [patch]

[James Bottomley]

On Wed, 2004-01-14 at 06:41, Miquel van Smoorenburg wrote:
> I have been testing with a 3ware controller over the last few weeks.
> It turns out that in RAID5 mode, it performs much better if the
> queue depth is limited to 64 instead of the default 254.
> 
> So I patched the 3ware driver to make this tunable through a kernel/module
> command line option, documented my findings, and send all that
> stuff to the (very responsive) 3ware folks who are now looking at it
> to see if this fixes a problem or just the symptoms of a
> bigger problem.
> 
> Then it occured to me that it would be much easier to set this
> limit (which is per device, not per host anyway) though sysfs.
> Since a lot of SCSI drivers have options to set the queue depth,
> it probably fills a need.
> 
> Below is a patch that makes /sys/block/<dev>/device/queue_depth
> a read/write variable. Minimum limit is 1, maximum limit is the
> value set in host->cmd_per_lun, so it should be perfectly safe.
> 
> Since the patch is trivial and it fixes a real problem (in my case,
> tuning this gains 200% performance) please consider this for
> 2.6.NEXT (or 2.6.mm-next).

In 2.6 this isn't the correct way to do it.

The problem (which your patch doesn't address) is that a queue often has
driver allocated resources behind it, so the driver needs to participate
in queue depth adjustment.  You account for this in the check to make
sure the user isn't raising the depth, but nothing frees the (now
useless) queue resources the driver potentially has.

The correct fix is to do a queue depth attribute override in the 3ware
driver.  For an example of exactly how this is done, look in the
53c700.c driver.  It actually has no driver backed queue resources, so
it does essentially what your second patch does...If the 3ware is in the
same position (i.e. can stand a depth increase) then I've no problem
with moving this store_queue_depth routine into scsi_sysfs and making it
globally available for drivers that want a settable queue depth.

James

Re: make sysfs scsi queue_depth read/write [patch]

[Miquel van Smoorenburg]

On Wed, 14 Jan 2004 15:56:24, James Bottomley wrote:
> On Wed, 2004-01-14 at 06:41, Miquel van Smoorenburg wrote:
>
> > Below is a patch that makes /sys/block/<dev>/device/queue_depth
> > a read/write variable. Minimum limit is 1, maximum limit is the
> > value set in host->cmd_per_lun, so it should be perfectly safe.
> 
> In 2.6 this isn't the correct way to do it.
> 
> The problem (which your patch doesn't address) is that a queue often has
> driver allocated resources behind it, so the driver needs to participate
> in queue depth adjustment.  You account for this in the check to make
> sure the user isn't raising the depth, but nothing frees the (now
> useless) queue resources the driver potentially has.

Ah, I see. That is a problem. Thanks for pointing that out.

> The correct fix is to do a queue depth attribute override in the 3ware
> driver.  For an example of exactly how this is done, look in the
> 53c700.c driver.  It actually has no driver backed queue resources, so
> it does essentially what your second patch does...If the 3ware is in the
> same position (i.e. can stand a depth increase) then I've no problem
> with moving this store_queue_depth routine into scsi_sysfs and making it
> globally available for drivers that want a settable queue depth.

Oh, that looks pretty cool indeed. Nice infrastructure! That's easily
implementable in 3w-xxxx.c, so that's what I'll do.

Thanks!

Mike.