[PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Jens Axboe]

Commit 1e6f2416044c0 changed the scsi sysfs 'queue_depth' code to
rejects depths higher than the scsi host template setting. But lots
of hosts set this to 1, and update the settings in the scsi host
when the controller/devices probing happens.

This breaks (at least) mpt2sas and mpt3sas runtime setting of queue
depth, returning EINVAL for all settings but '1'. And once it's set to
1, there's no way to go back up.

Cc: stable@kernel.org
Fixes: 1e6f2416044c0 "scsi: don't allow setting of queue_depth bigger than can_queue"
Signed-off-by: Jens Axboe <axboe@fb.com>
---
 drivers/scsi/scsi_sysfs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 1ac38e73df7e..9ad41168d26d 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -859,7 +859,7 @@ sdev_store_queue_depth(struct device *dev, struct device_attribute *attr,
 
 	depth = simple_strtoul(buf, NULL, 0);
 
-	if (depth < 1 || depth > sht->can_queue)
+	if (depth < 1 || depth > sdev->host->can_queue)
 		return -EINVAL;
 
 	retval = sht->change_queue_depth(sdev, depth);
-- 
2.4.1.168.g1ea28e1

-- 
Jens Axboe

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Martin K. Petersen]

>>>>> "Jens" == Jens Axboe <axboe@fb.com> writes:

Jens> Commit 1e6f2416044c0 changed the scsi sysfs 'queue_depth' code to
Jens> rejects depths higher than the scsi host template setting. But
Jens> lots of hosts set this to 1, and update the settings in the scsi
Jens> host when the controller/devices probing happens.

Yeah. We can't rely on the template for this.

Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

I wonder if we should start to gradually phase out these tuables in the
host template.  It's not really more complicated to set them directly
in the host anyway.

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Hannes Reinecke]

On 07/14/2015 09:13 AM, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> I wonder if we should start to gradually phase out these tuables in the
> host template.  It's not really more complicated to set them directly
> in the host anyway.

Fully agreed. I would vote for making the host template read-only
and modify all drivers to use the shost setting.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Jens Axboe]

On 07/14/2015 05:22 AM, Hannes Reinecke wrote:
> On 07/14/2015 09:13 AM, Christoph Hellwig wrote:
>> Looks good:
>>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>
>> I wonder if we should start to gradually phase out these tuables in the
>> host template.  It's not really more complicated to set them directly
>> in the host anyway.
>
> Fully agreed. I would vote for making the host template read-only
> and modify all drivers to use the shost setting.

Indeed, that would be a much saner choice. The settings in a 
(effectively already) read-only host template seems like somewhat of a 
relic.

-- 
Jens Axboe

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Hannes Reinecke]

On 07/14/2015 04:04 PM, Jens Axboe wrote:
> On 07/14/2015 05:22 AM, Hannes Reinecke wrote:
>> On 07/14/2015 09:13 AM, Christoph Hellwig wrote:
>>> Looks good:
>>>
>>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>>>
>>> I wonder if we should start to gradually phase out these tuables
>>> in the
>>> host template.  It's not really more complicated to set them
>>> directly
>>> in the host anyway.
>>
>> Fully agreed. I would vote for making the host template read-only
>> and modify all drivers to use the shost setting.
> 
> Indeed, that would be a much saner choice. The settings in a
> (effectively already) read-only host template seems like somewhat of
> a relic.
> 
Plus some drivers have the habit of modifying the template based on
the settings _for this particular_ device, which leads to
interesting results if you have several devices with _different_
settings ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Martin K. Petersen]

>>>>> "Jens" == Jens Axboe <axboe@fb.com> writes:

>> Fully agreed. I would vote for making the host template read-only and
>> modify all drivers to use the shost setting.

Jens> Indeed, that would be a much saner choice. The settings in a
Jens> (effectively already) read-only host template seems like somewhat
Jens> of a relic.

Agree 100%.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] fix host max depth checking for the 'queue_depth' sysfs interface

[Hannes Reinecke]

On 07/14/2015 04:20 PM, Martin K. Petersen wrote:
>>>>>> "Jens" == Jens Axboe <axboe@fb.com> writes:
> 
>>> Fully agreed. I would vote for making the host template read-only and
>>> modify all drivers to use the shost setting.
> 
> Jens> Indeed, that would be a much saner choice. The settings in a
> Jens> (effectively already) read-only host template seems like somewhat
> Jens> of a relic.
> 
> Agree 100%.
> 
While we're at it: why is the 'cmd_pool' attached to the host
template? Do all hosts with the same driver share a common pool?
(I sincerely do hope not ...)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html