On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote: > Since we have a number of places where we workaround timing issues > with > devices, attributes (files in general) not being available at the > time > of processing them by calling usleep in a loop for a fixed number of > tries, we could as well have a utility function that would do that. > Therefore we won't have to duplicate this ugly workaround even more. > > This is a prerequisite for > https://bugzilla.redhat.com/show_bug.cgi?id=1463285. > > Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++ > src/util/virfile.h | 2 ++ > 3 files changed, 39 insertions(+) > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > index c1e9471c5..53878a30f 100644 > --- a/src/libvirt_private.syms > +++ b/src/libvirt_private.syms > @@ -1698,6 +1698,7 @@ virFileStripSuffix; > virFileTouch; > virFileUnlock; > virFileUpdatePerm; > +virFileWaitForAccess; > virFileWrapperFdClose; > virFileWrapperFdFree; > virFileWrapperFdNew; > diff --git a/src/util/virfile.c b/src/util/virfile.c > index 6bbcc3d15..0b1a91699 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const > char *format, ...) > VIR_FREE(str); > return ret; > } > + > + > +/** > + * virFileWaitForAccess: > + * @path: absolute path to a sysfs attribute (can be a symlink) > + * @ms: how long to wait (in milliseconds) > + * @tries: how many times should we try to wait for @path to become > accessible > + * > + * Checks the existence of @path. In case the file defined by @path > + * doesn't exist, we wait for it to appear in 100ms (for up to > @tries times). > + * > + * Returns 0 on success, -1 on error (ENOENT is fine here). > + */ > +int > +virFileWaitForAccess(const char *path, size_t ms, size_t tries) > +{ > + errno = 0; > + > + /* wait for @path to be accessible in @ms milliseconds, up to > @tries */ > + while (tries-- > 0 && !virFileExists(path)) { > + if (errno != ENOENT) { > + virReportSystemError(errno, "%s", path); > + return -1; > + } else if (tries == 10) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("Failed to access '%s' after %zu > tries"), > + path, tries); > + return -1; > + } else { > + VIR_DEBUG("Failed to access '%s', re-try in %zu ms", > path, ms); > + usleep(ms * 1000); > + } > + } > + > + return 0; > +} Just FYI, there's another way to address it by calling udevadm settle before and after "touching" a block device, libguestfs is using this approach and it works very well: https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=udev_s ettle&type=

On Tue, 2017-06-20 at 17:45 +0200, Michal Privoznik wrote: > On 06/20/2017 05:21 PM, Dawid Zamirski wrote: > > On Tue, 2017-06-20 at 17:03 +0200, Erik Skultety wrote: > > > Since we have a number of places where we workaround timing > > > issues > > > with > > > devices, attributes (files in general) not being available at the > > > time > > > of processing them by calling usleep in a loop for a fixed number > > > of > > > tries, we could as well have a utility function that would do > > > that. > > > Therefore we won't have to duplicate this ugly workaround even > > > more. > > > > > > This is a prerequisite for > > > https://bugzilla.redhat.com/show_bug.cgi?id=1463285. > > > > > > Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; > > > --- > > > src/libvirt_private.syms | 1 + > > > src/util/virfile.c | 36 > > > ++++++++++++++++++++++++++++++++++++ > > > src/util/virfile.h | 2 ++ > > > 3 files changed, 39 insertions(+) > > > > > > diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms > > > index c1e9471c5..53878a30f 100644 > > > --- a/src/libvirt_private.syms > > > +++ b/src/libvirt_private.syms > > > @@ -1698,6 +1698,7 @@ virFileStripSuffix; > > > virFileTouch; > > > virFileUnlock; > > > virFileUpdatePerm; > > > +virFileWaitForAccess; > > > virFileWrapperFdClose; > > > virFileWrapperFdFree; > > > virFileWrapperFdNew; > > > diff --git a/src/util/virfile.c b/src/util/virfile.c > > > index 6bbcc3d15..0b1a91699 100644 > > > --- a/src/util/virfile.c > > > +++ b/src/util/virfile.c > > > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const > > > char *format, ...) > > > VIR_FREE(str); > > > return ret; > > > } > > > + > > > + > > > +/** > > > + * virFileWaitForAccess: > > > + * @path: absolute path to a sysfs attribute (can be a symlink) > > > + * @ms: how long to wait (in milliseconds) > > > + * @tries: how many times should we try to wait for @path to > > > become > > > accessible > > > + * > > > + * Checks the existence of @path. In case the file defined by > > > @path > > > + * doesn't exist, we wait for it to appear in 100ms (for up to > > > @tries times). > > > + * > > > + * Returns 0 on success, -1 on error (ENOENT is fine here). > > > + */ > > > +int > > > +virFileWaitForAccess(const char *path, size_t ms, size_t tries) > > > +{ > > > + errno = 0; > > > + > > > + /* wait for @path to be accessible in @ms milliseconds, up > > > to > > > @tries */ > > > + while (tries-- > 0 && !virFileExists(path)) { > > > + if (errno != ENOENT) { > > > + virReportSystemError(errno, "%s", path); > > > + return -1; > > > + } else if (tries == 10) { > > > + virReportError(VIR_ERR_INTERNAL_ERROR, > > > + _("Failed to access '%s' after %zu > > > tries"), > > > + path, tries); > > > + return -1; > > > + } else { > > > + VIR_DEBUG("Failed to access '%s', re-try in %zu ms", > > > path, ms); > > > + usleep(ms * 1000); > > > + } > > > + } > > > + > > > + return 0; > > > +} > > > > Just FYI, there's another way to address it by calling udevadm > > settle > > before and after "touching" a block device, libguestfs is using > > this > > approach and it works very well: > > > > https://github.com/libguestfs/libguestfs/search?utf8=%E2%9C%93&q=ud > > ev_s > > ettle&type= > > Does it? udevadm settle waits for all the events to be processed, not > just the one that we want. The wait time would be unpredictable IMO. > > Michal Well it's a kind of brute-force approach but at least guarantees the device will be accessible once it exits. Why would setting a custom arbitrary timeout be better which may be too short? Is the predictable wait time here more important than being able to open the device at all?

On Tue, Jun 20, 2017 at 17:03:31 +0200, Erik Skultety wrote: > Since we have a number of places where we workaround timing issues with > devices, attributes (files in general) not being available at the time > of processing them by calling usleep in a loop for a fixed number of > tries, we could as well have a utility function that would do that. > Therefore we won't have to duplicate this ugly workaround even more. > > This is a prerequisite for > https://bugzilla.redhat.com/show_bug.cgi?id=1463285. This statement is useless. The helper function can be reused elsewhere. > > Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; > --- > src/libvirt_private.syms | 1 + > src/util/virfile.c | 36 ++++++++++++++++++++++++++++++++++++ > src/util/virfile.h | 2 ++ > 3 files changed, 39 insertions(+) [...] > diff --git a/src/util/virfile.c b/src/util/virfile.c > index 6bbcc3d15..0b1a91699 100644 > --- a/src/util/virfile.c > +++ b/src/util/virfile.c > @@ -4164,3 +4164,39 @@ virFileReadValueString(char **value, const char *format, ...) > VIR_FREE(str); > return ret; > } > + > + > +/** > + * virFileWaitForAccess: > + * @path: absolute path to a sysfs attribute (can be a symlink) > + * @ms: how long to wait (in milliseconds) > + * @tries: how many times should we try to wait for @path to become accessible > + * > + * Checks the existence of @path. In case the file defined by @path > + * doesn't exist, we wait for it to appear in 100ms (for up to @tries times). > + * > + * Returns 0 on success, -1 on error (ENOENT is fine here). This description does not make sense. You don't state that this reports errors. Also the mention of ENOENT does not make sense. This function in fact sometimes returns enoent if the file does not appear until timeout. > + */ > +int > +virFileWaitForAccess(const char *path, size_t ms, size_t tries) > +{ > + errno = 0; > + > + /* wait for @path to be accessible in @ms milliseconds, up to @tries */ > + while (tries-- > 0 && !virFileExists(path)) { > + if (errno != ENOENT) { > + virReportSystemError(errno, "%s", path); This does not really explain stuff to users. You might want to add a more comprehensive error message or leave error reporting to users. > + return -1; > + } else if (tries == 10) { This does not make any sense. The while loop counts down and checks if tries is more than 10. And this checks that tries is equal to 10. So if somebody passes 11 as @tries he/she gets this error? If tries is set to < 10 you get success even on timeout? Did you modify the code without testing it?

If we find ourselves in the situation that the 'add' uevent has been fired earlier than the sysfs tree for a device, we'd better wait for the attributes to be ready rather than discarding the device from our device list forever.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1463285 Signed-off-by: Erik Skultety <eskultet(a)redhat.com&gt; --- src/node_device/node_device_udev.c | 48 +++++++++++++++++++++++++++++++++++++- 1 file changed, 47 insertions(+), 1 deletion(-) diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index 174124a1b..4f9ca0c67 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -60,6 +60,43 @@ struct _udevPrivate { }; +/** + * udevWaitForAttrs: + * @sys_path: node device base path + * @...: attributes to wait for, last attribute must be NULL + * + * Takes a list of attributes to wait for, waits until all of them are + * available, unless the max number of tries (10) has been reached. + * + * Returns 0 if all attributes became available, -1 on error.

+ */ +static int +udevWaitForAttrs(const char *sys_path, ...) +{ + int ret = -1; + const char *attr = NULL; + char *attr_path = NULL; + va_list args; + + va_start(args, sys_path); + while ((attr = va_arg(args, char *))) { + if (virAsprintf(&attr_path, "%s/%s", sys_path, attr) < 0) + goto cleanup; + + if (virFileWaitForAccess(attr_path, 100, 10) < 0)

+ goto cleanup; + + VIR_FREE(attr_path); + } + + ret = 0; + cleanup: + va_end(args); + VIR_FREE(attr_path); + return ret; +} + + static bool udevHasDeviceProperty(struct udev_device *dev, const char *key) @@ -1113,12 +1150,21 @@ udevProcessMediatedDevice(struct udev_device *dev, { int ret = -1; const char *uuidstr = NULL; + const char *devpath = udev_device_get_syspath(dev); int iommugrp = -1; char *linkpath = NULL; 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 (udevWaitForAttrs(devpath, "mdev_type", "iommu_group", NULL) < 0) + goto cleanup;

+ + if (virAsprintf(&linkpath, "%s/mdev_type", devpath) < 0) goto cleanup;