[libvirt PATCH 0/3] add support for Fibre Channel VMID
Pavel Hrdina (3): vircgroup: introduce virCgroupGetInode function conf: introduce support for Fibre Channel VMID qemu: implement support for Fibre Channel VMID docs/formatdomain.rst | 13 ++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++ src/conf/domain_conf.c | 5 ++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 32 +++++++++++++++++++++++++ src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 11 files changed, 143 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml -- 2.31.1
For new feature Fibre Channel VMID we will need to get inode of the VM root cgroup as it is used in the new kernel API together with VMID. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 3 files changed, 40 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..d8451fcfff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; +virCgroupGetInode; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryStat; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1b3b28342e..ea702e7b63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period, return 0; } + + +/** + * virCgroupGetInode: + * + * @cgroup: the cgroup to get inode for + * + * Get the @cgroup inode and return its value to the caller. + * + * Returns inode on success, -1 on error with error message reported. + */ +int +virCgroupGetInode(virCgroup *cgroup) +{ + VIR_AUTOCLOSE fd = 0; + struct stat st; + int controller = virCgroupGetAnyController(cgroup); + g_autofree char *path = NULL; + + if (controller < 0) + return -1; + + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) + return -1; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); + return -1; + } + + if (fstat(fd, &st) < 0) { + virReportSystemError(errno, _("failed to get fstat for '%s'"), path); + return -1; + } + + return st.st_ino; +} diff --git a/src/util/vircgroup.h b/src/util/vircgroup.h index 8002cac570..690f09465c 100644 --- a/src/util/vircgroup.h +++ b/src/util/vircgroup.h @@ -283,3 +283,5 @@ int virCgroupSetOwner(virCgroup *cgroup, int virCgroupHasEmptyTasks(virCgroup *cgroup, int controller); bool virCgroupControllerAvailable(int controller); + +int virCgroupGetInode(virCgroup *cgroup); -- 2.31.1
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
For new feature Fibre Channel VMID we will need to get inode of the VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..d8451fcfff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; +virCgroupGetInode; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryStat; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1b3b28342e..ea702e7b63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period,
return 0; } + + +/** + * virCgroupGetInode: + * + * @cgroup: the cgroup to get inode for + * + * Get the @cgroup inode and return its value to the caller. + * + * Returns inode on success, -1 on error with error message reported. + */ +int +virCgroupGetInode(virCgroup *cgroup) +{ + VIR_AUTOCLOSE fd = 0; + struct stat st; + int controller = virCgroupGetAnyController(cgroup); + g_autofree char *path = NULL; + + if (controller < 0) + return -1; + + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) + return -1; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); + return -1; + }
Is the open() necessary? Why isn't plain stat() enough? Michal
On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
For new feature Fibre Channel VMID we will need to get inode of the VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..d8451fcfff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; +virCgroupGetInode; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryStat; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1b3b28342e..ea702e7b63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period,
return 0; } + + +/** + * virCgroupGetInode: + * + * @cgroup: the cgroup to get inode for + * + * Get the @cgroup inode and return its value to the caller. + * + * Returns inode on success, -1 on error with error message reported. + */ +int +virCgroupGetInode(virCgroup *cgroup) +{ + VIR_AUTOCLOSE fd = 0; + struct stat st; + int controller = virCgroupGetAnyController(cgroup); + g_autofree char *path = NULL; + + if (controller < 0) + return -1; + + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) + return -1; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); + return -1; + }
Is the open() necessary? Why isn't plain stat() enough?
Good question :) I was lazy so I looked for first similar code in libvirt codebase and copied it. You are right, stat should be enough as according to man page stat and fstat are identical except or the fd vs path parameter. I'll fix it before pushing or should I send a v2? Pavel
On 8/4/21 10:22 AM, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
For new feature Fibre Channel VMID we will need to get inode of the VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..d8451fcfff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; +virCgroupGetInode; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryStat; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1b3b28342e..ea702e7b63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period,
return 0; } + + +/** + * virCgroupGetInode: + * + * @cgroup: the cgroup to get inode for + * + * Get the @cgroup inode and return its value to the caller. + * + * Returns inode on success, -1 on error with error message reported. + */ +int +virCgroupGetInode(virCgroup *cgroup) +{ + VIR_AUTOCLOSE fd = 0; + struct stat st; + int controller = virCgroupGetAnyController(cgroup); + g_autofree char *path = NULL; + + if (controller < 0) + return -1; + + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) + return -1; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); + return -1; + }
Is the open() necessary? Why isn't plain stat() enough?
Good question :) I was lazy so I looked for first similar code in libvirt codebase and copied it. You are right, stat should be enough as according to man page stat and fstat are identical except or the fd vs path parameter. I'll fix it before pushing or should I send a v2?
No need, just drop open() and switch to stat() before pushing. My Reviewed-by still applies. Michal
On Wed, Aug 04, 2021 at 10:26:41AM +0200, Michal Prívozník wrote:
On 8/4/21 10:22 AM, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:06:17AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
For new feature Fibre Channel VMID we will need to get inode of the VM root cgroup as it is used in the new kernel API together with VMID.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ 3 files changed, 40 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6961cdb137..d8451fcfff 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1920,6 +1920,7 @@ virCgroupGetCpuShares; virCgroupGetDevicePermsString; virCgroupGetDomainTotalCpuStats; virCgroupGetFreezerState; +virCgroupGetInode; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; virCgroupGetMemoryStat; diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 1b3b28342e..ea702e7b63 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -3973,3 +3973,40 @@ virCgroupGetCpuPeriodQuota(virCgroup *cgroup, unsigned long long *period,
return 0; } + + +/** + * virCgroupGetInode: + * + * @cgroup: the cgroup to get inode for + * + * Get the @cgroup inode and return its value to the caller. + * + * Returns inode on success, -1 on error with error message reported. + */ +int +virCgroupGetInode(virCgroup *cgroup) +{ + VIR_AUTOCLOSE fd = 0; + struct stat st; + int controller = virCgroupGetAnyController(cgroup); + g_autofree char *path = NULL; + + if (controller < 0) + return -1; + + if (virCgroupPathOfController(cgroup, controller, "", &path) < 0) + return -1; + + if ((fd = open(path, O_RDONLY | O_NONBLOCK)) < 0) { + virReportSystemError(errno, _("failed to open cgroup '%s'"), path); + return -1; + }
Is the open() necessary? Why isn't plain stat() enough?
Good question :) I was lazy so I looked for first similar code in libvirt codebase and copied it. You are right, stat should be enough as according to man page stat and fstat are identical except or the fd vs path parameter. I'll fix it before pushing or should I send a v2?
No need, just drop open() and switch to stat() before pushing. My Reviewed-by still applies.
OK, will do. Thanks for review! Pavel
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index 61ccd8895a..f6a1885acf 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2232,6 +2232,19 @@ event name Description ``emulation_faults`` the count of emulation faults, that is when the kernel traps on unimplemented instrucions and emulates them for user space, by applications running on the platform ``perf.emulation_faults`` =========================== ======================================================================================================================================================================================= ================================ +Fibre Channel VMID +------------------- + +The FC SAN can provide various QOS levels, access control depending on the +VMID. Also it can collect telemetry data at per-VM level which can be used +to enhance the IO performance of the VM. This can be configured by using +the ``vmid`` element. The element contains single string (max 128 bytes). + +Using this feature requires Fibre Channel capable HW, kernel compiled with +option ``CONFIG_BLK_CGROUP_FC_APPID`` and ``nvme_fc`` kernel module loaded. +:since:`Since 7.7.0` + + :anchor:`<a id="elementsDevices"/>` Devices diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 2442078969..d2e6039c76 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -62,6 +62,9 @@ <optional> <ref name="perf"/> </optional> + <optional> + <ref name="vmid"/> + </optional> <optional> <ref name="idmap"/> </optional> @@ -576,6 +579,14 @@ </element> </define> + <define name="vmid"> + <element name="vmid"> + <data type="string"> + <param name="pattern">.{1,128}</param> + </data> + </element> + </define> + <!-- The Identifiers can be: - an optional id attribute with a number on the domain element diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 09da4ab952..426ea03049 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -19646,6 +19646,8 @@ virDomainDefParseXML(xmlDocPtr xml, if (virDomainDefParseBootOptions(def, ctxt) < 0) goto error; + def->vmid = virXPathString("string(./vmid[1])", ctxt); + /* analysis of the disk devices */ if ((n = virXPathNodeSet("./devices/disk", ctxt, &nodes)) < 0) goto error; @@ -28161,6 +28163,9 @@ virDomainDefFormatInternalSetRootName(virDomainDef *def, virBufferAddLit(buf, "</clock>\n"); } + if (def->vmid) + virBufferAsprintf(buf, "<vmid>%s</vmid>\n", def->vmid); + if (virDomainEventActionDefFormat(buf, def->onPoweroff, "on_poweroff", virDomainLifecycleActionTypeToString) < 0) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9f32bcf9cf..4e5e2259c0 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -2803,6 +2803,8 @@ struct _virDomainDef { virDomainClockDef clock; + char *vmid; + size_t ngraphics; virDomainGraphicsDef **graphics; diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index a9e4519b1a..9d599e4bd6 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -55,6 +55,22 @@ virDomainDefBootValidate(const virDomainDef *def) } +static int +virDomainDefVMIDValidate(const virDomainDef *def) +{ + if (!def->vmid) + return 0; + + if (strlen(def->vmid) > 128) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("VMID cannot be longer then 128 characters")); + return -1; + } + + return 0; +} + + static int virDomainDefVideoValidate(const virDomainDef *def) { @@ -1538,6 +1554,9 @@ static int virDomainDefValidateInternal(const virDomainDef *def, virDomainXMLOption *xmlopt) { + if (virDomainDefVMIDValidate(def) < 0) + return -1; + if (virDomainDefDuplicateDiskInfoValidate(def) < 0) return -1; diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu-system-x86_64</emulator> + </devices> +</domain> diff --git a/tests/genericxml2xmltest.c b/tests/genericxml2xmltest.c index ef51ed91a6..e77f4ff386 100644 --- a/tests/genericxml2xmltest.c +++ b/tests/genericxml2xmltest.c @@ -225,6 +225,8 @@ mymain(void) DO_TEST_DIFFERENT("cputune"); DO_TEST("device-backenddomain"); + DO_TEST("vmid"); + #define DO_TEST_BACKUP_FULL(name, intrnl) \ do { \ const struct testCompareBackupXMLData data = { .testname = name, \ -- 2.31.1
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name. It is rather too generic and doesn't really give any suggestion as to what it is used for. 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 Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion as to what it is used for.
I was struggling with that as well. To be honest I don't know where to put it. In downstream BZ there was suggestion to place it under <blkiotune> but this is not tunning any IO so I don't thing it should belong there. We can move it closer to UUID and GENID and all this ID elements. Any idea is welcome :). Pavel
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion as to what it is used for.
I was struggling with that as well. To be honest I don't know where to put it. In downstream BZ there was suggestion to place it under <blkiotune> but this is not tunning any IO so I don't thing it should belong there. We can move it closer to UUID and GENID and all this ID elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in practice, and/or what format it is required to be in. I was wondering what would happen if we simply wrote the VM UUID into this attribute unconditionally, and thus avoid need for an XML element ? 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 Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion as to what it is used for.
I was struggling with that as well. To be honest I don't know where to put it. In downstream BZ there was suggestion to place it under <blkiotune> but this is not tunning any IO so I don't thing it should belong there. We can move it closer to UUID and GENID and all this ID elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in practice, and/or what format it is required to be in.
I was wondering what would happen if we simply wrote the VM UUID into this attribute unconditionally, and thus avoid need for an XML element ?
Right, should have provided some links probably, always forget to do that. We need to have it in XML as users will provide their own VMID and it can be anything, ti doesn't have to be VM UUID but it can be part of it. I was not able to find any specification, only some mentions from HW vendors: [1] "Virtual-machine awareness: The MDS 9148T provides visibility into all virtual machines that are accessing the storage LUNs in the fabric. This feature is available through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every Fibre Channel frame. Virtual-machine awareness can be extended to intelligent fabric services such as analytics1 to visualize performance of every flow originating from each virtual machine in the fabric." [2] "Provides visibility into each virtual machine logged into the fabric through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every FC frame." Fibre Channel VMID is can be included in every FC frame which allows monitoring or QoS and other things based on the VMID provided by users. The VMID can contain anything so a group of VMs can share the same VMID, for example "database". Or to allow per application and VM tracking another example is "database:$UUID". Originally before I started working on this feature I thought that the VM UUID will be added automatically to the user provided string but I realized that assumption was incorrect as the user doesn't necessarily want to or need to have the UUID as part of the FC VMID. I was just straggling how to model this in the XML. Pavel [1] <https://www.cisco.com/c/en/us/products/collateral/storage-networking/mds-9148t-32-gbps-48-port-fibre-channel-switch/data-sheet-c78-740623.html> [2] <https://buy.hpe.com/za/en/storage/storage-networking/c-series-switches/c-series-fabric-switches/hpe-c-series-sn6610c-fibre-channel-switch/hpe-sn6610c-32gb-8-port-16gb-short-wave-sfp-fibre-channel-switch/p/Q9D34A>
On Wed, Aug 04, 2021 at 11:54:07AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion as to what it is used for.
I was struggling with that as well. To be honest I don't know where to put it. In downstream BZ there was suggestion to place it under <blkiotune> but this is not tunning any IO so I don't thing it should belong there. We can move it closer to UUID and GENID and all this ID elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in practice, and/or what format it is required to be in.
I was wondering what would happen if we simply wrote the VM UUID into this attribute unconditionally, and thus avoid need for an XML element ?
Right, should have provided some links probably, always forget to do that. We need to have it in XML as users will provide their own VMID and it can be anything, ti doesn't have to be VM UUID but it can be part of it. I was not able to find any specification, only some mentions from HW vendors:
[1] "Virtual-machine awareness: The MDS 9148T provides visibility into all virtual machines that are accessing the storage LUNs in the fabric. This feature is available through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every Fibre Channel frame. Virtual-machine awareness can be extended to intelligent fabric services such as analytics1 to visualize performance of every flow originating from each virtual machine in the fabric."
[2] "Provides visibility into each virtual machine logged into the fabric through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every FC frame."
IIUC, the implication is that the VMID associated with QEMU gets associated with I/O issued from any <disk> devices that happen to be backed by the fibre channel HBA consuming it.
Fibre Channel VMID is can be included in every FC frame which allows monitoring or QoS and other things based on the VMID provided by users. The VMID can contain anything so a group of VMs can share the same VMID, for example "database". Or to allow per application and VM tracking another example is "database:$UUID".
Ok, so the "group of VMs can share the VMID" eliminates the option of unconditionally using the VM UUID. I'm inclined to suggest that we put it inside the <resource> element, since it is essentially a tunable for FC resource control. eg something like <resource> <fibrechannel appid="...."/> </resouce> 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 Wed, Aug 04, 2021 at 03:32:16PM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:54:07AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:32:00AM +0100, Daniel P. Berrangé wrote:
On Wed, Aug 04, 2021 at 11:19:35AM +0200, Pavel Hrdina wrote:
On Wed, Aug 04, 2021 at 10:09:30AM +0100, Daniel P. Berrangé wrote:
On Tue, Aug 03, 2021 at 04:29:32PM +0200, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- docs/formatdomain.rst | 13 +++++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++++ src/conf/domain_conf.c | 5 +++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++++++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 7 files changed, 71 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
diff --git a/tests/genericxml2xmlindata/vmid.xml b/tests/genericxml2xmlindata/vmid.xml new file mode 100644 index 0000000000..df984df3f5 --- /dev/null +++ b/tests/genericxml2xmlindata/vmid.xml @@ -0,0 +1,19 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static'>4</vcpu> + <os> + <type arch='x86_64' machine='q35'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <vmid>someapp:c7a5fdbd-edaf-9455-926a-d65c16db1809</vmid>
I'm not convinced by adding this element here with this name.
It is rather too generic and doesn't really give any suggestion as to what it is used for.
I was struggling with that as well. To be honest I don't know where to put it. In downstream BZ there was suggestion to place it under <blkiotune> but this is not tunning any IO so I don't thing it should belong there. We can move it closer to UUID and GENID and all this ID elements. Any idea is welcome :).
I'm struggling to find info on what this attribute actally does in practice, and/or what format it is required to be in.
I was wondering what would happen if we simply wrote the VM UUID into this attribute unconditionally, and thus avoid need for an XML element ?
Right, should have provided some links probably, always forget to do that. We need to have it in XML as users will provide their own VMID and it can be anything, ti doesn't have to be VM UUID but it can be part of it. I was not able to find any specification, only some mentions from HW vendors:
[1] "Virtual-machine awareness: The MDS 9148T provides visibility into all virtual machines that are accessing the storage LUNs in the fabric. This feature is available through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every Fibre Channel frame. Virtual-machine awareness can be extended to intelligent fabric services such as analytics1 to visualize performance of every flow originating from each virtual machine in the fabric."
[2] "Provides visibility into each virtual machine logged into the fabric through HBAs capable of priority tagging the Virtual Machine Identifier (VMID) on every FC frame."
IIUC, the implication is that the VMID associated with QEMU gets associated with I/O issued from any <disk> devices that happen to be backed by the fibre channel HBA consuming it.
Fibre Channel VMID is can be included in every FC frame which allows monitoring or QoS and other things based on the VMID provided by users. The VMID can contain anything so a group of VMs can share the same VMID, for example "database". Or to allow per application and VM tracking another example is "database:$UUID".
Ok, so the "group of VMs can share the VMID" eliminates the option of unconditionally using the VM UUID.
I'm inclined to suggest that we put it inside the <resource> element, since it is essentially a tunable for FC resource control.
eg something like
<resource> <fibrechannel appid="...."/> </resouce>
Sounds reasonable as this is somehow related to cgroups and we already have <partition> there. Thanks! I'll post v2 as I like this better as well. Pavel
Based on kernel commit messages the interface is /sys/class/fc/fc_udev_device/appid_store where we need to write the following string "$INODE:$VMID". $INODE is the VM root cgroup inode in hexadecimal and $VMID is user provided string that will be attached to each FC frame for the VM within the cgroup identified by inode and has limit 128 bytes. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f2d99abcfa..df021b5985 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm) } +static int +qemuSetupCgroupVMID(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int inode = virCgroupGetInode(priv->cgroup); + const char *path = "/sys/class/fc/fc_udev_device/appid_store"; + g_autofree char *appid = NULL; + + if (!vm->def->vmid) + return 0; + + appid = g_strdup_printf("%X:%s", inode, vm->def->vmid); + + if (virFileWriteStr(path, appid, 0) < 0) { + if (errno == ENOENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("FC VMID not available on this host")); + return -1; + } + + virReportSystemError(errno, + _("Unable to write '%s' to '%s'"), appid, path); + return -1; + } + + return 0; +} + + static int qemuInitCgroup(virDomainObj *vm, size_t nnicindexes, @@ -1096,6 +1125,9 @@ qemuSetupCgroup(virDomainObj *vm, if (qemuSetupCpusetCgroup(vm) < 0) return -1; + if (qemuSetupCgroupVMID(vm) < 0) + return -1; + return 0; } -- 2.31.1
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
Based on kernel commit messages the interface is
/sys/class/fc/fc_udev_device/appid_store
where we need to write the following string "$INODE:$VMID".
$INODE is the VM root cgroup inode in hexadecimal and $VMID is user provided string that will be attached to each FC frame for the VM within the cgroup identified by inode and has limit 128 bytes.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f2d99abcfa..df021b5985 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm) }
+static int +qemuSetupCgroupVMID(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int inode = virCgroupGetInode(priv->cgroup); + const char *path = "/sys/class/fc/fc_udev_device/appid_store"; + g_autofree char *appid = NULL; + + if (!vm->def->vmid) + return 0; + + appid = g_strdup_printf("%X:%s", inode, vm->def->vmid); + + if (virFileWriteStr(path, appid, 0) < 0) { + if (errno == ENOENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("FC VMID not available on this host")); + return -1; + } + + virReportSystemError(errno, + _("Unable to write '%s' to '%s'"), appid, path);
Alternatively, use the following pattern: if (errno == ENOENT) { virReportError(); } else { virReportSystemError(); } return -1; Or use just virReportSystemError(); I believe "Unable to write to /sys/class/...: No such file or directory" is pretty comprehensive. Michal
On Wed, Aug 04, 2021 at 10:06:21AM +0200, Michal Prívozník wrote:
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
Based on kernel commit messages the interface is
/sys/class/fc/fc_udev_device/appid_store
where we need to write the following string "$INODE:$VMID".
$INODE is the VM root cgroup inode in hexadecimal and $VMID is user provided string that will be attached to each FC frame for the VM within the cgroup identified by inode and has limit 128 bytes.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_cgroup.c | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+)
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c index f2d99abcfa..df021b5985 100644 --- a/src/qemu/qemu_cgroup.c +++ b/src/qemu/qemu_cgroup.c @@ -904,6 +904,35 @@ qemuSetupCpuCgroup(virDomainObj *vm) }
+static int +qemuSetupCgroupVMID(virDomainObj *vm) +{ + qemuDomainObjPrivate *priv = vm->privateData; + int inode = virCgroupGetInode(priv->cgroup); + const char *path = "/sys/class/fc/fc_udev_device/appid_store"; + g_autofree char *appid = NULL; + + if (!vm->def->vmid) + return 0; + + appid = g_strdup_printf("%X:%s", inode, vm->def->vmid); + + if (virFileWriteStr(path, appid, 0) < 0) { + if (errno == ENOENT) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("FC VMID not available on this host")); + return -1; + } + + virReportSystemError(errno, + _("Unable to write '%s' to '%s'"), appid, path);
Alternatively, use the following pattern:
if (errno == ENOENT) { virReportError(); } else { virReportSystemError(); }
return -1;
Or use just virReportSystemError(); I believe "Unable to write to /sys/class/...: No such file or directory" is pretty comprehensive.
I wanted to have nice explanatory error messages where missing presence of the file means that the kernel is old and doesn't have the feature implemented at all. I wanted to do the same for case where the feature is not enabled in kernel config but that is basically impossible. The file will still be present but any write to it will return EINVAL. However, the same happens for actual invalid string format or if invalid inode and so on. I guess I'll drop this attempt too and use single error message as you've suggested. Pavel
On 8/3/21 4:29 PM, Pavel Hrdina wrote:
Pavel Hrdina (3): vircgroup: introduce virCgroupGetInode function conf: introduce support for Fibre Channel VMID qemu: implement support for Fibre Channel VMID
docs/formatdomain.rst | 13 ++++++++++ docs/schemas/domaincommon.rng | 11 +++++++++ src/conf/domain_conf.c | 5 ++++ src/conf/domain_conf.h | 2 ++ src/conf/domain_validate.c | 19 +++++++++++++++ src/libvirt_private.syms | 1 + src/qemu/qemu_cgroup.c | 32 +++++++++++++++++++++++++ src/util/vircgroup.c | 37 +++++++++++++++++++++++++++++ src/util/vircgroup.h | 2 ++ tests/genericxml2xmlindata/vmid.xml | 19 +++++++++++++++ tests/genericxml2xmltest.c | 2 ++ 11 files changed, 143 insertions(+) create mode 100644 tests/genericxml2xmlindata/vmid.xml
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (3)
-
Daniel P. Berrangé -
Michal Prívozník -
Pavel Hrdina