[PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Melanie Plageman (Microsoft)]

The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
allocation.

Cap cmd_per_lun at can_queue to avoid dispatch errors.

Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
---
 drivers/scsi/storvsc_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
index 6bc5453cea8a..d7953a6e00e6 100644
--- a/drivers/scsi/storvsc_drv.c
+++ b/drivers/scsi/storvsc_drv.c
@@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
 				(max_sub_channels + 1) *
 				(100 - ring_avail_percent_lowater) / 100;
 
+	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun, scsi_driver.can_queue);
+
 	host = scsi_host_alloc(&scsi_driver,
 			       sizeof(struct hv_host_device));
 	if (!host)
-- 
2.20.1

RE: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Michael Kelley]

From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday, March 5, 2021 3:22 PM
> 
> The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
> allocation.
> 
> Cap cmd_per_lun at can_queue to avoid dispatch errors.
> 
> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> ---
>  drivers/scsi/storvsc_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 6bc5453cea8a..d7953a6e00e6 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
>  				(max_sub_channels + 1) *
>  				(100 - ring_avail_percent_lowater) / 100;
> 
> +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun, scsi_driver.can_queue);
> +

I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
Be aware that the calculation of "can_queue" in this driver is somewhat
flawed -- it should not be based on the size of the ring buffer, but instead on
the maximum number of requests Hyper-V will queue.  And even then,
can_queue doesn't provide the cap you might expect because the blk-mq layer
allocates can_queue tags for each HW queue, not as a total.

I agree that the cmd_per_lun setting is also too big, but we should fix that in
the context of getting all of these different settings working together correctly,
and not piecemeal.

Michael

>  	host = scsi_host_alloc(&scsi_driver,
>  			       sizeof(struct hv_host_device));
>  	if (!host)
> --
> 2.20.1

Re: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Melanie Plageman]

On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
> From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday, March 5, 2021 3:22 PM
> > 
> > The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
> > allocation.
> > 
> > Cap cmd_per_lun at can_queue to avoid dispatch errors.
> > 
> > Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> > ---
> >  drivers/scsi/storvsc_drv.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > index 6bc5453cea8a..d7953a6e00e6 100644
> > --- a/drivers/scsi/storvsc_drv.c
> > +++ b/drivers/scsi/storvsc_drv.c
> > @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
> >  				(max_sub_channels + 1) *
> >  				(100 - ring_avail_percent_lowater) / 100;
> > 
> > +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun, scsi_driver.can_queue);
> > +
> 
> I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?

The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
Scsi_Host->cmd_per_lun in storvsc_probe().

In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
called and sets the scsi_device->queue_depth to the Scsi_Host's
cmd_per_lun with this code:
    
scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?    
                                        sdev->host->cmd_per_lun : 1);

During dispatch, the scsi_device->queue_depth is used in
scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
whether or not the device can queue another command.    

On some machines, with the 2048 value of cmd_per_lun that was used to
set the initial scsi_device->queue_depth, commands can be queued that
are later not able to be dispatched after running out of space in the
ringbuffer.

On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
(running an fio workload that I can provide if needed), storvsc_do_io()
ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
                                                                      
This is the call stack: 
    
hv_get_bytes_to_write
hv_ringbuffer_write    
vmbus_send_packet      
storvsc_dio_io      
storvsc_queuecommand
scsi_dispatch_cmd    
scsi_queue_rq       
dispatch_rq_list

> Be aware that the calculation of "can_queue" in this driver is somewhat
> flawed -- it should not be based on the size of the ring buffer, but instead on
> the maximum number of requests Hyper-V will queue.  And even then,
> can_queue doesn't provide the cap you might expect because the blk-mq layer
> allocates can_queue tags for each HW queue, not as a total.


The docs for scsi_mid_low_api document Scsi_Host can_queue this way:

  can_queue    
  - must be greater than 0; do not send more than can_queue    
    commands to the adapter. 
        
I did notice that in scsi_host.h, the comment for can_queue does say
can_queue is the "maximum number of simultaneous commands a single hw
queue in HBA will accept." However, I don't see it being used this way
in the code.
                                                                            
During dispatch, In scsi_target_queue_ready(), there is this code:

        if (busy >= starget->can_queue)
                goto starved;

And the scsi_target->can_queue value should be coming from Scsi_host as
mentioned in the scsi_target definition in scsi_device.h
    /*
      * LLDs should set this in the slave_alloc host template callout.
      * If set to zero then there is not limit.
      */
    unsigned int            can_queue;

So, I don't really see how this would be per hardware queue.

> 
> I agree that the cmd_per_lun setting is also too big, but we should fix that in
> the context of getting all of these different settings working together correctly,
> and not piecemeal.
> 

Capping Scsi_Host->cmd_per_lun to scsi_driver.can_queue during probe
will also prevent the LUN queue_depth from being set to a value that is
higher than it can ever be set to again by the user when
storvsc_change_queue_depth() is invoked. 

Also in scsi_sysfs sdev_store_queue_depth() there is this check:

          if (depth < 1 || depth > sdev->host->can_queue)
                return -EINVAL;

I would also note that VirtIO SCSI in virtscsi_probe(), Scsi_Host->cmd_per_lun
is set to the min of the configured cmd_per_lun and
Scsi_Host->can_queue:

    shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);

Best,
Melanie

Re: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[John Garry]

On 08/03/2021 17:56, Melanie Plageman wrote:
> On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
>> From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday, March 5, 2021 3:22 PM
>>>
>>> The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
>>> allocation.
>>>
>>> Cap cmd_per_lun at can_queue to avoid dispatch errors.
>>>
>>> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
>>> ---
>>>   drivers/scsi/storvsc_drv.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
>>> index 6bc5453cea8a..d7953a6e00e6 100644
>>> --- a/drivers/scsi/storvsc_drv.c
>>> +++ b/drivers/scsi/storvsc_drv.c
>>> @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
>>>   				(max_sub_channels + 1) *
>>>   				(100 - ring_avail_percent_lowater) / 100;
>>>
>>> +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun, scsi_driver.can_queue);
>>> +
>>
>> I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
> 
> The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
> Scsi_Host->cmd_per_lun in storvsc_probe().
> 
> In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
> called and sets the scsi_device->queue_depth to the Scsi_Host's
> cmd_per_lun with this code:
>      
> scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
>                                          sdev->host->cmd_per_lun : 1);
> 
> During dispatch, the scsi_device->queue_depth is used in
> scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
> whether or not the device can queue another command.
> 
> On some machines, with the 2048 value of cmd_per_lun that was used to
> set the initial scsi_device->queue_depth, commands can be queued that
> are later not able to be dispatched after running out of space in the
> ringbuffer.
> 
> On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
> (running an fio workload that I can provide if needed), storvsc_do_io()
> ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
>                                                                        
> This is the call stack:
>      
> hv_get_bytes_to_write
> hv_ringbuffer_write
> vmbus_send_packet
> storvsc_dio_io
> storvsc_queuecommand
> scsi_dispatch_cmd
> scsi_queue_rq
> dispatch_rq_list
> 
>> Be aware that the calculation of "can_queue" in this driver is somewhat
>> flawed -- it should not be based on the size of the ring buffer, but instead on
>> the maximum number of requests Hyper-V will queue.  And even then,
>> can_queue doesn't provide the cap you might expect because the blk-mq layer
>> allocates can_queue tags for each HW queue, not as a total.
> 
> 
> The docs for scsi_mid_low_api document Scsi_Host can_queue this way:
> 
>    can_queue
>    - must be greater than 0; do not send more than can_queue
>      commands to the adapter.
>          
> I did notice that in scsi_host.h, the comment for can_queue does say
> can_queue is the "maximum number of simultaneous commands a single hw
> queue in HBA will accept." However, I don't see it being used this way
> in the code.
>                            

JFYI, the block layer ensures that no more than can_queue requests are 
sent to the host. See scsi_mq_setup_tags(), and how the tagset queue 
depth is set to shost->can_queue.

Thanks,
John


> During dispatch, In scsi_target_queue_ready(), there is this code:
> 
>          if (busy >= starget->can_queue)
>                  goto starved;
> 
> And the scsi_target->can_queue value should be coming from Scsi_host as
> mentioned in the scsi_target definition in scsi_device.h
>      /*
>        * LLDs should set this in the slave_alloc host template callout.
>        * If set to zero then there is not limit.
>        */
>      unsigned int            can_queue;
> 
> So, I don't really see how this would be per hardware queue.
> 
>>
>> I agree that the cmd_per_lun setting is also too big, but we should fix that in
>> the context of getting all of these different settings working together correctly,
>> and not piecemeal.
>>
> 
> Capping Scsi_Host->cmd_per_lun to scsi_driver.can_queue during probe
> will also prevent the LUN queue_depth from being set to a value that is
> higher than it can ever be set to again by the user when
> storvsc_change_queue_depth() is invoked.
> 
> Also in scsi_sysfs sdev_store_queue_depth() there is this check:
> 
>            if (depth < 1 || depth > sdev->host->can_queue)
>                  return -EINVAL;
> 
> I would also note that VirtIO SCSI in virtscsi_probe(), Scsi_Host->cmd_per_lun
> is set to the min of the configured cmd_per_lun and
> Scsi_Host->can_queue:
> 
>      shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> 
> Best,
> Melanie
> .
> 

RE: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Michael Kelley]

From: John Garry <john.garry@huawei.com> Sent: Tuesday, March 9, 2021 2:10 AM
> 
> On 08/03/2021 17:56, Melanie Plageman wrote:
> > On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
> >> From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday,
> March 5, 2021 3:22 PM
> >>>
> >>> The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
> >>> allocation.
> >>>
> >>> Cap cmd_per_lun at can_queue to avoid dispatch errors.
> >>>
> >>> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> >>> ---
> >>>   drivers/scsi/storvsc_drv.c | 2 ++
> >>>   1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >>> index 6bc5453cea8a..d7953a6e00e6 100644
> >>> --- a/drivers/scsi/storvsc_drv.c
> >>> +++ b/drivers/scsi/storvsc_drv.c
> >>> @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
> >>>   				(max_sub_channels + 1) *
> >>>   				(100 - ring_avail_percent_lowater) / 100;
> >>>
> >>> +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun,
> scsi_driver.can_queue);
> >>> +
> >>
> >> I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
> >
> > The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
> > Scsi_Host->cmd_per_lun in storvsc_probe().
> >
> > In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
> > called and sets the scsi_device->queue_depth to the Scsi_Host's
> > cmd_per_lun with this code:
> >
> > scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
> >                                          sdev->host->cmd_per_lun : 1);
> >
> > During dispatch, the scsi_device->queue_depth is used in
> > scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
> > whether or not the device can queue another command.
> >
> > On some machines, with the 2048 value of cmd_per_lun that was used to
> > set the initial scsi_device->queue_depth, commands can be queued that
> > are later not able to be dispatched after running out of space in the
> > ringbuffer.
> >
> > On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
> > (running an fio workload that I can provide if needed), storvsc_do_io()
> > ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
> >
> > This is the call stack:
> >
> > hv_get_bytes_to_write
> > hv_ringbuffer_write
> > vmbus_send_packet
> > storvsc_dio_io
> > storvsc_queuecommand
> > scsi_dispatch_cmd
> > scsi_queue_rq
> > dispatch_rq_list
> >
> >> Be aware that the calculation of "can_queue" in this driver is somewhat
> >> flawed -- it should not be based on the size of the ring buffer, but instead on
> >> the maximum number of requests Hyper-V will queue.  And even then,
> >> can_queue doesn't provide the cap you might expect because the blk-mq layer
> >> allocates can_queue tags for each HW queue, not as a total.
> >
> >
> > The docs for scsi_mid_low_api document Scsi_Host can_queue this way:
> >
> >    can_queue
> >    - must be greater than 0; do not send more than can_queue
> >      commands to the adapter.
> >
> > I did notice that in scsi_host.h, the comment for can_queue does say
> > can_queue is the "maximum number of simultaneous commands a single hw
> > queue in HBA will accept." However, I don't see it being used this way
> > in the code.
> >
> 
> JFYI, the block layer ensures that no more than can_queue requests are
> sent to the host. See scsi_mq_setup_tags(), and how the tagset queue
> depth is set to shost->can_queue.
> 
> Thanks,
> John

Agree on what's in scsi_mq_setup_tags().  But scsi_mq_setup_tags() calls
blk_mq_alloc_tag_set(), which in turn calls blk_mq_alloc_map_and_requests(),
which calls __blk_mq_alloc_rq_maps() repeatedly, reducing the tag
set queue_depth as needed until it succeeds.

The key thing is that __blk_mq_alloc_rq_maps() iterates over the
number of HW queues calling __blk_mq_alloc_map_and_request().
The latter function allocates the map and the requests with a count
of the tag set's queue_depth.   There's no logic to apportion the
can_queue value across multiple HW queues. So each HW queue gets
can_queue tags allocated, and the SCSI host driver may see up to
(can_queue * # HW queues) simultaneous requests.

I'm certainly not an expert in this area, but that's what I see in the
code.  We've run live experiments, and can see the number
simultaneous requests sent to the storvsc driver be greater than
can_queue when the # of HW queues is greater than 1, which seems
to be consistent with the code.

Michael

> 
> 
> > During dispatch, In scsi_target_queue_ready(), there is this code:
> >
> >          if (busy >= starget->can_queue)
> >                  goto starved;
> >
> > And the scsi_target->can_queue value should be coming from Scsi_host as
> > mentioned in the scsi_target definition in scsi_device.h
> >      /*
> >        * LLDs should set this in the slave_alloc host template callout.
> >        * If set to zero then there is not limit.
> >        */
> >      unsigned int            can_queue;
> >
> > So, I don't really see how this would be per hardware queue.
> >
> >>
> >> I agree that the cmd_per_lun setting is also too big, but we should fix that in
> >> the context of getting all of these different settings working together correctly,
> >> and not piecemeal.
> >>
> >
> > Capping Scsi_Host->cmd_per_lun to scsi_driver.can_queue during probe
> > will also prevent the LUN queue_depth from being set to a value that is
> > higher than it can ever be set to again by the user when
> > storvsc_change_queue_depth() is invoked.
> >
> > Also in scsi_sysfs sdev_store_queue_depth() there is this check:
> >
> >            if (depth < 1 || depth > sdev->host->can_queue)
> >                  return -EINVAL;
> >
> > I would also note that VirtIO SCSI in virtscsi_probe(), Scsi_Host->cmd_per_lun
> > is set to the min of the configured cmd_per_lun and
> > Scsi_Host->can_queue:
> >
> >      shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> >
> > Best,
> > Melanie
> > .
> >

Re: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[John Garry]

On 09/03/2021 15:57, Michael Kelley wrote:
> From: John Garry <john.garry@huawei.com> Sent: Tuesday, March 9, 2021 2:10 AM
>>
>> On 08/03/2021 17:56, Melanie Plageman wrote:
>>> On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
>>>> From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday,
>> March 5, 2021 3:22 PM
>>>>>
>>>>> The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
>>>>> allocation.
>>>>>
>>>>> Cap cmd_per_lun at can_queue to avoid dispatch errors.
>>>>>
>>>>> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
>>>>> ---
>>>>>    drivers/scsi/storvsc_drv.c | 2 ++
>>>>>    1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
>>>>> index 6bc5453cea8a..d7953a6e00e6 100644
>>>>> --- a/drivers/scsi/storvsc_drv.c
>>>>> +++ b/drivers/scsi/storvsc_drv.c
>>>>> @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
>>>>>    				(max_sub_channels + 1) *
>>>>>    				(100 - ring_avail_percent_lowater) / 100;
>>>>>
>>>>> +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun,
>> scsi_driver.can_queue);
>>>>> +
>>>>
>>>> I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
>>>
>>> The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
>>> Scsi_Host->cmd_per_lun in storvsc_probe().
>>>
>>> In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
>>> called and sets the scsi_device->queue_depth to the Scsi_Host's
>>> cmd_per_lun with this code:
>>>
>>> scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
>>>                                           sdev->host->cmd_per_lun : 1);
>>>
>>> During dispatch, the scsi_device->queue_depth is used in
>>> scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
>>> whether or not the device can queue another command.
>>>
>>> On some machines, with the 2048 value of cmd_per_lun that was used to
>>> set the initial scsi_device->queue_depth, commands can be queued that
>>> are later not able to be dispatched after running out of space in the
>>> ringbuffer.
>>>
>>> On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
>>> (running an fio workload that I can provide if needed), storvsc_do_io()
>>> ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
>>>
>>> This is the call stack:
>>>
>>> hv_get_bytes_to_write
>>> hv_ringbuffer_write
>>> vmbus_send_packet
>>> storvsc_dio_io
>>> storvsc_queuecommand
>>> scsi_dispatch_cmd
>>> scsi_queue_rq
>>> dispatch_rq_list
>>>
>>>> Be aware that the calculation of "can_queue" in this driver is somewhat
>>>> flawed -- it should not be based on the size of the ring buffer, but instead on
>>>> the maximum number of requests Hyper-V will queue.  And even then,
>>>> can_queue doesn't provide the cap you might expect because the blk-mq layer
>>>> allocates can_queue tags for each HW queue, not as a total.
>>>
>>>
>>> The docs for scsi_mid_low_api document Scsi_Host can_queue this way:
>>>
>>>     can_queue
>>>     - must be greater than 0; do not send more than can_queue
>>>       commands to the adapter.
>>>
>>> I did notice that in scsi_host.h, the comment for can_queue does say
>>> can_queue is the "maximum number of simultaneous commands a single hw
>>> queue in HBA will accept." However, I don't see it being used this way
>>> in the code.
>>>
>>
>> JFYI, the block layer ensures that no more than can_queue requests are
>> sent to the host. See scsi_mq_setup_tags(), and how the tagset queue
>> depth is set to shost->can_queue.
>>
>> Thanks,
>> John
> 
> Agree on what's in scsi_mq_setup_tags().  But scsi_mq_setup_tags() calls
> blk_mq_alloc_tag_set(), which in turn calls blk_mq_alloc_map_and_requests(),
> which calls __blk_mq_alloc_rq_maps() repeatedly, reducing the tag
> set queue_depth as needed until it succeeds.
> 
> The key thing is that __blk_mq_alloc_rq_maps() iterates over the
> number of HW queues calling __blk_mq_alloc_map_and_request().
> The latter function allocates the map and the requests with a count
> of the tag set's queue_depth.   There's no logic to apportion the
> can_queue value across multiple HW queues. So each HW queue gets
> can_queue tags allocated, and the SCSI host driver may see up to
> (can_queue * # HW queues) simultaneous requests.
> 
> I'm certainly not an expert in this area, but that's what I see in the
> code.  We've run live experiments, and can see the number
> simultaneous requests sent to the storvsc driver be greater than
> can_queue when the # of HW queues is greater than 1, which seems
> to be consistent with the code.

ah, ok. I assumed that # of HW queues = 1 here. So you're describing a 
problem similar to 
https://lore.kernel.org/linux-scsi/b3e4e597-779b-7c1e-0d3c-07bc3dab1bb5@huawei.com/

So if you check nr_hw_queues comment in include/scsi/scsi_host.h, it reads:
the total queue depth per host is nr_hw_queues * can_queue. However, for 
when host_tagset is set, the total queue depth is can_queue.

Setting .host_tagset will ensure at most can_queue requests will be sent 
over all HW queues at any given time. A few SCSI MQ drivers set this now.

Thanks,
John

> 
> Michael
> 
>>
>>
>>> During dispatch, In scsi_target_queue_ready(), there is this code:
>>>
>>>           if (busy >= starget->can_queue)
>>>                   goto starved;
>>>
>>> And the scsi_target->can_queue value should be coming from Scsi_host as
>>> mentioned in the scsi_target definition in scsi_device.h
>>>       /*
>>>         * LLDs should set this in the slave_alloc host template callout.
>>>         * If set to zero then there is not limit.
>>>         */
>>>       unsigned int            can_queue;
>>>
>>> So, I don't really see how this would be per hardware queue.
>>>
>>>>
>>>> I agree that the cmd_per_lun setting is also too big, but we should fix that in
>>>> the context of getting all of these different settings working together correctly,
>>>> and not piecemeal.
>>>>
>>>
>>> Capping Scsi_Host->cmd_per_lun to scsi_driver.can_queue during probe
>>> will also prevent the LUN queue_depth from being set to a value that is
>>> higher than it can ever be set to again by the user when
>>> storvsc_change_queue_depth() is invoked.
>>>
>>> Also in scsi_sysfs sdev_store_queue_depth() there is this check:
>>>
>>>             if (depth < 1 || depth > sdev->host->can_queue)
>>>                   return -EINVAL;
>>>
>>> I would also note that VirtIO SCSI in virtscsi_probe(), Scsi_Host->cmd_per_lun
>>> is set to the min of the configured cmd_per_lun and
>>> Scsi_Host->can_queue:
>>>
>>>       shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
>>>
>>> Best,
>>> Melanie
>>> .
>>>
> 

RE: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Michael Kelley]

From: John Garry <john.garry@huawei.com> Sent: Tuesday, March 9, 2021 8:36 AM
> 
> On 09/03/2021 15:57, Michael Kelley wrote:
> > From: John Garry <john.garry@huawei.com> Sent: Tuesday, March 9, 2021 2:10 AM
> >>
> >> On 08/03/2021 17:56, Melanie Plageman wrote:
> >>> On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
> >>>> From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday,
> >> March 5, 2021 3:22 PM
> >>>>>
> >>>>> The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
> >>>>> allocation.
> >>>>>
> >>>>> Cap cmd_per_lun at can_queue to avoid dispatch errors.
> >>>>>
> >>>>> Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> >>>>> ---
> >>>>>    drivers/scsi/storvsc_drv.c | 2 ++
> >>>>>    1 file changed, 2 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> >>>>> index 6bc5453cea8a..d7953a6e00e6 100644
> >>>>> --- a/drivers/scsi/storvsc_drv.c
> >>>>> +++ b/drivers/scsi/storvsc_drv.c
> >>>>> @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
> >>>>>    				(max_sub_channels + 1) *
> >>>>>    				(100 - ring_avail_percent_lowater) / 100;
> >>>>>
> >>>>> +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun,
> >> scsi_driver.can_queue);
> >>>>> +
> >>>>
> >>>> I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
> >>>
> >>> The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
> >>> Scsi_Host->cmd_per_lun in storvsc_probe().
> >>>
> >>> In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
> >>> called and sets the scsi_device->queue_depth to the Scsi_Host's
> >>> cmd_per_lun with this code:
> >>>
> >>> scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
> >>>                                           sdev->host->cmd_per_lun : 1);
> >>>
> >>> During dispatch, the scsi_device->queue_depth is used in
> >>> scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
> >>> whether or not the device can queue another command.
> >>>
> >>> On some machines, with the 2048 value of cmd_per_lun that was used to
> >>> set the initial scsi_device->queue_depth, commands can be queued that
> >>> are later not able to be dispatched after running out of space in the
> >>> ringbuffer.
> >>>
> >>> On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
> >>> (running an fio workload that I can provide if needed), storvsc_do_io()
> >>> ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
> >>>
> >>> This is the call stack:
> >>>
> >>> hv_get_bytes_to_write
> >>> hv_ringbuffer_write
> >>> vmbus_send_packet
> >>> storvsc_dio_io
> >>> storvsc_queuecommand
> >>> scsi_dispatch_cmd
> >>> scsi_queue_rq
> >>> dispatch_rq_list
> >>>
> >>>> Be aware that the calculation of "can_queue" in this driver is somewhat
> >>>> flawed -- it should not be based on the size of the ring buffer, but instead on
> >>>> the maximum number of requests Hyper-V will queue.  And even then,
> >>>> can_queue doesn't provide the cap you might expect because the blk-mq layer
> >>>> allocates can_queue tags for each HW queue, not as a total.
> >>>
> >>>
> >>> The docs for scsi_mid_low_api document Scsi_Host can_queue this way:
> >>>
> >>>     can_queue
> >>>     - must be greater than 0; do not send more than can_queue
> >>>       commands to the adapter.
> >>>
> >>> I did notice that in scsi_host.h, the comment for can_queue does say
> >>> can_queue is the "maximum number of simultaneous commands a single hw
> >>> queue in HBA will accept." However, I don't see it being used this way
> >>> in the code.
> >>>
> >>
> >> JFYI, the block layer ensures that no more than can_queue requests are
> >> sent to the host. See scsi_mq_setup_tags(), and how the tagset queue
> >> depth is set to shost->can_queue.
> >>
> >> Thanks,
> >> John
> >
> > Agree on what's in scsi_mq_setup_tags().  But scsi_mq_setup_tags() calls
> > blk_mq_alloc_tag_set(), which in turn calls blk_mq_alloc_map_and_requests(),
> > which calls __blk_mq_alloc_rq_maps() repeatedly, reducing the tag
> > set queue_depth as needed until it succeeds.
> >
> > The key thing is that __blk_mq_alloc_rq_maps() iterates over the
> > number of HW queues calling __blk_mq_alloc_map_and_request().
> > The latter function allocates the map and the requests with a count
> > of the tag set's queue_depth.   There's no logic to apportion the
> > can_queue value across multiple HW queues. So each HW queue gets
> > can_queue tags allocated, and the SCSI host driver may see up to
> > (can_queue * # HW queues) simultaneous requests.
> >
> > I'm certainly not an expert in this area, but that's what I see in the
> > code.  We've run live experiments, and can see the number
> > simultaneous requests sent to the storvsc driver be greater than
> > can_queue when the # of HW queues is greater than 1, which seems
> > to be consistent with the code.
> 
> ah, ok. I assumed that # of HW queues = 1 here. So you're describing a
> problem similar to
> https://lore.kernel.org/linux-scsi/b3e4e597-779b-7c1e-0d3c-07bc3dab1bb5@huawei.com/

Yes.

> 
> So if you check nr_hw_queues comment in include/scsi/scsi_host.h, it reads:
> the total queue depth per host is nr_hw_queues * can_queue. However, for
> when host_tagset is set, the total queue depth is can_queue.
> 
> Setting .host_tagset will ensure at most can_queue requests will be sent
> over all HW queues at any given time. A few SCSI MQ drivers set this now.
> 

I had seen the "host_tagset" option in some recent investigations, and it
looks like it better models what the storvsc driver wants.  But given that
only a few drivers were using it, it seemed like it might be far enough off the
beaten path to be a bit risky.

The storvsc driver is the driver for the synthetic SCSI controller provided
by Microsoft's Hyper-V hypervisor, and as such it is used heavily in the
Azure public cloud.  The settings in storvsc have changed incrementally
over the years, and there are now some inconsistencies as Melanie has
pointed out.  Storvsc and the underlying Hyper-V run in VMs with relatively
low IOPS limits and also in VMs that can hit hundreds of K IOPS.  Getting
enough parallelism to drive the high IOPS while not overloading in the
low IOPS environments is the challenge.   As a purely synthetic device,
there aren't really multiple HW queues, but configuring for multiple
queues helps drive the higher end IOPS numbers in VMs with sizable
vCPU counts.

So setting "host_tagset" while allowing multiple HW queues for
higher parallelism might be a good approach.  We will experiment.

Michael

RE: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue

[Michael Kelley]

From: Melanie Plageman <melanieplageman@gmail.com> Sent: Monday, March 8, 2021 9:56 AM
> 
> On Mon, Mar 08, 2021 at 02:37:40PM +0000, Michael Kelley wrote:
> > From: Melanie Plageman (Microsoft) <melanieplageman@gmail.com> Sent: Friday, March
> 5, 2021 3:22 PM
> > >
> > > The scsi_device->queue_depth is set to Scsi_Host->cmd_per_lun during
> > > allocation.
> > >
> > > Cap cmd_per_lun at can_queue to avoid dispatch errors.
> > >
> > > Signed-off-by: Melanie Plageman (Microsoft) <melanieplageman@gmail.com>
> > > ---
> > >  drivers/scsi/storvsc_drv.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> > > index 6bc5453cea8a..d7953a6e00e6 100644
> > > --- a/drivers/scsi/storvsc_drv.c
> > > +++ b/drivers/scsi/storvsc_drv.c
> > > @@ -1946,6 +1946,8 @@ static int storvsc_probe(struct hv_device *device,
> > >  				(max_sub_channels + 1) *
> > >  				(100 - ring_avail_percent_lowater) / 100;
> > >
> > > +	scsi_driver.cmd_per_lun = min_t(u32, scsi_driver.cmd_per_lun,
> scsi_driver.can_queue);
> > > +
> >
> > I'm not sure what you mean by "avoid dispatch errors".  Can you elaborate?
> 
> The scsi_driver.cmd_per_lun is set to 2048. Which is then used to set
> Scsi_Host->cmd_per_lun in storvsc_probe().
> 
> In storvsc_probe(), when doing scsi_scan_host(), scsi_alloc_sdev() is
> called and sets the scsi_device->queue_depth to the Scsi_Host's
> cmd_per_lun with this code:
> 
> scsi_change_queue_depth(sdev, sdev->host->cmd_per_lun ?
>                                         sdev->host->cmd_per_lun : 1);
> 
> During dispatch, the scsi_device->queue_depth is used in
> scsi_dev_queue_ready(), called by scsi_mq_get_budget() to determine
> whether or not the device can queue another command.
> 
> On some machines, with the 2048 value of cmd_per_lun that was used to
> set the initial scsi_device->queue_depth, commands can be queued that
> are later not able to be dispatched after running out of space in the
> ringbuffer.
> 
> On an 8 core Azure VM with 16GB of memory with a single 1 TiB SSD
> (running an fio workload that I can provide if needed), storvsc_do_io()
> ends up often returning SCSI_MLQUEUE_DEVICE_BUSY.
> 
> This is the call stack:
> 
> hv_get_bytes_to_write
> hv_ringbuffer_write
> vmbus_send_packet
> storvsc_dio_io
> storvsc_queuecommand
> scsi_dispatch_cmd
> scsi_queue_rq
> dispatch_rq_list
> 
> > Be aware that the calculation of "can_queue" in this driver is somewhat
> > flawed -- it should not be based on the size of the ring buffer, but instead on
> > the maximum number of requests Hyper-V will queue.  And even then,
> > can_queue doesn't provide the cap you might expect because the blk-mq layer
> > allocates can_queue tags for each HW queue, not as a total.
> 
> 
> The docs for scsi_mid_low_api document Scsi_Host can_queue this way:
> 
>   can_queue
>   - must be greater than 0; do not send more than can_queue
>     commands to the adapter.
> 
> I did notice that in scsi_host.h, the comment for can_queue does say
> can_queue is the "maximum number of simultaneous commands a single hw
> queue in HBA will accept." 

Yes, this comment is correct.  The can_queue value is per HW queue.

> However, I don't see it being used this way
> in the code.
> 
> During dispatch, In scsi_target_queue_ready(), there is this code:
> 
>         if (busy >= starget->can_queue)
>                 goto starved;
> 
> And the scsi_target->can_queue value should be coming from Scsi_host as
> mentioned in the scsi_target definition in scsi_device.h
>     /*
>       * LLDs should set this in the slave_alloc host template callout.
>       * If set to zero then there is not limit.
>       */
>     unsigned int            can_queue;
> 
> So, I don't really see how this would be per hardware queue.

For the storvsc driver, the can_queue value in the scsi_target is initialized
to zero in scsi_alloc_target() and it remains unchanged.  Maybe I'm missing
something, but the only place I see that sets starget->can_queue to a
non-zero value is iscsi_target_alloc().  The storvsc slave_alloc() function does
not set it.  So the test in scsi_target_queue_ready() for exceeding can_queue
never executes.

We've run live tests, and can see that the number of requests sent to the
storvsc driver exceeds the can_queue value when the # of HW queues is
greater than 1.  That result is consistent with what I see in the code.

Michael

> 
> >
> > I agree that the cmd_per_lun setting is also too big, but we should fix that in
> > the context of getting all of these different settings working together correctly,
> > and not piecemeal.
> >
> 
> Capping Scsi_Host->cmd_per_lun to scsi_driver.can_queue during probe
> will also prevent the LUN queue_depth from being set to a value that is
> higher than it can ever be set to again by the user when
> storvsc_change_queue_depth() is invoked.
> 
> Also in scsi_sysfs sdev_store_queue_depth() there is this check:
> 
>           if (depth < 1 || depth > sdev->host->can_queue)
>                 return -EINVAL;
> 
> I would also note that VirtIO SCSI in virtscsi_probe(), Scsi_Host->cmd_per_lun
> is set to the min of the configured cmd_per_lun and
> Scsi_Host->can_queue:
> 
>     shost->cmd_per_lun = min_t(u32, cmd_per_lun, shost->can_queue);
> 
> Best,
> Melanie