Re: [3/4] bonding: the calling of bond->slave_cnt need protection

[Ding Tianhong <dingtianhong@huawei.com>]

On 2013/7/20 23:00, Veaceslav Falico wrote:
> On Sat, Jul 20, 2013 at 02:42:37PM +0200, Nikolay Aleksandrov wrote:
>> On 07/20/2013 12:47 PM, Veaceslav Falico wrote:
>>> On Sat, Jul 20, 2013 at 03:23:57PM +0800, dingtianhong wrote:
>>>> The bonding_store_mode has rtnl protection, so no need to get read lock
>>>> for bond->slave_cnt, but the bonding_store_fail_over_mac need to protect
>>>> the bond->slave_cnt, so add read_lock().
>>>>
>>>> Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
>>>>
>> <snip>
>>>
>>> Maybe it's Saturday, but I really don't see *any* point in this locking.
>>>
>>> I think you've meant that we need the rtnl protection while reading
>>> slave_cnt AND updating the .fail_over_mac, so that in between we won't add
>>> new slaves with outdated params.
>>>
>>> Something like this (untested):
>>>
>> Indeed, Veaceslav's way is the correct one (I've looked at this race
>> before), but IMO it's not worth it to protect fail_over_mac as the worst
>> that could happen is inconsistency with the MAC addresses which isn't
>> fatal. Anyway, I still haven't had my coffee and might be missing something :-)
> 
> Yep, agree that it's kind of minor and hard to hit in real life.
> 
> OTOH, getting the rtnl here costs us virtually nothing and might save
> someone from a headache :). And it also follows the logic "don't change
> anything slave-related without rtnl".
> 
> So I'd rather have it, as a minor improvement :).
> 
>>
>> Cheers,
>> Nik

yes , i think it is hard to hit the problem in real lift, just looks better. :)

>>
> 
>