Re: [libvirt PATCH 2/2] hostdev: mdev: Lookup mdevs by sysfs path rather than mdev struct

On a Thursday in 2021, Erik Skultety wrote: > The lookup didn't do anything apart from comparing the sysfs paths > anyway since that's what makes each mdev unique. > The most ridiculous usage of the old logic was in > virHostdevReAttachMediatedDevices where in order to drop an mdev > hostdev from the list of active devices we first had to create a new > mdev and use it in the lookup call. Why couldn't we have used the > hostdev directly? Because the hostdev and mdev structures are > incompatible. > > The way mdevs are currently removed is via a write to a specific sysfs > attribute. If you do it while the machine which has the mdev assigned > is running, the write call may block (this should return a write error > instead and it is likely a bug in the vendor driver) until the device

> is no longer bound to vfio which is when the QEMU process exits. > > The interesting part here comes afterwards when we're cleaning up and > call virHostdevReAttachMediatedDevices. The domain doesn't exist > anymore, so the list of active hostdevs needs to be updated and the > respective hostdevs removed from the list, but remember we had to > create an mdev object in the memory in order to find it in the list > first which will fail because the write to sysfs had already removed > the mdev instance from the host system. > And so the next time you try to start the same domain you'll get: > > "Requested operation is not valid: mediated device <path> is in use by > driver QEMU, domain <name>" > Is there a public bug/issue you could link here?

> Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; > --- > src/hypervisor/virhostdev.c | 10 ++++------ > src/util/virmdev.c | 16 ++++++++-------- > src/util/virmdev.h | 4 ++-- > 3 files changed, 14 insertions(+), 16 deletions(-) > > diff --git a/src/hypervisor/virhostdev.c b/src/hypervisor/virhostdev.c > index aa3fc8738f..ea04382d0d 100644 > --- a/src/hypervisor/virhostdev.c > +++ b/src/hypervisor/virhostdev.c > @@ -1975,7 +1975,7 @@ virHostdevReAttachMediatedDevices(virHostdevManagerPtr mgr, > > virObjectLock(mgr->activeMediatedHostdevs); > for (i = 0; i < nhostdevs; i++) { > - g_autoptr(virMediatedDevice) mdev = NULL; > + g_autofree char * sysfspath = NULL; extra space > virMediatedDevicePtr tmp; > virDomainHostdevSubsysMediatedDevPtr mdevsrc; > virDomainHostdevDefPtr hostdev = hostdevs[i]; Reviewed-by: Ján Tomko <jtomko(a)redhat.com&gt;