storvsc: Set up correct queue depth values for IDE devices
Subsystems (2)
PATCH 1/1 storvsc: Set up correct queue depth values for IDE devices
From: Long Li <[email protected]>
Unlike SCSI and FC, we don't use multiple channels for IDE. So set queue depth correctly for IDE.
Also set the correct cmd_per_lun for all devices.
--- drivers/scsi/storvsc_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/scsi/storvsc_drv.c +6 -2
@@ -1722,15 +1722,19 @@ static int storvsc_probe(struct hv_device *device, max_targets =STORVSC_MAX_TARGETS ; max_channels =STORVSC_MAX_CHANNELS ;/* -* On Windows8 and above, we support sub-channels for storage. +* On Windows8 and above, we support sub-channels for storage +* on SCSI and FC controllers. * The number of sub-channels offerred is based on the number of * VCPUs in the guest. */ - max_sub_channels = (num_cpus / storvsc_vcpus_per_sub_channel); +if (!dev_is_ide) + max_sub_channels = + num_cpus / storvsc_vcpus_per_sub_channel; } scsi_driver.can_queue = (max_outstanding_req_per_channel * (max_sub_channels +1 )); + scsi_driver.cmd_per_lun = scsi_driver.can_queue; host =scsi_host_alloc (&scsi_driver,sizeof (struct hv_host_device ));
Signature · 1 line
2.14.1
Quoted 8 lines · On Thu, 15 Mar 2018 16:52:23 -0700 Long Li <[email protected]>…
From: Long Li <[email protected]>
Unlike SCSI and FC, we don't use multiple channels for IDE. So set queue depth correctly for IDE.
Also set the correct
cmd_per_lunfor all devices.Signed-off-by Long Li <[email protected]>
Looks correct.
Quoted 31 lines
-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Long Li
Sent: Thursday, March 15, 2018 4:52 PM
To: KY Srinivasan <[email protected]>; Haiyang Zhang <[email protected]>; Stephen Hemminger <[email protected]>; James E . J . Bottomley <[email protected]>; Martin K . Petersen <[email protected]>; [email protected]; linux- [email protected]; [email protected]
Cc: Long Li <[email protected]>
Subject: [PATCH] storvsc: Set up correct queue depth values for IDE devicesFrom: Long Li <[email protected]>
Unlike SCSI and FC, we don't use multiple channels for IDE. So set queue depth correctly for IDE.
Also set the correct
cmd_per_lunfor all devices.Signed-off-by Long Li <[email protected]>--- drivers/scsi/storvsc_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/scsi/storvsc_drv.c +0
-0@@ -1722,15 +1722,19 @@ static int storvsc_probe(struct hv_device *device, max_targets =STORVSC_MAX_TARGETS ; max_channels =STORVSC_MAX_CHANNELS ;/*
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device, · line 1732
Quoted 10 lines
drivers/scsi/storvsc_drv.c +5
-2-* On Windows8 and above, we support sub-channels for storage. +* On Windows8 and above, we support sub-channels for storage +* on SCSI and FC controllers. * The number of sub-channels offerred is based on the number of * VCPUs in the guest. */ - max_sub_channels = (num_cpus / storvsc_vcpus_per_sub_channel); +if (!dev_is_ide) + max_sub_channels = + num_cpus / storvsc_vcpus_per_sub_channel;
This calculation of the # of sub-channels doesn't get the right answer (and it didn't before this patch either). storvsc_vcpus_per_sub_channel defaults to 4. If num_cpus is 8, max_sub_channels will be 2, but it should be 1. The sub-channel count should not include the main channel since we add 1 to the sub-channel count below when calculating can_queue.
Furthermore, this is calculation is just a guess, in the sense that we're replicating the algorithm we think Hyper-V is using to determine the number
of sub-channels to offer. It turns out Hyper-V is changing that algorithm for
particular devices in an upcoming new Azure VM size. But the only use of max_sub_channels is in the calculation of can_queue below, so the impact is minimal.
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device, · line 1737
Quoted 5 lines
drivers/scsi/storvsc_drv.c +1
-0} scsi_driver.can_queue = (max_outstanding_req_per_channel * (max_sub_channels +1 )); + scsi_driver.cmd_per_lun = scsi_driver.can_queue;
can_queue is defined as "int", while cmd_per_lun is defined as "short". The calculated value of can_queue could easily be over 32,767 with 15 sub-channels and max_outstanding_req_per_channel being 3036 for the default 1 Mbyte ring buffer. So the assignment to cmd_per_lun could produce truncation and even a negative number.
More broadly, since max_outstanding_req_per_channel is based on the ring buffer size, these calculations imply that Hyper-V storvsp's queuing capacity is based on the ring buffer size. I don't think that's the case. From conversations with the storvsp folks, I think Hyper-V always removes entries from the guest->host ring buffer and then
lets storvsp queue them separately. So we don't want to be linkingcmd_per_lun (or even can_queue, for that matter) to the ring buffer size. The current default ring buffer size of 1 Mbyte is probably 10x bigger than needed, and we want to be able to adjust that without ending up with can_queue and cmd_per_lun values that are too small.
We would probably do better to set can_queue to a constant, and
leave cmd_per_lun at its current value of 2048. The can_queue
value is already capped at 10240 in the blk-mq layer, so maybe that's a reasonable constant to use.
Thoughts?
Quoted 5 lines
drivers/scsi/storvsc_drv.c +0
-1host =scsi_host_alloc (&scsi_driver,sizeof (struct hv_host_device )); --2.14.1
Quoted 35 lines
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device
Quoted 20 lines
Subject: [PATCH] storvsc: Set up correct queue depth values for IDE devices
From: Long Li <[email protected]>
Unlike SCSI and FC, we don't use multiple channels for IDE. So set queue depth correctly for IDE.
Also set the correct
cmd_per_lunfor all devices.Signed-off-by Long Li <[email protected]>--- drivers/scsi/storvsc_drv.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)drivers/scsi/storvsc_drv.c +0
-0@@ -1722,15 +1722,19 @@ static int storvsc_probe(struct hv_device*device,
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device,
Quoted 4 lines
drivers/scsi/storvsc_drv.c +0
-1max_targets =STORVSC_MAX_TARGETS ; max_channels =STORVSC_MAX_CHANNELS ;/* -* On Windows8 and above, we support sub-channels for storage.
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device · line 1725
Quoted 1 line
drivers/scsi/storvsc_drv.c +1
-0+* On Windows8 and above, we support sub-channels for storage
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device · line 1727
Quoted 2 lines
drivers/scsi/storvsc_drv.c +1
-0+* on SCSI and FC controllers. * The number of sub-channels offerred is based on the number of
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device,
Quoted 3 lines
drivers/scsi/storvsc_drv.c +0
-1* VCPUs in the guest. */ - max_sub_channels = (num_cpus /
storvsc_vcpus_per_sub_channel);
Quoted 10 lines
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device, · line 1732
Quoted 3 lines
drivers/scsi/storvsc_drv.c +3
-0+if (!dev_is_ide) + max_sub_channels = + num_cpus / storvsc_vcpus_per_sub_channel;This calculation of the # of sub-channels doesn't get the right answer (and it didn't before this patch either).
storvsc_vcpus_per_sub_channeldefaults to 4.
Ifnum_cpusis 8,max_sub_channelswill be 2, but it should be 1. The sub- channel count should not include the main channel since we add 1 to the sub-channel count below when calculatingcan_queue.
This is a good point. I will fix the code calculating can_queue.
Quoted 10 lines
Furthermore, this is calculation is just a guess, in the sense that we're replicating the algorithm we think Hyper-V is using to determine the number
of sub-channels to offer. It turns out Hyper-V is changing that algorithm for
particular devices in an upcoming new Azure VM size. But the only use ofmax_sub_channelsis in the calculation ofcan_queuebelow, so the impact is minimal.Quoted 2 lines
drivers/scsi/storvsc_drv.c +0
-0}
Quoted 9 lines
drivers/scsi/storvsc_drv.c static int storvsc_probe(struct hv_device *device, · line 1737
Quoted 3 lines
drivers/scsi/storvsc_drv.c +1
-0scsi_driver.can_queue = (max_outstanding_req_per_channel * (max_sub_channels +1 )); + scsi_driver.cmd_per_lun = scsi_driver.can_queue;
can_queueis defined as "int", whilecmd_per_lunis defined as "short". The calculated value ofcan_queuecould easily be over 32,767 with 15 sub-channels andmax_outstanding_req_per_channelbeing 3036 for the default 1 Mbyte ring buffer. So the assignment tocmd_per_luncould produce truncation and even a negative number.
This is a good catch. I think I should try calling blk_set_queue_depth() and pass the correct value.
Quoted 11 lines
More broadly, since
max_outstanding_req_per_channelis based on the ring buffer size, these calculations imply that Hyper-V storvsp's queuing capacity is based on the ring buffer size. I don't think that's the case. From conversations with the storvsp folks, I think Hyper-V always removes entries from theguest->hostring buffer and then
lets storvsp queue them separately. So we don't want to be linkingcmd_per_lun(or evencan_queue, for that matter) to the ring buffer size. The current default ring buffer size of 1 Mbyte is probably 10x bigger than needed, and we want to be able to adjust that without ending up withcan_queueandcmd_per_lunvalues that are too small.
cmd_per_lun needs to reflect the device capacity. What value do you propose? It's not a good idea to leave them constant. Setting those values are also important because we don't' want to return BUSY on writing to ring buffer on full, that will slow down the SCSI stack.
Historically we use ring buffer size to calculate device properties (e.g. can_queue for SCSI host).
I agree this doesn't need to be based on the exact queuing capacity of ring buffer, maybe we can do 2X of that value (e.g. look at how block uses nr_request in MQ). Setting those values smaller is more conservative and I don't see an ill effect.
Quoted 5 lines
We would probably do better to set
can_queueto a constant, and
leavecmd_per_lunat its current value of 2048. Thecan_queue
value is already capped at 10240 in the blk-mq layer, so maybe that's a reasonable constant to use.
Actually this is not a good idea for smaller ring buffers. You'll see the problem when setting both ring buffer sizes to 10 pages.
Quoted 8 lines
Thoughts?
Quoted 5 lines
drivers/scsi/storvsc_drv.c +0
-1host =scsi_host_alloc (&scsi_driver,sizeof (struct hv_host_device )); --2.14.1