util: mdev: support persistent devices with mdevctl

[libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

Posted by Jonathon Jongsma 4 years, 8 months ago

When a host is rebooted, mediated devices disappear from sysfs.  mdevctl
(https://github.com/mdevctl/mdevctl) is a utility for managing and
persisting these devices. It maintains a registry of mediated devices
and can start and stop them by UUID.

when libvirt attempts to create a new mediated device object, we currently
fail if the path does not exist in sysfs. This patch tries a little bit
harder by using mdevctl to attempt to activate the device.  The approach
is fairly naive -- it just calls 'mdevctl start -u $UUID' without
checking whether this UUID is registered with mdevctl or not.

See https://bugzilla.redhat.com/show_bug.cgi?id=1699274

Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
NOTES:
- an argument could be made that we should simply do nothing here. mdevctl does
  have support for automatically activating the mediated device when the parent
  device becomes available (via udev). So if the administrator set up the mdev
  to start automatically, this patch should not even be necessary. That said, I
  think this patch could still be useful.
- As I said above, the approach is pretty naive. I could have checked whether
  the device was defined in mdevctl's registry and given a different error
  message ("try registering this device with mdevctl") in that scenario. But
  I chose to keep it simple.

 src/util/virmdev.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 3d5488cdae..7a2f0adedc 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -25,6 +25,7 @@
 #include "virfile.h"
 #include "virstring.h"
 #include "viralloc.h"
+#include "vircommand.h"
 
 #define VIR_FROM_THIS VIR_FROM_NONE
 
@@ -148,9 +149,24 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model)
         return NULL;
 
     if (!virFileExists(sysfspath)) {
-        virReportError(VIR_ERR_DEVICE_MISSING,
-                       _("mediated device '%s' not found"), uuidstr);
-        return NULL;
+        bool activated = false;
+        /* if the mdev device path doesn't exist, we may still be able to start
+         * the device using mdevctl
+         */
+        char *mdevctl = virFindFileInPath("mdevctl");
+        if (mdevctl) {
+            const char *runargs[] = { mdevctl, "start", "-u", uuidstr, NULL };
+            if (virRun(runargs, NULL) == 0) {
+                /* check to see if it the device exists now */
+                activated = virFileExists(sysfspath);
+            }
+        }
+
+        if (!activated) {
+            virReportError(VIR_ERR_DEVICE_MISSING,
+                           _("mediated device '%s' not found"), uuidstr);
+            return NULL;
+        }
     }
 
     if (VIR_ALLOC(dev) < 0)
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

Posted by Fabiano Fidêncio 4 years, 8 months ago

On Wed, Aug 14, 2019 at 12:03 AM Jonathon Jongsma <jjongsma@redhat.com> wrote:
>
> When a host is rebooted, mediated devices disappear from sysfs.  mdevctl
> (https://github.com/mdevctl/mdevctl) is a utility for managing and
> persisting these devices. It maintains a registry of mediated devices
> and can start and stop them by UUID.
>
> when libvirt attempts to create a new mediated device object, we currently
> fail if the path does not exist in sysfs. This patch tries a little bit
> harder by using mdevctl to attempt to activate the device.  The approach
> is fairly naive -- it just calls 'mdevctl start -u $UUID' without
> checking whether this UUID is registered with mdevctl or not.
>
> See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> NOTES:
> - an argument could be made that we should simply do nothing here. mdevctl does
>   have support for automatically activating the mediated device when the parent
>   device becomes available (via udev). So if the administrator set up the mdev
>   to start automatically, this patch should not even be necessary. That said, I
>   think this patch could still be useful.
> - As I said above, the approach is pretty naive. I could have checked whether
>   the device was defined in mdevctl's registry and given a different error
>   message ("try registering this device with mdevctl") in that scenario. But
>   I chose to keep it simple.

So, I've noticed that mdevctl is not on Fedora yet, at least according
to: https://bugzilla.redhat.com/show_bug.cgi?id=1728381

In any case, would you mind to also update the spec file and add
something like: `Recommends: mdevctl`?

Recommends is a weak dependency. It shouldn't force mdevctl to be
installed, but suggests it to be installed.

Best Regards,
-- 
Fabiano Fidêncio

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

Posted by Boris Fiuczynski 4 years, 8 months ago

On 8/14/19 12:02 AM, Jonathon Jongsma wrote:
> When a host is rebooted, mediated devices disappear from sysfs.  mdevctl
> (https://github.com/mdevctl/mdevctl) is a utility for managing and
> persisting these devices. It maintains a registry of mediated devices
> and can start and stop them by UUID.
> 
> when libvirt attempts to create a new mediated device object, we currently
> fail if the path does not exist in sysfs. This patch tries a little bit
> harder by using mdevctl to attempt to activate the device.  The approach
> is fairly naive -- it just calls 'mdevctl start -u $UUID' without
> checking whether this UUID is registered with mdevctl or not.
> 
> See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
> 
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> NOTES:
> - an argument could be made that we should simply do nothing here. mdevctl does
>    have support for automatically activating the mediated device when the parent
>    device becomes available (via udev). So if the administrator set up the mdev
>    to start automatically, this patch should not even be necessary. That said, I
>    think this patch could still be useful.

I would actually like to use this argument since this patch 
unconditionally starts/creates a persistently defined mdev without ever 
stopping/destroying it and also not looking if it is defined as to be 
automatically started or manually started. If the mdev is specified in 
mdevctl to be started automatically I would rate this as mdevctl is in 
control of this mdevs lifecycle and libvirt should not interfere with 
it. It might be that I am over-interpreting auto and manual as start 
options in mdevctl but it feels to me that libvirt and mdevctl should 
not run into a management clash of host resources.

In addition what about a user specifiable tag in the domain xmls mdev 
definition which controls the management behavior of an mdev with 
mdevctl or another tool?


> - As I said above, the approach is pretty naive. I could have checked whether
>    the device was defined in mdevctl's registry and given a different error
>    message ("try registering this device with mdevctl") in that scenario. But
>    I chose to keep it simple.
> 
>   src/util/virmdev.c | 22 +++++++++++++++++++---
>   1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 3d5488cdae..7a2f0adedc 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -25,6 +25,7 @@
>   #include "virfile.h"
>   #include "virstring.h"
>   #include "viralloc.h"
> +#include "vircommand.h"
>   
>   #define VIR_FROM_THIS VIR_FROM_NONE
>   
> @@ -148,9 +149,24 @@ virMediatedDeviceNew(const char *uuidstr, virMediatedDeviceModelType model)
>           return NULL;
>   
>       if (!virFileExists(sysfspath)) {
> -        virReportError(VIR_ERR_DEVICE_MISSING,
> -                       _("mediated device '%s' not found"), uuidstr);
> -        return NULL;
> +        bool activated = false;
> +        /* if the mdev device path doesn't exist, we may still be able to start
> +         * the device using mdevctl
> +         */
> +        char *mdevctl = virFindFileInPath("mdevctl");
> +        if (mdevctl) {
> +            const char *runargs[] = { mdevctl, "start", "-u", uuidstr, NULL };
> +            if (virRun(runargs, NULL) == 0) {
> +                /* check to see if it the device exists now */
> +                activated = virFileExists(sysfspath);
> +            }
> +        }
> +
> +        if (!activated) {
> +            virReportError(VIR_ERR_DEVICE_MISSING,
> +                           _("mediated device '%s' not found"), uuidstr);
> +            return NULL;
> +        }
>       }
>   
>       if (VIR_ALLOC(dev) < 0)
> 


-- 
Mit freundlichen Grüßen/Kind regards
    Boris Fiuczynski

IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

Posted by Jonathon Jongsma 4 years, 8 months ago

On Wed, 2019-08-14 at 16:14 +0200, Boris Fiuczynski wrote:
> On 8/14/19 12:02 AM, Jonathon Jongsma wrote:
> > When a host is rebooted, mediated devices disappear from
> > sysfs.  mdevctl
> > (https://github.com/mdevctl/mdevctl) is a utility for managing and
> > persisting these devices. It maintains a registry of mediated
> > devices
> > and can start and stop them by UUID.
> > 
> > when libvirt attempts to create a new mediated device object, we
> > currently
> > fail if the path does not exist in sysfs. This patch tries a little
> > bit
> > harder by using mdevctl to attempt to activate the device.  The
> > approach
> > is fairly naive -- it just calls 'mdevctl start -u $UUID' without
> > checking whether this UUID is registered with mdevctl or not.
> > 
> > See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
> > 
> > Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> > ---
> > NOTES:
> > - an argument could be made that we should simply do nothing here.
> > mdevctl does
> >    have support for automatically activating the mediated device
> > when the parent
> >    device becomes available (via udev). So if the administrator set
> > up the mdev
> >    to start automatically, this patch should not even be necessary.
> > That said, I
> >    think this patch could still be useful.
> 
> I would actually like to use this argument since this patch 
> unconditionally starts/creates a persistently defined mdev without
> ever 
> stopping/destroying it and also not looking if it is defined as to
> be 
> automatically started or manually started. If the mdev is specified
> in 
> mdevctl to be started automatically I would rate this as mdevctl is
> in 
> control of this mdevs lifecycle and libvirt should not interfere
> with 
> it. It might be that I am over-interpreting auto and manual as start 
> options in mdevctl but it feels to me that libvirt and mdevctl
> should 
> not run into a management clash of host resources.
> 
> In addition what about a user specifiable tag in the domain xmls
> mdev 
> definition which controls the management behavior of an mdev with 
> mdevctl or another tool?

Thanks for the feedback. I welcome additional opinions on this. If
there's a concensus that the right approach is to do nothing, I can
drop this patch. Or alternately, we could simply point users toward
mdevctl in the error message. For example, something like:


diff --git a/src/util/virmdev.c b/src/util/virmdev.c
index 3d5488cdae..70d990eace 100644
--- a/src/util/virmdev.c
+++ b/src/util/virmdev.c
@@ -149,7 +149,7 @@ virMediatedDeviceNew(const char *uuidstr,
virMediatedDeviceModelType model)
 
     if (!virFileExists(sysfspath)) {
         virReportError(VIR_ERR_DEVICE_MISSING,
-                       _("mediated device '%s' not found"), uuidstr);
+                       _("mediated device '%s' not found. Persistent
devices can be managed with 'mdevctl'."), uuidstr);
         return NULL;
     }
 


> 
> 
> > - As I said above, the approach is pretty naive. I could have
> > checked whether
> >    the device was defined in mdevctl's registry and given a
> > different error
> >    message ("try registering this device with mdevctl") in that
> > scenario. But
> >    I chose to keep it simple.
> > 
> >   src/util/virmdev.c | 22 +++++++++++++++++++---
> >   1 file changed, 19 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> > index 3d5488cdae..7a2f0adedc 100644
> > --- a/src/util/virmdev.c
> > +++ b/src/util/virmdev.c
> > @@ -25,6 +25,7 @@
> >   #include "virfile.h"
> >   #include "virstring.h"
> >   #include "viralloc.h"
> > +#include "vircommand.h"
> >   
> >   #define VIR_FROM_THIS VIR_FROM_NONE
> >   
> > @@ -148,9 +149,24 @@ virMediatedDeviceNew(const char *uuidstr,
> > virMediatedDeviceModelType model)
> >           return NULL;
> >   
> >       if (!virFileExists(sysfspath)) {
> > -        virReportError(VIR_ERR_DEVICE_MISSING,
> > -                       _("mediated device '%s' not found"),
> > uuidstr);
> > -        return NULL;
> > +        bool activated = false;
> > +        /* if the mdev device path doesn't exist, we may still be
> > able to start
> > +         * the device using mdevctl
> > +         */
> > +        char *mdevctl = virFindFileInPath("mdevctl");
> > +        if (mdevctl) {
> > +            const char *runargs[] = { mdevctl, "start", "-u",
> > uuidstr, NULL };
> > +            if (virRun(runargs, NULL) == 0) {
> > +                /* check to see if it the device exists now */
> > +                activated = virFileExists(sysfspath);
> > +            }
> > +        }
> > +
> > +        if (!activated) {
> > +            virReportError(VIR_ERR_DEVICE_MISSING,
> > +                           _("mediated device '%s' not found"),
> > uuidstr);
> > +            return NULL;
> > +        }
> >       }
> >   
> >       if (VIR_ALLOC(dev) < 0)
> > 
> 
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] util: mdev: support persistent devices with mdevctl

Posted by Michal Privoznik 4 years, 8 months ago

On 8/19/19 4:48 PM, Jonathon Jongsma wrote:
> On Wed, 2019-08-14 at 16:14 +0200, Boris Fiuczynski wrote:
>> On 8/14/19 12:02 AM, Jonathon Jongsma wrote:
>>> When a host is rebooted, mediated devices disappear from
>>> sysfs.  mdevctl
>>> (https://github.com/mdevctl/mdevctl) is a utility for managing and
>>> persisting these devices. It maintains a registry of mediated
>>> devices
>>> and can start and stop them by UUID.
>>>
>>> when libvirt attempts to create a new mediated device object, we
>>> currently
>>> fail if the path does not exist in sysfs. This patch tries a little
>>> bit
>>> harder by using mdevctl to attempt to activate the device.  The
>>> approach
>>> is fairly naive -- it just calls 'mdevctl start -u $UUID' without
>>> checking whether this UUID is registered with mdevctl or not.
>>>
>>> See https://bugzilla.redhat.com/show_bug.cgi?id=1699274
>>>
>>> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
>>> ---
>>> NOTES:
>>> - an argument could be made that we should simply do nothing here.
>>> mdevctl does
>>>     have support for automatically activating the mediated device
>>> when the parent
>>>     device becomes available (via udev). So if the administrator set
>>> up the mdev
>>>     to start automatically, this patch should not even be necessary.
>>> That said, I
>>>     think this patch could still be useful.
>>
>> I would actually like to use this argument since this patch
>> unconditionally starts/creates a persistently defined mdev without
>> ever
>> stopping/destroying it and also not looking if it is defined as to
>> be
>> automatically started or manually started. If the mdev is specified
>> in
>> mdevctl to be started automatically I would rate this as mdevctl is
>> in
>> control of this mdevs lifecycle and libvirt should not interfere
>> with
>> it. It might be that I am over-interpreting auto and manual as start
>> options in mdevctl but it feels to me that libvirt and mdevctl
>> should
>> not run into a management clash of host resources.
>>
>> In addition what about a user specifiable tag in the domain xmls
>> mdev
>> definition which controls the management behavior of an mdev with
>> mdevctl or another tool?
> 
> Thanks for the feedback. I welcome additional opinions on this. If
> there's a concensus that the right approach is to do nothing, I can
> drop this patch. Or alternately, we could simply point users toward
> mdevctl in the error message. For example, something like:
> 
> 
> diff --git a/src/util/virmdev.c b/src/util/virmdev.c
> index 3d5488cdae..70d990eace 100644
> --- a/src/util/virmdev.c
> +++ b/src/util/virmdev.c
> @@ -149,7 +149,7 @@ virMediatedDeviceNew(const char *uuidstr,
> virMediatedDeviceModelType model)
>   
>       if (!virFileExists(sysfspath)) {
>           virReportError(VIR_ERR_DEVICE_MISSING,
> -                       _("mediated device '%s' not found"), uuidstr);
> +                       _("mediated device '%s' not found. Persistent
> devices can be managed with 'mdevctl'."), uuidstr);
>           return NULL;
>       }
>   

Unless there is a request for us to start mdevs, I'd rather see this 
error message than us trying to do anything because libvirt's already 
trying to manage more than enough and it's biting us back.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list