* [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
@ 2021-04-19 16:06 John Garry
2021-04-20 0:02 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2021-04-19 16:06 UTC (permalink / raw)
To: jejb, martin.petersen
Cc: linux-scsi, linux-kernel, kashyap.desai, ming.lei, dgilbert, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
2021-04-19 16:06 [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue John Garry
@ 2021-04-20 0:02 ` Ming Lei
2021-04-20 8:14 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2021-04-20 0:02 UTC (permalink / raw)
To: John Garry
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, kashyap.desai, dgilbert
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
2021-04-20 0:02 ` Ming Lei
@ 2021-04-20 8:14 ` John Garry
2021-04-22 1:38 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2021-04-20 8:14 UTC (permalink / raw)
To: Ming Lei
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, kashyap.desai, dgilbert
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
2021-04-20 8:14 ` John Garry
@ 2021-04-22 1:38 ` Ming Lei
2021-04-22 16:35 ` John Garry
0 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2021-04-22 1:38 UTC (permalink / raw)
To: John Garry
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, kashyap.desai, dgilbert
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
2021-04-22 1:38 ` Ming Lei
@ 2021-04-22 16:35 ` John Garry
2021-04-23 1:54 ` Ming Lei
0 siblings, 1 reply; 6+ messages in thread
From: John Garry @ 2021-04-22 16:35 UTC (permalink / raw)
To: Ming Lei
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, kashyap.desai, dgilbert
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue
2021-04-22 16:35 ` John Garry
@ 2021-04-23 1:54 ` Ming Lei
0 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2021-04-23 1:54 UTC (permalink / raw)
To: John Garry
Cc: jejb, martin.petersen, linux-scsi, linux-kernel, kashyap.desai, dgilbert
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-04-23 1:54 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 16:06 [PATCH] scsi: core: Cap initial sdev queue depth at shost.can_queue John Garry
2021-04-20 0:02 ` Ming Lei
2021-04-20 8:14 ` John Garry
2021-04-22 1:38 ` Ming Lei
2021-04-22 16:35 ` John Garry
2021-04-23 1:54 ` Ming Lei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).