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 e3a647e3d..96a87f4ab 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 (virFileWaitForAccess(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.3

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

On 08/24/2017 07:23 AM, 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(-)
> 

IIRC - I pointed out to you that this is eerily familiar to something
that happens in the vHBA code w/r/t to wwnn/wwpn files. Except that the
files exist, but have a -1 in them which is totally bogus. Then some
magic thing happens and the real wwnn/wwpn is placed into the file, but
libvirt already looked and failed.  When I tried to work around this the
decision was to let it be and call it a kernel / udev bug.

https://www.redhat.com/archives/libvir-list/2016-June/msg02213.html

and Daniel's answer

https://www.redhat.com/archives/libvir-list/2016-July/msg00912.html

Although yes, with the other changes in place one would think having a
wait is no big deal.

Still are you guaranteed that once the file exists that the data within
the file is valid?  In the vHBA case it wasn't and that led to issues.

I'd "use this" processing instead of the hack I proposed as well seeing
as it doesn't seem kernel/udev fixing issues such as these is on any
priority list /-{

John

> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index e3a647e3d..96a87f4ab 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 (virFileWaitForAccess(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;
> 

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

On Mon, Aug 28, 2017 at 12:40:44PM -0400, John Ferlan wrote:
>
>
> On 08/24/2017 07:23 AM, 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(-)
> >
>
> IIRC - I pointed out to you that this is eerily familiar to something
> that happens in the vHBA code w/r/t to wwnn/wwpn files. Except that the
> files exist, but have a -1 in them which is totally bogus. Then some
> magic thing happens and the real wwnn/wwpn is placed into the file, but
> libvirt already looked and failed.  When I tried to work around this the
> decision was to let it be and call it a kernel / udev bug.
>
> https://www.redhat.com/archives/libvir-list/2016-June/msg02213.html
>
> and Daniel's answer
>
> https://www.redhat.com/archives/libvir-list/2016-July/msg00912.html
>
> Although yes, with the other changes in place one would think having a
> wait is no big deal.
>
> Still are you guaranteed that once the file exists that the data within
> the file is valid?  In the vHBA case it wasn't and that led to issues.

Yes, I recall you pointing me to this issue before and you're right that if the
data is bogus, we can't do much about that, except that in this case, I'm only
relying on the existence of the file/dir, because I need its name to determine
the mediated device's type, not its content, which arguably makes it a different
problem.

Erik

>
> I'd "use this" processing instead of the hack I proposed as well seeing
> as it doesn't seem kernel/udev fixing issues such as these is on any
> priority list /-{

Exactly ^this :(

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