If we find ourselves in the situation that the 'add' uevent has been
fired earlier than the sysfs tree for a device was created, we should
use the best-effort approach and give kernel some predetermined amount
of time, thus waiting for the attributes to be ready rather than
discarding the device from our device list forever. If those don't appear
in the given time frame, we need to move on, since libvirt can't wait
indefinitely.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 src/node_device/node_device_udev.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 70e15ffb8..2f63256a3 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1166,9 +1166,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
     char *canonicalpath = NULL;
     virNodeDevCapMdevPtr data = &def->caps->data.mdev;
 
-    if (virAsprintf(&linkpath, "%s/mdev_type", udev_device_get_syspath(dev)) < 0)
+    /* Because of a kernel uevent race, we might get the 'add' event prior to
+     * the sysfs tree being ready, so any attempt to access any sysfs attribute
+     * would result in ENOENT and us dropping the device, so let's work around
+     * it by waiting for the attributes to become available.
+     */
+
+    if (virAsprintf(&linkpath, "%s/mdev_type",
+                    udev_device_get_syspath(dev)) < 0)
         goto cleanup;
 
+    if (virFileWaitForExists(linkpath, 1, 100) < 0) {
+        virReportSystemError(errno,
+                             _("failed to wait for file '%s' to appear"),
+                             linkpath);
+        goto cleanup;
+    }
+
     if (virFileResolveLink(linkpath, &canonicalpath) < 0) {
         virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
         goto cleanup;
-- 
2.13.5

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

On 09/18/2017 12:34 PM, Erik Skultety wrote:
> If we find ourselves in the situation that the 'add' uevent has been
> fired earlier than the sysfs tree for a device was created, we should
> use the best-effort approach and give kernel some predetermined amount
> of time, thus waiting for the attributes to be ready rather than
> discarding the device from our device list forever. If those don't appear
> in the given time frame, we need to move on, since libvirt can't wait
> indefinitely.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 70e15ffb8..2f63256a3 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1166,9 +1166,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
>      char *canonicalpath = NULL;
>      virNodeDevCapMdevPtr data = &def->caps->data.mdev;
>  
> -    if (virAsprintf(&linkpath, "%s/mdev_type", udev_device_get_syspath(dev)) < 0)
> +    /* Because of a kernel uevent race, we might get the 'add' event prior to
> +     * the sysfs tree being ready, so any attempt to access any sysfs attribute
> +     * would result in ENOENT and us dropping the device, so let's work around
> +     * it by waiting for the attributes to become available.
> +     */
> +
> +    if (virAsprintf(&linkpath, "%s/mdev_type",
> +                    udev_device_get_syspath(dev)) < 0)
>          goto cleanup;
>  
> +    if (virFileWaitForExists(linkpath, 1, 100) < 0) {
> +        virReportSystemError(errno,
> +                             _("failed to wait for file '%s' to appear"),
> +                             linkpath);
> +        goto cleanup;
> +    }
> +

So the linkpath gets created after the canonicalpath... and we don't
have to check that right?

Considering what I pointed out in my previous review - I wouldn't be
able to use virFileWaitForExists, but a similar loop would be possible.
For NPIV the file exists, it's the contents that are bogus momentarily.

Other consumers waiting that I looked at usually wait on some sort of
open() and usleep() when ENOENT is returned (there's multiple examples
if you search on ulseep).  Wonder if any of those could utilize
something like this...  I know patches welcome ;-)

Reviewed-by: John Ferlan <jferlan@redhat.com>


>      if (virFileResolveLink(linkpath, &canonicalpath) < 0) {
>          virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
>          goto cleanup;
> 

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

On Wed, Sep 20, 2017 at 10:33:14AM -0400, John Ferlan wrote:
>
>
> On 09/18/2017 12:34 PM, Erik Skultety wrote:
> > If we find ourselves in the situation that the 'add' uevent has been
> > fired earlier than the sysfs tree for a device was created, we should
> > use the best-effort approach and give kernel some predetermined amount
> > of time, thus waiting for the attributes to be ready rather than
> > discarding the device from our device list forever. If those don't appear
> > in the given time frame, we need to move on, since libvirt can't wait
> > indefinitely.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285
> >
> > Signed-off-by: Erik Skultety <eskultet@redhat.com>
> > ---
> >  src/node_device/node_device_udev.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> > index 70e15ffb8..2f63256a3 100644
> > --- a/src/node_device/node_device_udev.c
> > +++ b/src/node_device/node_device_udev.c
> > @@ -1166,9 +1166,23 @@ udevProcessMediatedDevice(struct udev_device *dev,
> >      char *canonicalpath = NULL;
> >      virNodeDevCapMdevPtr data = &def->caps->data.mdev;
> >
> > -    if (virAsprintf(&linkpath, "%s/mdev_type", udev_device_get_syspath(dev)) < 0)
> > +    /* Because of a kernel uevent race, we might get the 'add' event prior to
> > +     * the sysfs tree being ready, so any attempt to access any sysfs attribute
> > +     * would result in ENOENT and us dropping the device, so let's work around
> > +     * it by waiting for the attributes to become available.
> > +     */
> > +
> > +    if (virAsprintf(&linkpath, "%s/mdev_type",
> > +                    udev_device_get_syspath(dev)) < 0)
> >          goto cleanup;
> >
> > +    if (virFileWaitForExists(linkpath, 1, 100) < 0) {
> > +        virReportSystemError(errno,
> > +                             _("failed to wait for file '%s' to appear"),
> > +                             linkpath);
> > +        goto cleanup;
> > +    }
> > +
>
> So the linkpath gets created after the canonicalpath... and we don't

Well, I would assume so, I believe that the kernel wouldn't create symlinks
first and the canonical paths second, such a design would be sooo broken.

Thanks,
Erik

> have to check that right?
>
> Considering what I pointed out in my previous review - I wouldn't be
> able to use virFileWaitForExists, but a similar loop would be possible.
> For NPIV the file exists, it's the contents that are bogus momentarily.
>
> Other consumers waiting that I looked at usually wait on some sort of
> open() and usleep() when ENOENT is returned (there's multiple examples
> if you search on ulseep).  Wonder if any of those could utilize
> something like this...  I know patches welcome ;-)
>
> Reviewed-by: John Ferlan <jferlan@redhat.com>
>
>
> >      if (virFileResolveLink(linkpath, &canonicalpath) < 0) {
> >          virReportSystemError(errno, _("failed to resolve '%s'"), linkpath);
> >          goto cleanup;
> >

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