Re: [PATCH net] failover: eliminate callback hell

["Samudrala, Sridhar" <sridhar.samudrala@intel.com>]

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.