Re: [libvirt PATCH 6/7] nodedev: Implement virNodeDeviceIsPersistent()/IsActive()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Jun 22, 2021 at 2:08 AM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote:
>
> On 6/14/21 10:46 PM, Jonathon Jongsma wrote:
> > On Mon, Jun 14, 2021 at 12:27 PM Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx> wrote:
> >>
> >> On 6/3/21 10:11 PM, Jonathon Jongsma wrote:
> >>> Implement these new API functions in the nodedev driver.
> >>>
> >>> Signed-off-by: Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> >>> ---
> >>>    src/node_device/node_device_driver.c | 50 ++++++++++++++++++++++++++++
> >>>    src/node_device/node_device_driver.h |  6 ++++
> >>>    src/node_device/node_device_udev.c   | 21 +++++++-----
> >>>    3 files changed, 69 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c
> >>> index 9ebe609aa4..75391f18b8 100644
> >>> --- a/src/node_device/node_device_driver.c
> >>> +++ b/src/node_device/node_device_driver.c
> >>> @@ -1804,3 +1804,53 @@ nodeDeviceGetAutostart(virNodeDevice *device,
> >>>        virNodeDeviceObjEndAPI(&obj);
> >>>        return ret;
> >>>    }
> >>> +
> >>> +
> >>> +int
> >>> +nodeDeviceIsPersistent(virNodeDevice *device)
> >>> +{
> >>> +    virNodeDeviceObj *obj = NULL;
> >>> +    virNodeDeviceDef *def = NULL;
> >>> +    int ret = -1;
> >>> +
> >>> +    if (nodeDeviceInitWait() < 0)
> >>> +        return -1;
> >>> +
> >>> +    if (!(obj = nodeDeviceObjFindByName(device->name)))
> >>> +        return -1;
> >>> +    def = virNodeDeviceObjGetDef(obj);
> >>> +
> >>> +    if (virNodeDeviceIsPersistentEnsureACL(device->conn, def) < 0)
> >>> +        goto cleanup;
> >>> +
> >>> +    ret = virNodeDeviceObjIsPersistent(obj);
> >>> +
> >>> + cleanup:
> >>> +    virNodeDeviceObjEndAPI(&obj);
> >>> +    return ret;
> >>> +}
> >>> +
> >>> +
> >>> +int
> >>> +nodeDeviceIsActive(virNodeDevice *device)
> >>> +{
> >>> +    virNodeDeviceObj *obj = NULL;
> >>> +    virNodeDeviceDef *def = NULL;
> >>> +    int ret = -1;
> >>> +
> >>> +    if (nodeDeviceInitWait() < 0)
> >>> +        return -1;
> >>> +
> >>> +    if (!(obj = nodeDeviceObjFindByName(device->name)))
> >>> +        return -1;
> >>> +    def = virNodeDeviceObjGetDef(obj);
> >>> +
> >>> +    if (virNodeDeviceIsActiveEnsureACL(device->conn, def) < 0)
> >>> +        goto cleanup;
> >>> +
> >>> +    ret = virNodeDeviceObjIsActive(obj);
> >>> +
> >>> + cleanup:
> >>> +    virNodeDeviceObjEndAPI(&obj);
> >>> +    return ret;
> >>> +}
> >>> diff --git a/src/node_device/node_device_driver.h b/src/node_device/node_device_driver.h
> >>> index d178a18180..744dd42632 100644
> >>> --- a/src/node_device/node_device_driver.h
> >>> +++ b/src/node_device/node_device_driver.h
> >>> @@ -180,6 +180,12 @@ int
> >>>    nodeDeviceGetAutostart(virNodeDevice *dev,
> >>>                           int *autostart);
> >>>
> >>> +int
> >>> +nodeDeviceIsPersistent(virNodeDevice *dev);
> >>> +
> >>> +int
> >>> +nodeDeviceIsActive(virNodeDevice *dev);
> >>> +
> >>>    virCommand*
> >>>    nodeDeviceGetMdevctlSetAutostartCommand(virNodeDeviceDef *def,
> >>>                                            bool autostart,
> >>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> >>> index 21273083a6..eb15ccce7f 100644
> >>> --- a/src/node_device/node_device_udev.c
> >>> +++ b/src/node_device/node_device_udev.c
> >>> @@ -1487,7 +1487,7 @@ udevAddOneDevice(struct udev_device *device)
> >>>        virObjectEvent *event = NULL;
> >>>        bool new_device = true;
> >>>        int ret = -1;
> >>> -    bool was_persistent = false;
> >>> +    bool persistent = true;
> >>>        bool autostart = true;
> >>>        bool is_mdev;
> >>>
> >>> @@ -1518,7 +1518,8 @@ udevAddOneDevice(struct udev_device *device)
> >>>
> >>>            if (is_mdev)
> >>>                nodeDeviceDefCopyFromMdevctl(def, objdef);
> >>> -        was_persistent = virNodeDeviceObjIsPersistent(obj);
> >>> +
> >>> +        persistent = virNodeDeviceObjIsPersistent(obj);
> >>>            autostart = virNodeDeviceObjIsAutostart(obj);
> >>>
> >>>            /* If the device was defined by mdevctl and was never instantiated, it
> >>> @@ -1527,11 +1528,12 @@ udevAddOneDevice(struct udev_device *device)
> >>>
> >>>            virNodeDeviceObjEndAPI(&obj);
> >>>        } else {
> >>> -        /* All non-mdev devices report themselves as autostart since they
> >>> -         * should still be present and active after a reboot unless the device
> >>> -         * is removed from the host. Mediated devices can only be persistent if
> >>> -         * they are in already in the device list from parsing the mdevctl
> >>> -         * output. */
> >>> +        /* All non-mdev devices report themselves as persistent and autostart
> >>> +         * since they should still be present and active after a reboot unless
> >>> +         * the device is removed from the host. Mediated devices can only be
> >>> +         * persistent if they are in already in the device list from parsing
> >>> +         * the mdevctl output. */
> >>
> >> The assumption for all non-mdev devices ends up very misleading.
> >> For example: The parent device of an mdev needs to be bound to a vfio
> >> device driver. Without it the device ends up without the capability to
> >> create mdevs.
> >> If this driver binding is not persisted (e.g. with setting up driverctl)
> >> but instead the device is just manually being rebound to a vfio device
> >> driver than after reboot neither the mdev capability on the parent
> >> device nor the mdev device as a child device will be existing.
> >> A user calling nodedev-info before the reboot gets
> >> on the parent device
> >>          Persistent:     yes
> >>          Autostart:      yes
> >> and on the mdev device
> >>          Persistent:     yes
> >>          Autostart:      yes
> >> After a reboot he ends up with with nodedev-info
> >> on the parent device
> >>          Persistent:     yes
> >>          Autostart:      yes
> >> and the mdev device does not exist.
> >
> > Before I get to the rest of your suggestion, I'd like to know more
> > about this. If the mdev device is persistent (i.e. "defined" in
> > mdevctl terminology), then it should still exist after a reboot, even
> > if it's not started. If it doesn't, then it's a bug. An mdev can be
> > defined even if its parent device is not present.
> >
> > Does this device show up if you run 'mdevctl list --defined'?
> > Does it show up if you run `virsh nodedev-list --cap mdev --all`?
> >
> >> I suggest to use three states like yes, no, unknown
> >> or not showing Persistent and Autostart on devices that libvirt does not
> >> manage/know about their persistence and autostart configuration.
> >
> > Well, I suppose that we have the error value (-1) that could be used
> > for this case, but it doesn't exactly seem like an error. Adding a
> > separate tri-state return value would make this API inconsistent with
> > all of the other IsActive()/IsPersistent() APIs in libvirt, so I don't
> > think that's a very good idea.
> >
> > Jonathon
> >
>
> Just noticed something while playing around with this.
> The default "yes" seems to be not really a good choice even for mdevs.
> See below:
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name:           mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent:         css_0_0_0033
> Active:         yes
> Persistent:     yes
> Autostart:      yes
>
> # virsh nodedev-destroy mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Destroyed node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
>
> # mdevctl list --defined
> e60cef97-3f6b-485e-ac46-0520f9f66ac2 0.0.0033 vfio_ccw-io auto
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name:           mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent:         css_0_0_0033
> Active:         no
> Persistent:     yes
> Autostart:      yes
>
> # virsh nodedev-dumpxml mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> <device>
>    <name>mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2</name>
>    <parent>css_0_0_0033</parent>
>    <capability type='mdev'>
>      <type id='vfio_ccw-io'/>
>      <uuid>e60cef97-3f6b-485e-ac46-0520f9f66ac2</uuid>
>      <iommuGroup number='1'/>
>    </capability>
> </device>
>
> # virsh nodedev-start mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Device mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2 started
>
> # virsh nodedev-undefine mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Undefined node device 'mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2'
>
> # virsh nodedev-info mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Name:           mdev_e60cef97_3f6b_485e_ac46_0520f9f66ac2
> Parent:         css_0_0_0033
> Active:         yes
> Persistent:     no
> Autostart:      yes

So it appears that there is a bug where an mdev is still marked as
autostart even after it's undefined. Was there anything else you were
trying to demonstrate?

Jonathon




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux