Re: [libvirt] Fwd: [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
Hi Mike & Co,
---------- Forwarded message ---------- From: Mike Perez <thingee@gmail.com> Date: Fri, Jun 27, 2014 at 7:39 PM Subject: Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi To: Ján Tomko <jtomko@redhat.com> Cc: Eric Blake <eblake@redhat.com>, libvir-list@redhat.com, berrange@redhat.com, "Nab@daterainc.com" <Nab@daterainc.com>
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options.
We could also use camelCase [1], or just roll a dice.
Well underscores are originally what I had in my first patch [1]. Eric what do you think?
Can we please keep the libvirt names consistent for virtio-scsi parameters with what is already defined in qemu..? (Paolo CC'ed)
From qemu/include/hw/virtio/virtio-scsi.h:
#define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field) \ DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \ DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF),\ DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128) AFAICT, having different names between qemu + libvirt parameters can only add confusion. --nab
On 07/02/2014 03:51 AM, Nicholas A. Bellinger wrote:
Hi Mike & Co,
Can we please keep the libvirt names consistent for virtio-scsi parameters with what is already defined in qemu..? (Paolo CC'ed)
From qemu/include/hw/virtio/virtio-scsi.h:
#define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field) \ DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \
This one is already exposed as 'queues' and it has been in a few libvirt releases - changing it would break compatibility.
DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF),\ DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
These are the attribute names I just pushed. Jan
AFAICT, having different names between qemu + libvirt parameters can only add confusion.
--nab
On Tue, Jul 01, 2014 at 06:51:46PM -0700, Nicholas A. Bellinger wrote:
Hi Mike & Co,
---------- Forwarded message ---------- From: Mike Perez <thingee@gmail.com> Date: Fri, Jun 27, 2014 at 7:39 PM Subject: Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi To: Ján Tomko <jtomko@redhat.com> Cc: Eric Blake <eblake@redhat.com>, libvir-list@redhat.com, berrange@redhat.com, "Nab@daterainc.com" <Nab@daterainc.com>
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options.
We could also use camelCase [1], or just roll a dice.
Well underscores are originally what I had in my first patch [1]. Eric what do you think?
Can we please keep the libvirt names consistent for virtio-scsi parameters with what is already defined in qemu..? (Paolo CC'ed)
From qemu/include/hw/virtio/virtio-scsi.h:
#define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field) \ DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \ DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF),\ DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
AFAICT, having different names between qemu + libvirt parameters can only add confusion.
QEMU is not the only hypervisor that libvirt targets, so tieing libvirt names to QEMU names is a non-goal. We pick the names that make most sense in the context of libvirt. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Wed, 2014-07-02 at 10:27 +0100, Daniel P. Berrange wrote:
On Tue, Jul 01, 2014 at 06:51:46PM -0700, Nicholas A. Bellinger wrote:
Hi Mike & Co,
---------- Forwarded message ---------- From: Mike Perez <thingee@gmail.com> Date: Fri, Jun 27, 2014 at 7:39 PM Subject: Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi To: Ján Tomko <jtomko@redhat.com> Cc: Eric Blake <eblake@redhat.com>, libvir-list@redhat.com, berrange@redhat.com, "Nab@daterainc.com" <Nab@daterainc.com>
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options.
We could also use camelCase [1], or just roll a dice.
Well underscores are originally what I had in my first patch [1]. Eric what do you think?
Can we please keep the libvirt names consistent for virtio-scsi parameters with what is already defined in qemu..? (Paolo CC'ed)
From qemu/include/hw/virtio/virtio-scsi.h:
#define DEFINE_VIRTIO_SCSI_PROPERTIES(_state, _conf_field) \ DEFINE_PROP_UINT32("num_queues", _state, _conf_field.num_queues, 1), \ DEFINE_PROP_UINT32("max_sectors", _state, _conf_field.max_sectors, 0xFFFF),\ DEFINE_PROP_UINT32("cmd_per_lun", _state, _conf_field.cmd_per_lun, 128)
AFAICT, having different names between qemu + libvirt parameters can only add confusion.
QEMU is not the only hypervisor that libvirt targets, so tieing libvirt names to QEMU names is a non-goal. We pick the names that make most sense in the context of libvirt.
Not sure I follow.. virtio-scsi is specific to QEMU/KVM, and per the comment in the original patch: 'Currently it only supports attribute <code>queues</code> (<span class="since">1.0.5</span>, QEMU and KVM only)' would seem to indicate the parameter names are only used in the context of QEMU/KVM, no..? If the virtio-scsi parameters are intended to be used across hypervisors, then matching them to QEMU's own names doesn't really matter. But if they are specific to virtio-scsi and only used by QEMU/KVM instances, then renaming them to something arbitrary to libvirt is pointless and confusing. --nab
On 07/02/2014 06:28 PM, Nicholas A. Bellinger wrote:
QEMU is not the only hypervisor that libvirt targets, so tieing libvirt names to QEMU names is a non-goal. We pick the names that make most sense in the context of libvirt.
Not sure I follow.. virtio-scsi is specific to QEMU/KVM, and per the comment in the original patch:
'Currently it only supports attribute <code>queues</code> (<span class="since">1.0.5</span>, QEMU and KVM only)'
would seem to indicate the parameter names are only used in the context of QEMU/KVM, no..?
Just because qemu is the only hypervisor driver that _currently_ uses the feature doesn't preclude the libxl hypervisor from _also_ gaining support for the feature in a future libvirt release, at which point the documentation would mention the new version number for the additional use of the feature. Again, the name qemu chose is not necessarily the best name compared to what it might map to in libxl or any other hypervisor, so libvirt tries to pick names that are consistent with other libvirt terms, even if they don't match underlying qemu names.
If the virtio-scsi parameters are intended to be used across hypervisors, then matching them to QEMU's own names doesn't really matter. But if they are specific to virtio-scsi and only used by QEMU/KVM instances, then renaming them to something arbitrary to libvirt is pointless and confusing.
virtio is not necessarily a qemu-only concept. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, 2014-07-02 at 18:48 -0600, Eric Blake wrote:
On 07/02/2014 06:28 PM, Nicholas A. Bellinger wrote:
QEMU is not the only hypervisor that libvirt targets, so tieing libvirt names to QEMU names is a non-goal. We pick the names that make most sense in the context of libvirt.
Not sure I follow.. virtio-scsi is specific to QEMU/KVM, and per the comment in the original patch:
'Currently it only supports attribute <code>queues</code> (<span class="since">1.0.5</span>, QEMU and KVM only)'
would seem to indicate the parameter names are only used in the context of QEMU/KVM, no..?
Just because qemu is the only hypervisor driver that _currently_ uses the feature doesn't preclude the libxl hypervisor from _also_ gaining support for the feature in a future libvirt release, at which point the documentation would mention the new version number for the additional use of the feature. Again, the name qemu chose is not necessarily the best name compared to what it might map to in libxl or any other hypervisor, so libvirt tries to pick names that are consistent with other libvirt terms, even if they don't match underlying qemu names.
It's worth mentioning that these names are coming directly from the Linux SCSI subsystem tunables that are read from virtio-scsi config space and set during drivers/scsi/virtio_scsi.c:virtscsi_probe() init time, and did *not* originate with QEMU.
If the virtio-scsi parameters are intended to be used across hypervisors, then matching them to QEMU's own names doesn't really matter. But if they are specific to virtio-scsi and only used by QEMU/KVM instances, then renaming them to something arbitrary to libvirt is pointless and confusing.
virtio is not necessarily a qemu-only concept.
The point is that if a potential port of virtio-scsi to Xen ever happens, it will use the same parameter names that every Linux SCSI LLD has been using for the last 20 years. Eg: They will be the same parameters names that the virtio-scsi LLD pulls out of configuration space, regardless of if it's KVM or XEN. --nab
On Wed, Jul 02, 2014 at 06:48:36PM -0600, Eric Blake wrote:
On 07/02/2014 06:28 PM, Nicholas A. Bellinger wrote:
QEMU is not the only hypervisor that libvirt targets, so tieing libvirt names to QEMU names is a non-goal. We pick the names that make most sense in the context of libvirt.
Not sure I follow.. virtio-scsi is specific to QEMU/KVM, and per the comment in the original patch:
'Currently it only supports attribute <code>queues</code> (<span class="since">1.0.5</span>, QEMU and KVM only)'
would seem to indicate the parameter names are only used in the context of QEMU/KVM, no..?
Just because qemu is the only hypervisor driver that _currently_ uses the feature doesn't preclude the libxl hypervisor from _also_ gaining support for the feature in a future libvirt release, at which point the documentation would mention the new version number for the additional use of the feature. Again, the name qemu chose is not necessarily the best name compared to what it might map to in libxl or any other hypervisor, so libvirt tries to pick names that are consistent with other libvirt terms, even if they don't match underlying qemu names.
If the virtio-scsi parameters are intended to be used across hypervisors, then matching them to QEMU's own names doesn't really matter. But if they are specific to virtio-scsi and only used by QEMU/KVM instances, then renaming them to something arbitrary to libvirt is pointless and confusing.
virtio is not necessarily a qemu-only concept.
Indeed, virtio is already used outside of QEMU & Linux by the FreeBSD BHyve virt platform. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Ján Tomko -
Nicholas A. Bellinger