Re: Bonding on bond

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

Nicolas de Pesloüan 	<nicolas.2p.debian@gmail.com> wrote:

>Le 20/01/2011 16:31, Jiri Bohac a écrit :
>> On Wed, Jan 19, 2011 at 09:33:19PM +0100, Nicolas de Pesloüan wrote:
>>> Even if it is possible to test for slave and for master with a
>>> single condition (IFF_BONDING), I suggest to split the tests and the
>>> error messages, to give end user the best possible diagnostic.
>>
>> OK, why not. The below patch still uses IFF_BONDING to detect a
>> master is being enslaved, because IFF_MASTER is also used by the
>> eql driver. No idea if it works / someone ever uses it with
>> bonding, but it might collide.
>
>Thanks Jiri.
>
>> bonding: prohibit enslaving of bonding masters
>>
>> Nested bonding is not supported and will result in strange problems, e.g.:
>> - netif_receive_skb() will not properly change skb->dev to point to the
>>    uppoer-most bonding master
>> - arp monitor will not work (dev->last_rx is only updated by hardware drivers)
>> - accidentally enslaving a bonding master to itself will cause an infinite
>>    recursion in the TX path

	Did you test these?  I'm curious about the ARP monitor
assertion, because last_rx is updated by bonding itself now (in
skb_bond_should_drop), not in the device drivers.

	I'm in agreement that, by and large, nesting of bonds is
pointless.  However, I suspect that there are users out in the world who
are happily doing so, and this patch may shut them down.

	I've not tested with nesting in a while; I know it used to work
(at least for limited cases, typically an active-backup bond with a pair
of balance-xor or balance-rr or sometimes 802.3ad enslaved to it), but
has never really been a deliberate feature.  Is nesting now utterly
broken, as suggested by the list of problems above?

>> This patch prevents this by prohibiting a bonding master from being further enslaved.
>>
>> Signed-off-by: Jiri Bohac<jbohac@suse.cz>
>
>Reviewed-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr>

	If nesting really doesn't work and is going to be disabled, then
at a minimum it should also have an update to the documentation
explaining this.

	-J

>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> index b1025b8..b117dd8 100644
>> --- a/drivers/net/bonding/bond_main.c
>> +++ b/drivers/net/bonding/bond_main.c
>> @@ -1453,6 +1453,12 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>>   		return -EBUSY;
>>   	}
>>
>> +	/* cannot enslave a master */
>> +	if (slave_dev->priv_flags&  IFF_BONDING) {
>> +		pr_debug("Error, cannot enslave a bonding master\n");
>> +		return -EBUSY;
>> +	}
>> +
>>   	/* vlan challenged mutual exclusion */
>>   	/* no need to lock since we're protected by rtnl_lock */
>>   	if (slave_dev->features&  NETIF_F_VLAN_CHALLENGED) {
>>

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