| Message ID | 20150713142439.GA14427@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>>>> "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>
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.
--
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
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
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.
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
>>>>> "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%.
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
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);
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(-)