Re: [libvirt] [RFC PATCH] udev: Remove udev handle from main loop when udev thread stops

On 2/13/19 4:34 AM, Marc Hartmayer wrote: > On Tue, Feb 12, 2019 at 09:46 PM +0100, John Ferlan <jferlan(a)redhat.com&gt; wrote: >> On 2/7/19 11:08 AM, Marc Hartmayer wrote: >>> Commit "nodedev: Move device enumumeration out of nodeStateInitialize" >>> (9f0ae0b18e3e620) has moved the heavy task of device enumeration into >>> a separate thread. The problem with this commit is that there is a >>> functionality change in the cleanup when udevEnumerateDevices >>> fails. Before commit 9f0ae0b18e3e620, the entire driver was cleaned up >>> by calling nodeStateCleanup (defined in node_device_udev.c) which >>> resulted in libvirtd stopping with the error message >>> 'daemonRunStateInit:800 : Driver state initialization failed'. With >>> the commit 9f0ae0b18e3e620 there is only a signal to the udev thread >>> that it must stop. This means that for example the watch handle isn't >>> removed from the main loop and this can result in the main loop >>> consuming 100% CPU time as soon as a new udev event occurs. >>>>> This patch proposes a simple solution to the described problem. In >>> case the udev thread stops the watch handle is removed from the main >>> loop. >>>>> Fixes: 9f0ae0b18e3e620 ("nodedev: Move device enumumeration out of nodeStateInitialize") >>> Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>>> --- >>>>> Note: I'm not sure whether we should stop libvirtd (as it would have >>> been done before) or if this patch is already sufficient. >>>>> --- >>> src/node_device/node_device_udev.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>>>> What you have seems reasonable - although I wonder if it would be better >> to remove the event handle in the error of >> nodeStateInitializeEnumerate. > Might be better, yes - I’ve no strong opinion about that. I’ve added the > removal here because it doesn’t make sense to still have the watch > handle registered in the main loop if the udev thread stops - at least I > think so (just to be bulletproof). > The more important point is before your change, the behavior was that > libvirtd stops after this error. Now (with this patch) it only removes > the watch handle and stops the udev thread. Is this change of behavior > okay? This was mentioned during review of the patch - final response here: https://www.redhat.com/archives/libvir-list/2017-December/msg00531.html with the feeling that at least allowing other aspects of libvirt to function even though udev processing is crippled wasn't a bad thing.

Without the patch something would need to be done before starting libvirtd anyway. The "missing piece" was actually having what you had happen occur. Part of me wonders what fails such that enumeration fails and is that something we should be chasing instead. IOW: Is there something in udevEnumerateDevices failure that could just be ignored and we continue to operate logging the failure (like you've done in the patch you recently posted). Does that fix what caused this patch?