[libvirt] [PATCH] Add "io" option to virsh attach-disk sub-command.
--- tools/virsh-domain.c | 14 +++++++++++--- tools/virsh.pod | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0d19d0e..d2a2a05 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -267,6 +267,10 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_STRING, .help = N_("cache mode of disk device") }, + {.name = "io", + .type = VSH_OT_STRING, + .help = N_("io mode of disk device") + }, {.name = "type", .type = VSH_OT_STRING, .help = N_("target device type") @@ -504,8 +508,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *source = NULL, *target = NULL, *driver = NULL, *subdriver = NULL, *type = NULL, *mode = NULL, - *iothread = NULL, *cache = NULL, *serial = NULL, - *straddr = NULL, *wwn = NULL, *targetbus = NULL; + *iothread = NULL, *cache = NULL, *io = NULL, + *serial = NULL, *straddr = NULL, *wwn = NULL, + *targetbus = NULL; struct DiskAddress diskAddr; bool isFile = false, functionReturn = false; int ret; @@ -537,6 +542,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) vshCommandOptStringReq(ctl, cmd, "mode", &mode) < 0 || vshCommandOptStringReq(ctl, cmd, "iothread", &iothread) < 0 || vshCommandOptStringReq(ctl, cmd, "cache", &cache) < 0 || + vshCommandOptStringReq(ctl, cmd, "io", &io) < 0 || vshCommandOptStringReq(ctl, cmd, "serial", &serial) < 0 || vshCommandOptStringReq(ctl, cmd, "wwn", &wwn) < 0 || vshCommandOptStringReq(ctl, cmd, "address", &straddr) < 0 || @@ -579,7 +585,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAddLit(&buf, ">\n"); virBufferAdjustIndent(&buf, 2); - if (driver || subdriver || iothread || cache) { + if (driver || subdriver || iothread || cache || io) { virBufferAddLit(&buf, "<driver"); if (driver) @@ -590,6 +596,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, " iothread='%s'", iothread); if (cache) virBufferAsprintf(&buf, " cache='%s'", cache); + if (io) + virBufferAsprintf(&buf, " io='%s'", io); virBufferAddLit(&buf, "/>\n"); } diff --git a/tools/virsh.pod b/tools/virsh.pod index cd1f25f..9656411 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -2860,6 +2860,8 @@ I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough", "writeback", "directsync" or "unsafe". +I<io> io is "threads", or "native" and selects between pthread based disk +I/O and native Linux AIO. I<iothread> is the number within the range of domain IOThreads to which this disk may be attached (QEMU only). I<serial> is the serial of disk device. I<wwn> is the wwn of disk device. -- 2.9.3
(Apparently I managed to screw up sending that, twice. Long weekend.) Please consider adding the option of setting the "io" attribute when attaching disks to guests. Under Red Hat (unsure if this is true under vanilla libvirt), virt-install and virt-manager both default to explicitly setting "io='native'" in the disk "driver" tag. virsh, however, does not and also does not provide an option to specify that setting at all. As a result, disks use a different IO mechanism (the default, "threads") when attached post-setup using virsh. Adding this option allows users to keep disk performance consistent for disks attached at install, and those attached afterward.
On Tue, May 09, 2017 at 07:31:30AM -0700, Gordon Messmer wrote:
(Apparently I managed to screw up sending that, twice. Long weekend.)
No problem. Your first posting got blocked into the moderator queue since you weren't a subscriber. One of the list admins then approved it, not realizing you had already subscribed to the list & resent the patch
Please consider adding the option of setting the "io" attribute when attaching disks to guests. Under Red Hat (unsure if this is true under vanilla libvirt), virt-install and virt-manager both default to explicitly setting "io='native'" in the disk "driver" tag. virsh, however, does not and also does not provide an option to specify that setting at all. As a result, disks use a different IO mechanism (the default, "threads") when attached post-setup using virsh. Adding this option allows users to keep disk performance consistent for disks attached at install, and those attached afterward.
I think you can use the --iothread flag to "virsh attach-disk" to set this parameter. virsh doesn't set it if not specified, since it tries to avoid making policy decisions, like virt-manager/virt-install do Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 05/09/2017 07:38 AM, Daniel P. Berrange wrote:
I think you can use the --iothread flag to "virsh attach-disk" to set this parameter. virsh doesn't set it if not specified, since it tries to avoid making policy decisions, like virt-manager/virt-install do
That option sets the iothread attribute, which "attribute assigns the disk to an IOThread as defined by the range for the domain iothreads." That's not the same as switching between native AIO and threads IO.
On Tue, May 09, 2017 at 07:51:00AM -0700, Gordon Messmer wrote:
On 05/09/2017 07:38 AM, Daniel P. Berrange wrote:
I think you can use the --iothread flag to "virsh attach-disk" to set this parameter. virsh doesn't set it if not specified, since it tries to avoid making policy decisions, like virt-manager/virt-install do
That option sets the iothread attribute, which "attribute assigns the disk to an IOThread as defined by the range for the domain iothreads." That's not the same as switching between native AIO and threads IO.
Opps, yes, mixed terminology. You're right - virsh is lacking this feature. Feel free to send a patch to add a '--io' flag to the attach-disk command if you want to try fixing this. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On 05/09/2017 09:19 AM, Gordon Messmer wrote:
On 05/09/2017 07:58 AM, Daniel P. Berrange wrote:
Opps, yes, mixed terminology. You're right - virsh is lacking this feature. Feel free to send a patch to add a '--io' flag to the attach-disk command if you want to try fixing this.
Following up: could you let me know if the patch I sent is acceptable. If any other changes are needed, I want to get those in while I still have time to spend on this. Thanks.
On 05/07/2017 05:44 PM, Gordon Messmer wrote:
--- tools/virsh-domain.c | 14 +++++++++++--- tools/virsh.pod | 2 ++ 2 files changed, 13 insertions(+), 3 deletions(-)
Well for some reason I cannot apply this patch using "git am -3"... I think what you should do is rebase to the current git head and then git send-email the patch again after applying some changes. When you send it, be sure to note it's "v2"... I keep a template for that in a file and use: git send-email --confirm=always --no-chain-reply-to --annotate \ --cover-letter \ --to=libvir-list@redhat.com master [ --subject-prefix="PATCH v#" ] where --subject-prefix would be necessary for v2, v3, v4, etc. Also the --cover-letter is only necessary for a 1 patch (so for this one it wouldn't be). You should take the text from your follow up emails and write a commit message to describe the patch does. It should "look like" other patches on the list - there's also the : http://libvirt.org/hacking.html to help you out. Also be sure to run "make check syntax-check" As for the patch itself...
diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 0d19d0e..d2a2a05 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -267,6 +267,10 @@ static const vshCmdOptDef opts_attach_disk[] = { .type = VSH_OT_STRING, .help = N_("cache mode of disk device") }, + {.name = "io", + .type = VSH_OT_STRING, + .help = N_("io mode of disk device") + },
I'd say this is not so much io mode, but rather io policy for thread model used by the driver (or something like that).
{.name = "type", .type = VSH_OT_STRING, .help = N_("target device type") @@ -504,8 +508,9 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virDomainPtr dom = NULL; const char *source = NULL, *target = NULL, *driver = NULL, *subdriver = NULL, *type = NULL, *mode = NULL, - *iothread = NULL, *cache = NULL, *serial = NULL, - *straddr = NULL, *wwn = NULL, *targetbus = NULL; + *iothread = NULL, *cache = NULL, *io = NULL, + *serial = NULL, *straddr = NULL, *wwn = NULL, + *targetbus = NULL; struct DiskAddress diskAddr; bool isFile = false, functionReturn = false; int ret; @@ -537,6 +542,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) vshCommandOptStringReq(ctl, cmd, "mode", &mode) < 0 || vshCommandOptStringReq(ctl, cmd, "iothread", &iothread) < 0 || vshCommandOptStringReq(ctl, cmd, "cache", &cache) < 0 || + vshCommandOptStringReq(ctl, cmd, "io", &io) < 0 || vshCommandOptStringReq(ctl, cmd, "serial", &serial) < 0 || vshCommandOptStringReq(ctl, cmd, "wwn", &wwn) < 0 || vshCommandOptStringReq(ctl, cmd, "address", &straddr) < 0 || @@ -579,7 +585,7 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAddLit(&buf, ">\n"); virBufferAdjustIndent(&buf, 2);
- if (driver || subdriver || iothread || cache) { + if (driver || subdriver || iothread || cache || io) { virBufferAddLit(&buf, "<driver");
if (driver) @@ -590,6 +596,8 @@ cmdAttachDisk(vshControl *ctl, const vshCmd *cmd) virBufferAsprintf(&buf, " iothread='%s'", iothread); if (cache) virBufferAsprintf(&buf, " cache='%s'", cache); + if (io) + virBufferAsprintf(&buf, " io='%s'", io);
virBufferAddLit(&buf, "/>\n"); } diff --git a/tools/virsh.pod b/tools/virsh.pod index cd1f25f..9656411 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod
above here... there's the : =item B<attach-disk> I<domain> I<source> I<target> [[[I<--live>] [I<--config>] ... You'll need to an "[I<--io io>]" within the list somewhere.
@@ -2860,6 +2860,8 @@ I<mode> can specify the two specific mode I<readonly> or I<shareable>. I<sourcetype> can indicate the type of source (block|file) I<cache> can be one of "default", "none", "writethrough", "writeback", "directsync" or "unsafe". +I<io> io is "threads", or "native" and selects between pthread based disk +I/O and native Linux AIO. I<iothread> is the number within the range of domain IOThreads to which this disk may be attached (QEMU only). I<serial> is the serial of disk device. I<wwn> is the wwn of disk device.
Feel free to liberally borrow from the formatdomain description: The optional io attribute controls specific policies on I/O; qemu guests support "threads" and "native". John
participants (3)
-
Daniel P. Berrange -
Gordon Messmer -
John Ferlan