2:51 p.m.
On 7/24/23 8:05 AM, Peter Krempa wrote:
As I've promised a long time ago I gave your patches some testing
in
regards of cooperation with blockjobs and snapshots.
Since the new version of the patches was not yet posted on the list I'm
replying here including my observations from testing patches from your
gitlab branch:
On Tue, Jun 06, 2023 at 16:15:30 -0500, Jonathon Jongsma wrote:
> Requires recent qemu with support for the virtio-blk-vhost-vdpa device
> and the ability to pass a /dev/fdset/N path for the vdpa path (8.1.0)
>
> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1900770
Since this is a feature addition the 'Fixes' keyword doesn't make sense.
Use e.g. 'Resolves' instead.
Additionally you're missing the DCO certification here.
> ---
> src/qemu/qemu_block.c | 20 ++++++++--
> src/qemu/qemu_domain.c | 25 ++++++++++++
> src/qemu/qemu_validate.c | 44 +++++++++++++++++++---
> tests/qemuxml2argvdata/disk-vhostvdpa.args | 35 +++++++++++++++++
> tests/qemuxml2argvdata/disk-vhostvdpa.xml | 21 +++++++++++
> tests/qemuxml2argvtest.c | 2 +
> 6 files changed, 139 insertions(+), 8 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.args
> create mode 100644 tests/qemuxml2argvdata/disk-vhostvdpa.xml
[...]
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index 2f6b32e394..119e52a7d7 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -11157,6 +11157,28 @@ qemuDomainPrepareStorageSourceFDs(virStorageSource *src,
> }
>
>
> +static int
> +qemuDomainPrepareStorageSourceVDPA(virStorageSource *src,
> + qemuDomainObjPrivate *priv)
> +{
> + qemuDomainStorageSourcePrivate *srcpriv = NULL;
> + virStorageType actualType = virStorageSourceGetActualType(src);
> + int vdpafd = -1;
> +
> + if (actualType != VIR_STORAGE_TYPE_VHOST_VDPA)
> + return 0;
> +
> + if ((vdpafd = qemuVDPAConnect(src->path)) < 0)
> + return -1;
This function call directly touches the host filesystem, which is not
supposed to be in the *DomainPrepareStorageSource* functions but we
rather have a completely separate machinery in
qemuProcessPrepareHostStorage.
Unfortunately that one doesn't yet need to handle individual backing
chain members though.
This ensures that the code doesn't get accidentally called from tests
even without mocking the code as the tests reimplement the functions
differently for testing purposes.
Somehow I missed this comment earlier. Unfortunately, it doesn't seem
straightforward to move this code. We can't simply move all of the logic
to qemuProcessPrepareHostStorage() because that function doesn't get
called during the tests. I thought I could move only the opening of the
fd to the PrepareHostStorage() function and then keep the qemuFDPass
construction in this function, but that doesn't work: the
PrepareHostStorage() function actually gets called *after* this
function. So the fd would not even be open yet at the time this function
gets called.
So... it seems that the options are either:
- leave everything in qemuDomainPrepareStorageSourceVDPA() (as is)
- move the fd opening to PrepareHostStorage() and then move the rest to
a different common function that is called after that, such as
qemuBuildDiskSourceCommandLine()
- a third option (suggestions?)
It's worth noting that the vdpa *network* device essentially does
everything (opening the file, creating the qemuFDPass object, etc) in
the qemuBuildInterfaceCommandLine() function. This was done to match
other network devices that connect to open file descriptors (see
qemuBuildInterfaceConnect()). But based on your comments above, it
sounds like this may not be a the ideal situation even though the
function is mocked to not actually open any file descriptors from the
host filesystem under test.
Jonathon
> +
> + srcpriv = qemuDomainStorageSourcePrivateFetch(src);
> +
> + srcpriv->fdpass = qemuFDPassNew(src->nodestorage, priv);
> + qemuFDPassAddFD(srcpriv->fdpass, &vdpafd, "-vdpa");
> + return 0;
> +}
[...]
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 9dce908cfe..67b0944162 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -3220,6 +3220,28 @@ qemuValidateDomainDeviceDefDiskTransient(const
virDomainDiskDef *disk,
> }
>
>
> +static int
> +qemuValidateDomainDeviceDefDiskVhost(const virDomainDef *def,
> + virStorageType storagetype,
> + virQEMUCapsFlags flag,
> + virQEMUCaps *qemuCaps)
> +{
> + const char *vhosttype = virStorageTypeToString(storagetype);
> +
> + if (!virQEMUCapsGet(qemuCaps, flag)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
> + _("%1$s disk is not supported with this QEMU
binary"),
> + vhosttype);
> + return -1;
I'd prefer if both things this function does are duplicated inline below
rather than passing it via arguments here. It makes it harder to follow.
> + }
> +
> + if (qemuValidateDomainDefVhostUserRequireSharedMemory(def, vhosttype) < 0)
> + return -1;
> +
> + return 0;
> +}
In my testing of the code from the new branch I've observed that
blockjobs and snapshot creation work well thanks to libblkio, so we
don't have to add any additional checks or limitations.
I'll still need to go ahead and finish the series removing the 'raw'
driver when it's not necessary so that the fast-path, once implemented
will be possible. Waiting for that is not necessary for this series as
it works properly even with the 'raw' driver in place.
With your new version of the patches I've noticed the following
problems:
- After converting to store the vdpa device path in src->vdpadev:
- rejecting of the empty disk source doesn't work for vdpa. If you use
<source/> in stead of the proper path, the XML will be defined but
broken.
- virStorageSourceCopy doesn't copy the new member (as instructed in
the comment above the struct), thus block job code which uses this
extensively to work on the inactive definition creates broken
configurations.
I've also noticed that using 'qcow2' format for the device doesn't work:
error: internal error: process exited while connecting to monitor:
2023-07-24T12:54:15.818631Z qemu-system-x86_64: -blockdev
{"node-name":"libvirt-1-format","read-only":false,"cache":{"direct":true,"no-flush":false},"driver":"qcow2","file":"libvirt-1-storage"}:
Could not read qcow2 header: Invalid argument
If that is supposed to work, then qemu devs will probably need to know
about that, if that is not supposed to work, libvirt needs to add a
check, because the error doesn't tell much. It's also possible I've
messed up when formatting the image though, as didn't really try to
figure out what's happening.