[PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[John Garry]

Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
exceed shost.can_queue.

However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
initial sdev queue depth greater than can_queue.

Stop this happened by capping initial sdev queue depth at can_queue.

Signed-off-by: John Garry <john.garry@huawei.com>
---
Topic originally discussed at:
https://lore.kernel.org/linux-scsi/85dec8eb-8eab-c7d6-b0fb-5622747c5499@interlog.com/T/#m5663d0cac657d843b93d0c9a2374f98fc04384b9

Last idea there was to error/warn in scsi_add_host() for cmd_per_lun >
can_queue. However, such a shost driver could still configure the sdev
queue depth to be sound value at .slave_configure callback, so now thinking
the orig patch better.

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 9f1b7f3c650a..8de2f830bcdc 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -277,7 +277,11 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	WARN_ON_ONCE(!blk_get_queue(sdev->request_queue));
 	sdev->request_queue->queuedata = sdev;
 
-	depth = sdev->host->cmd_per_lun ?: 1;
+	if (sdev->host->cmd_per_lun)
+		depth = min_t(unsigned int, sdev->host->cmd_per_lun,
+			      sdev->host->can_queue);
+	else
+		depth = 1;
 
 	/*
 	 * Use .can_queue as budget map's depth because we have to
-- 
2.26.2

Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[Ming Lei]

On Tue, Apr 20, 2021 at 12:06:24AM +0800, John Garry wrote:
> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> exceed shost.can_queue.
> 
> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
> initial sdev queue depth greater than can_queue.
> 
> Stop this happened by capping initial sdev queue depth at can_queue.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
> Topic originally discussed at:
> https://lore.kernel.org/linux-scsi/85dec8eb-8eab-c7d6-b0fb-5622747c5499@interlog.com/T/#m5663d0cac657d843b93d0c9a2374f98fc04384b9
> 
> Last idea there was to error/warn in scsi_add_host() for cmd_per_lun >

No, that isn't my suggestion.

> can_queue. However, such a shost driver could still configure the sdev
> queue depth to be sound value at .slave_configure callback, so now thinking
> the orig patch better.

As I mentioned last time, why can't we fix ->cmd_per_lun in
scsi_add_host() using .can_queue?

-- 
Ming

Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[John Garry]

On 20/04/2021 01:02, Ming Lei wrote:
> On Tue, Apr 20, 2021 at 12:06:24AM +0800, John Garry wrote:
>> Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
>> exceed shost.can_queue.
>>
>> However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
>> initial sdev queue depth greater than can_queue.
>>
>> Stop this happened by capping initial sdev queue depth at can_queue.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>> Topic originally discussed at:
>> https://lore.kernel.org/linux-scsi/85dec8eb-8eab-c7d6-b0fb-5622747c5499@interlog.com/T/#m5663d0cac657d843b93d0c9a2374f98fc04384b9
>>
>> Last idea there was to error/warn in scsi_add_host() for cmd_per_lun >
> 

Hi Ming,

> No, that isn't my suggestion.

Right, it was what I mentioned.

> 
>> can_queue. However, such a shost driver could still configure the sdev
>> queue depth to be sound value at .slave_configure callback, so now thinking
>> the orig patch better.
> 
> As I mentioned last time, why can't we fix ->cmd_per_lun in
> scsi_add_host() using .can_queue?
> 

I would rather not change the values which are provided from the driver. 
I would rather take the original values and try to use them in a sane way.

I have not seen other places where driver shost config values are 
modified by the core code.

Thanks,
John

Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[Ming Lei]

On Tue, Apr 20, 2021 at 09:14:12AM +0100, John Garry wrote:
> On 20/04/2021 01:02, Ming Lei wrote:
> > On Tue, Apr 20, 2021 at 12:06:24AM +0800, John Garry wrote:
> > > Function sdev_store_queue_depth() enforces that the sdev queue depth cannot
> > > exceed shost.can_queue.
> > > 
> > > However, the LLDD may still set cmd_per_lun > can_queue, which leads to an
> > > initial sdev queue depth greater than can_queue.
> > > 
> > > Stop this happened by capping initial sdev queue depth at can_queue.
> > > 
> > > Signed-off-by: John Garry <john.garry@huawei.com>
> > > ---
> > > Topic originally discussed at:
> > > https://lore.kernel.org/linux-scsi/85dec8eb-8eab-c7d6-b0fb-5622747c5499@interlog.com/T/#m5663d0cac657d843b93d0c9a2374f98fc04384b9
> > > 
> > > Last idea there was to error/warn in scsi_add_host() for cmd_per_lun >
> > 
> 
> Hi Ming,
> 
> > No, that isn't my suggestion.
> 
> Right, it was what I mentioned.
> 
> > 
> > > can_queue. However, such a shost driver could still configure the sdev
> > > queue depth to be sound value at .slave_configure callback, so now thinking
> > > the orig patch better.
> > 
> > As I mentioned last time, why can't we fix ->cmd_per_lun in
> > scsi_add_host() using .can_queue?
> > 
> 
> I would rather not change the values which are provided from the driver. I
> would rather take the original values and try to use them in a sane way.
> 
> I have not seen other places where driver shost config values are modified
> by the core code.

Wrt. .cmd_per_lun, I think it is safe to modify it into one correct
depth because almost all drivers are just producer of .cmd_per_lun. And
except for debug purpose, there are only three consumers of .cmd_per_lun
in scsi, and all are for scsi_change_queue_depth():

	process_message()
	scsi_alloc_sdev()
	virtscsi_change_queue_depth()


Thanks,
Ming

Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[John Garry]

On 22/04/2021 02:38, Ming Lei wrote:
>> I would rather not change the values which are provided from the driver. I
>> would rather take the original values and try to use them in a sane way.
>>
>> I have not seen other places where driver shost config values are modified
>> by the core code.

Hi Ming,

> Wrt. .cmd_per_lun, I think it is safe to modify it into one correct
> depth because almost all drivers are just producer of .cmd_per_lun. And
> except for debug purpose, there are only three consumers of .cmd_per_lun
> in scsi, and all are for scsi_change_queue_depth():
> 
> 	process_message()
> 	scsi_alloc_sdev()
> 	virtscsi_change_queue_depth()

sg_ioctl_common() also looks to read it, but I can't imagine we could 
break that interface with either suggested change.

So I still prefer not to modify shost.cmd_per_lun, but if you feel 
strongly enough then I can look to make that change.

Thanks,
John

Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue

[Ming Lei]

On Thu, Apr 22, 2021 at 05:35:42PM +0100, John Garry wrote:
> On 22/04/2021 02:38, Ming Lei wrote:
> > > I would rather not change the values which are provided from the driver. I
> > > would rather take the original values and try to use them in a sane way.
> > > 
> > > I have not seen other places where driver shost config values are modified
> > > by the core code.
> 
> Hi Ming,
> 
> > Wrt. .cmd_per_lun, I think it is safe to modify it into one correct
> > depth because almost all drivers are just producer of .cmd_per_lun. And
> > except for debug purpose, there are only three consumers of .cmd_per_lun
> > in scsi, and all are for scsi_change_queue_depth():
> > 
> > 	process_message()
> > 	scsi_alloc_sdev()
> > 	virtscsi_change_queue_depth()
> 
> sg_ioctl_common() also looks to read it, but I can't imagine we could break
> that interface with either suggested change.

Then one bad .cmd_per_lun can be passed to userspace, as your patch
doesn't cover this case.

> 
> So I still prefer not to modify shost.cmd_per_lun, but if you feel strongly
> enough then I can look to make that change.

I still suggest to make .cmd_per_lun correct since the beginning,
otherwise you may have to cover anywhere .cmd_per_lun is used.


Thanks,
Ming