Re: [PATCH net-next 3/3] bonding: fix bond_release_all inconsistencies

[Jay Vosburgh <fubar@us.ibm.com>]

Nikolay Aleksandrov <nikolay@redhat.com> wrote:

>On 18/02/13 22:56, Jay Vosburgh wrote:
>> Nikolay Aleksandrov <nikolay@redhat.com> wrote:
>> 
>>> This patch fixes the following inconsistencies in bond_release_all:
>>> - IFF_BONDING flag is not stripped from slaves
>>> - MTU is not restored
>>> - no netdev notifiers are sent
>>> Instead of trying to keep bond_release and bond_release_all in sync
>>> I think we can re-use bond_release as the environment for calling it
>>> is correct (RTNL is held). I have been running tests for the past
>>> week and they came out successful. The only way for bond_release to fail
>>> is for the slave to be attached in a different bond or to not be a slave
>>> but that cannot happen as RTNL is held and no slave manipulations can be
>>> achieved.
>> 
>> 	It might be worthwhile to add an "all" argument to bond_release
>> that skips some things that don't make sense if all slaves are being
>> released.  I'm thinking in particular of this block:
>> 
>> 	if (oldcurrent == slave) {
>> 		/*
>> 		 * Note that we hold RTNL over this sequence, so there
>> 		 * is no concern that another slave add/remove event
>> 		 * will interfere.
>> 		 */
>> 		write_unlock_bh(&bond->lock);
>> 		read_lock(&bond->lock);
>> 		write_lock_bh(&bond->curr_slave_lock);
>> 
>> 		bond_select_active_slave(bond);
>> 
>> 		write_unlock_bh(&bond->curr_slave_lock);
>> 		read_unlock(&bond->lock);
>> 		write_lock_bh(&bond->lock);
>> 	}
>> 
>> 	as it's written now, for the release all case, the code may go
>> to the trouble of assigning a new active slave each time one slave is
>> removed (including various log messages, maybe sending IGMPs, etc).  If
>> all slaves are being removed, that's pointless.  This could be something
>> like:
>> 
>> 	if (release_all) {
>> 		bond->curr_active_slave = NULL;
>> 	} else if (oldcurrent == slave) {
>> 		[ the current block of stuff ]
>> 	}
>> 
>> 	it's safe here to unconditionally set curr_active_slave to NULL
>> because we hold bond->lock for write.  The lock dance stuff for the
>> bond_select_active_slave() call is to satisfy its locking requirements.
>> 
>> 	-J
>I see your point and I agree. I will prepare another version that
>incorporates it, although I can't add it as an argument since
>bond_release is used as ndo_del_slave. I'll have to make it a global
>variable.

	No, just rename the current bond_release to __bond_release_one,
add the extra argument, and create a new bond_release .ndo_del_slave
that calls __bond_release_one with "all=0".  Then, bond_release_all
calls __bond_release_one with all=1.

	Also, there's only one caller of bond_release_all, and since the
new & improved bond_release_all is trivial, it could be open coded into
bond_uninit, eliminating bond_release_all as a function.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com