1. Patchew
  2. Libvirt
  3. View series

[PATCH] node_device: Use "udev" monitor source

Michal Privoznik posted 1 patch 3 years, 5 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/cdc6d78a59fbbd41b41de216fd14aeeb9561695a.1605616187.git.mprivozn@redhat.com
src/node_device/node_device_udev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
[PATCH] node_device: Use "udev" monitor source
Posted by Michal Privoznik 3 years, 5 months ago 
In v6.3.0-rc1~67 I've made a switch: instead of listening on udev
events the nodedev driver started listening for kernel events.
This was because when a device changes its name (e.g. NICs) we
will get "move" event with DEVPATH_OLD property set, which we can
then use to remove the old device and thus keep our internal list
up to date. The switch to "kernel" source was made because if the
old NICs naming (eth0, eth1, ...) is enabled (e.g. via
net.ifnames=0 on the kernel cmd line) then udev overwrites the
property with the new name making our internal list go out of
sync. Interestingly, when the od NICs naming is not enabled then
the DEVPATH_OLD contains the correct value.

But as it turns out, "kernel" source might be missing some other
important properties, e.g. USB vendor/product IDs. Therefore,
switch back to "udev" source and wish the best of luck to users
using the old NICs naming.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1897625
Fixes: 9a13704818e4a018723e0ec5b9e97b176f1c8584
Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 src/node_device/node_device_udev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 65f312d8f4..dec19a3166 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1983,7 +1983,7 @@ nodeStateInitialize(bool privileged,
 
     virObjectLock(priv);
 
-    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "kernel");
+    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
     if (!priv->udev_monitor) {
         virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
                        _("udev_monitor_new_from_netlink returned NULL"));
-- 
2.26.2
Re: [PATCH] node_device: Use "udev" monitor source
Posted by Daniel P. Berrangé 3 years, 5 months ago 
On Tue, Nov 17, 2020 at 01:29:47PM +0100, Michal Privoznik wrote:
> In v6.3.0-rc1~67 I've made a switch: instead of listening on udev
> events the nodedev driver started listening for kernel events.
> This was because when a device changes its name (e.g. NICs) we
> will get "move" event with DEVPATH_OLD property set, which we can
> then use to remove the old device and thus keep our internal list
> up to date. The switch to "kernel" source was made because if the
> old NICs naming (eth0, eth1, ...) is enabled (e.g. via
> net.ifnames=0 on the kernel cmd line) then udev overwrites the
> property with the new name making our internal list go out of
> sync. Interestingly, when the od NICs naming is not enabled then
> the DEVPATH_OLD contains the correct value.

I don't see any difference in properties regardless of net.ifnames
is 0 or 1. In both cases, there is DEVPATH + DEVPAT_OLD, and
INTERFACE has the new NIC name.  Can you actually reproduce the
original problem with net.ifnames=0 ?  If so, which distro ?

> 
> But as it turns out, "kernel" source might be missing some other
> important properties, e.g. USB vendor/product IDs. Therefore,
> switch back to "udev" source and wish the best of luck to users
> using the old NICs naming.
> 
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1897625
> Fixes: 9a13704818e4a018723e0ec5b9e97b176f1c8584
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  src/node_device/node_device_udev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 65f312d8f4..dec19a3166 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1983,7 +1983,7 @@ nodeStateInitialize(bool privileged,
>  
>      virObjectLock(priv);
>  
> -    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "kernel");
> +    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
>      if (!priv->udev_monitor) {
>          virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                         _("udev_monitor_new_from_netlink returned NULL"));

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH] node_device: Use "udev" monitor source
Posted by Michal Privoznik 3 years, 5 months ago 
On 11/17/20 1:51 PM, Daniel P. Berrangé wrote:
> On Tue, Nov 17, 2020 at 01:29:47PM +0100, Michal Privoznik wrote:
>> In v6.3.0-rc1~67 I've made a switch: instead of listening on udev
>> events the nodedev driver started listening for kernel events.
>> This was because when a device changes its name (e.g. NICs) we
>> will get "move" event with DEVPATH_OLD property set, which we can
>> then use to remove the old device and thus keep our internal list
>> up to date. The switch to "kernel" source was made because if the
>> old NICs naming (eth0, eth1, ...) is enabled (e.g. via
>> net.ifnames=0 on the kernel cmd line) then udev overwrites the
>> property with the new name making our internal list go out of
>> sync. Interestingly, when the od NICs naming is not enabled then
>> the DEVPATH_OLD contains the correct value.
> 
> I don't see any difference in properties regardless of net.ifnames
> is 0 or 1. In both cases, there is DEVPATH + DEVPAT_OLD, and
> INTERFACE has the new NIC name.  Can you actually reproduce the
> original problem with net.ifnames=0 ?  If so, which distro ?

Have you tried renaming back and forth? for instance:

# ip link set tunl0 name tunl1
# ip link set tunl1 name tunl0


The "kernel" source yields:

KERNEL[778387.296879] move     /devices/virtual/net/tunl1 (net)
ACTION=move
DEVPATH=/devices/virtual/net/tunl1
DEVPATH_OLD=/devices/virtual/net/tunl0
IFINDEX=5
INTERFACE=tunl1
SEQNUM=20701
SUBSYSTEM=net

KERNEL[778400.944769] move     /devices/virtual/net/tunl0 (net)
ACTION=move
DEVPATH=/devices/virtual/net/tunl0
DEVPATH_OLD=/devices/virtual/net/tunl1
IFINDEX=5
INTERFACE=tunl0
SEQNUM=20702
SUBSYSTEM=net


But "udev" gets:

UDEV  [778387.297247] move     /devices/virtual/net/tunl1 (net)
ACTION=move
DEVPATH=/devices/virtual/net/tunl1
DEVPATH_OLD=/devices/virtual/net/tunl0
IFINDEX=5
INTERFACE=tunl1
SEQNUM=20701
SUBSYSTEM=net
USEC_INITIALIZED=5901449841
net.ifnames=0

UDEV  [778400.945195] move     /devices/virtual/net/tunl1 (net)
ACTION=move
DEVPATH=/devices/virtual/net/tunl1
DEVPATH_OLD=/devices/virtual/net/tunl1
IFINDEX=5
INTERFACE=tunl1
SEQNUM=20702
SUBSYSTEM=net
USEC_INITIALIZED=5901449841
net.ifnames=0


In both cases the first rename (tunl0 -> tunl1) gets correct 
DEVPATH_OLD, but the second rename is problematic (tunl1 -> tunl0) and 
only "kernel" gets it right.

This is on 5.9.1-gentoo-x86_64 and

# udevadm --version
243


> 
>>
>> But as it turns out, "kernel" source might be missing some other
>> important properties, e.g. USB vendor/product IDs. Therefore,
>> switch back to "udev" source and wish the best of luck to users
>> using the old NICs naming.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1897625
>> Fixes: 9a13704818e4a018723e0ec5b9e97b176f1c8584
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   src/node_device/node_device_udev.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
>> index 65f312d8f4..dec19a3166 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1983,7 +1983,7 @@ nodeStateInitialize(bool privileged,
>>   
>>       virObjectLock(priv);
>>   
>> -    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "kernel");
>> +    priv->udev_monitor = udev_monitor_new_from_netlink(udev, "udev");
>>       if (!priv->udev_monitor) {
>>           virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>>                          _("udev_monitor_new_from_netlink returned NULL"));
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 

Pushed. Thanks.

Michal
Re: [PATCH] node_device: Use "udev" monitor source
Posted by Daniel P. Berrangé 3 years, 5 months ago 
On Tue, Nov 17, 2020 at 02:29:27PM +0100, Michal Privoznik wrote:
> On 11/17/20 1:51 PM, Daniel P. Berrangé wrote:
> > On Tue, Nov 17, 2020 at 01:29:47PM +0100, Michal Privoznik wrote:
> > > In v6.3.0-rc1~67 I've made a switch: instead of listening on udev
> > > events the nodedev driver started listening for kernel events.
> > > This was because when a device changes its name (e.g. NICs) we
> > > will get "move" event with DEVPATH_OLD property set, which we can
> > > then use to remove the old device and thus keep our internal list
> > > up to date. The switch to "kernel" source was made because if the
> > > old NICs naming (eth0, eth1, ...) is enabled (e.g. via
> > > net.ifnames=0 on the kernel cmd line) then udev overwrites the
> > > property with the new name making our internal list go out of
> > > sync. Interestingly, when the od NICs naming is not enabled then
> > > the DEVPATH_OLD contains the correct value.
> > 
> > I don't see any difference in properties regardless of net.ifnames
> > is 0 or 1. In both cases, there is DEVPATH + DEVPAT_OLD, and
> > INTERFACE has the new NIC name.  Can you actually reproduce the
> > original problem with net.ifnames=0 ?  If so, which distro ?
> 
> Have you tried renaming back and forth? for instance:
> 
> # ip link set tunl0 name tunl1
> # ip link set tunl1 name tunl0
> 
> 
> The "kernel" source yields:
> 
> KERNEL[778387.296879] move     /devices/virtual/net/tunl1 (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/tunl1
> DEVPATH_OLD=/devices/virtual/net/tunl0
> IFINDEX=5
> INTERFACE=tunl1
> SEQNUM=20701
> SUBSYSTEM=net
> 
> KERNEL[778400.944769] move     /devices/virtual/net/tunl0 (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/tunl0
> DEVPATH_OLD=/devices/virtual/net/tunl1
> IFINDEX=5
> INTERFACE=tunl0
> SEQNUM=20702
> SUBSYSTEM=net
> 
> 
> But "udev" gets:
> 
> UDEV  [778387.297247] move     /devices/virtual/net/tunl1 (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/tunl1
> DEVPATH_OLD=/devices/virtual/net/tunl0
> IFINDEX=5
> INTERFACE=tunl1
> SEQNUM=20701
> SUBSYSTEM=net
> USEC_INITIALIZED=5901449841
> net.ifnames=0
> 
> UDEV  [778400.945195] move     /devices/virtual/net/tunl1 (net)
> ACTION=move
> DEVPATH=/devices/virtual/net/tunl1
> DEVPATH_OLD=/devices/virtual/net/tunl1
> IFINDEX=5
> INTERFACE=tunl1
> SEQNUM=20702
> SUBSYSTEM=net
> USEC_INITIALIZED=5901449841
> net.ifnames=0
>
> 
> In both cases the first rename (tunl0 -> tunl1) gets correct DEVPATH_OLD,
> but the second rename is problematic (tunl1 -> tunl0) and only "kernel" gets
> it right.

Hmm, this simply smells like a bug we should report to udev. It makes
no sense that the event is correct for the first rename, and then wrong
for the subsequent rename.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.