> Yes. If you look at other code in storvsc_drv.c, you'll see the use of
> "dev_err" for outputting warning messages. Follow the pattern of
> these other uses of "dev_err", and that should eliminate the
> checkpatch error.
I'm not sure I can use the dev_warn/dev_error macros (or the storvsc_log
macro which uses dev_warn) in storvsc_drv_init(), as it seems like it is
meant to be used with a device, and, at driver initialization, I'm not
sure how to access a specific device.
I originally used printk() after looking for an example of a driver
printing some logging message before erroring out in its init function
and had found this in drivers/scsi/iscsi_tcp.c
static int __init iscsi_sw_tcp_init(void)
{
if (iscsi_max_lun < 1) {
printk(KERN_ERR "iscsi_tcp: Invalid max_lun value of %u\n",
iscsi_max_lun);
return -EINVAL;
}
...
}
Unfortunately, the thread has split in two because of some issues with my
original mail. This message has the other updates I've made to the patch [1].
> I'm in agreement that the current handling of I/O queuing in the storvsc
> has problems. Your data definitely confirms that, and there are other
> data points that indicate that we need to more fundamentally rethink
> what I/Os get queued where. Storvsc is letting far too many I/Os get
> queued in the VMbus ring buffers and in the underlying Hyper-V.
>
> Adding a module parameter to specify the number of hardware queues
> may be part of the solution. But I really want to step back a bit and
> take into account all the data points we have before deciding what to
> change, what additional parameters to offer (if any), etc. There are
> other ways of limiting the number of I/Os being queued at the driver
> level, and I'm wondering how those tradeoff against adding a module
> parameter. I'm planning to jump in on this topic in just a few weeks,
> and would like to coordinate with you.
There are tradeoffs to reducing the number of hardware queues. It may
not be the right solution for many workloads. However, parameterizing
the number of hardware queues, given that the default would be the same
as current state, doesn't seem like it would be harmful.
virtio-scsi, for example, seems to provide num_queues as a configuration
option in virtscsi_probe() in drivers/scsi/virtio_scsi.c
num_queues = virtscsi_config_get(vdev, num_queues) ? : 1;
num_queues = min_t(unsigned int, nr_cpu_ids, num_queues);
And then use that value to set nr_hw_queues in the scsi host:
...
shost->nr_hw_queues = num_queues;
In fact, it may be worth modifying the patch to allow setting the number
of hardware queues to any number above zero.
I made it a module parameter to avoid having to change the interface at
any point above storvsc in the stack to allow for configuration of this
parameter.
[1] https://lore.kernel.org/linux-hyperv/20210205212447.3327-3-melanieplageman@xxxxxxxxx/
