Messages in this thread | | | Subject | Re: [PATCH v1] scsi: storvsc: Cap cmd_per_lun at can_queue | From | John Garry <> | Date | Tue, 9 Mar 2021 16:35:39 +0000 |
| |
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 >>> . >>> >
| |