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

[Nikolay Aleksandrov <nikolay@redhat.com>]

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.

Nik