Re: [PATCH net] failover: eliminate callback hell

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 5 Jun 2018 23:11:37 -0700
"Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:

> On 6/5/2018 11:00 PM, Stephen Hemminger wrote:
> > On Tue, 5 Jun 2018 22:39:12 -0700
> > "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> >  
> >> On 6/5/2018 8:51 PM, Stephen Hemminger wrote:  
> >>> On Tue, 5 Jun 2018 16:52:22 -0700
> >>> "Samudrala, Sridhar" <sridhar.samudrala@intel.com> wrote:
> >>>     
> >>>> On 6/5/2018 2:52 PM, Stephen Hemminger wrote:  
> >>>>> On Tue, 5 Jun 2018 22:38:43 +0300
> >>>>> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>>>>        
> >>>>>>> See:
> >>>>>>>       https://patchwork.ozlabs.org/patch/851711/  
> >>>>>> Let me try to summarize that:
> >>>>>>
> >>>>>> 	You wanted to speed up the delayed link up.  You had an idea to
> >>>>>> 	additionally take link up when userspace renames the interface (standby
> >>>>>> 	one which is also the failover for netvsc).
> >>>>>>
> >>>>>> 	But userspace might not do any renames, in which case there will
> >>>>>> 	still be the delay, and so this never got applied.
> >>>>>>
> >>>>>> 	Is this a good summary?
> >>>>>>
> >>>>>> Davem said delay should go away completely as it's not robust, and I
> >>>>>> think I agree.  So I don't think we should make all failover users use
> >>>>>> delay. IIUC failover kept a delay option especially for netvsc to
> >>>>>> minimize the surprise factor. Hopefully we can come up with
> >>>>>> something more robust and drop that option completely.  
> >>>>> The timeout was the original solution to how to complete setup after
> >>>>> userspace has had a chance to rename the device. Unfortunately, the whole network
> >>>>> device initialization (cooperation with udev and userspace) is a a mess because
> >>>>> there is no well defined specification, and there are multiple ways userspace
> >>>>> does this in old and new distributions.  The timeout has its own issues
> >>>>> (how long, handling errors during that window, what if userspace modifies other
> >>>>> device state); and open to finding a better solution.
> >>>>>
> >>>>> My point was that if name change can not be relied on (or used) by netvsc,
> >>>>> then we can't allow it for net_failover either.  
> >>>> I think the push back was with the usage of the delay, not bringing up the primary/standby
> >>>> device in the name change event handler.
> >>>> Can't netvsc use this mechanism instead of depending on the delay?
> >>>>
> >>>>     
> >>> The patch that was rejected for netvsc was about using name change.
> >>> Also, you can't depend on name change; you still need a timer. Not all distributions
> >>> change name of devices. Or user has blocked that by udev rules.  
> >> In the net_failover_slave_register() we do a dev_open() and ignore any failure due to
> >> EBUSY and do another dev_open() in the name change event handler.
> >> If the name is not expected to change, i would think the dev_open() at the time of
> >> register will succeed.  
> > The problem is your first dev_open will bring device up and lockout
> > udev from changing the network device name.  
> 
> I have tried with/without udev and didn't see any issue with the naming of the primary/standby
> devices in my testing.
> 
> With the 3-netdev failover model, we are only interested in setting the right name for the failover
> netdev and that is the reason we do SET_NETDEV_DEV on that netdev. Does it really matter if udev fails
> to rename the lower primary/standby netdevs, i don't think it will matter? The user is not expected
> to touch the lower netdevs.

Renaming matters to some users and the udev maintainers. They want the VF to be named enpXXX
The primary in virtio case needs to be ens3 with some cloud platforms.

I think you need to get rid of triggering off of the name change.

Long term, let's open the discussion about allowing network devices to change name when up?
Maybe with NETIF_LIVENAME_CHANGE flag?