Re: [Open-FCoE] [PATCH] fcoe: correct checking for bonding

[Joe Eykholt <joe.eykholt@gmail.com>]

On 2/28/11 10:37 PM, Jiri Pirko wrote:
> Mon, Feb 28, 2011 at 06:54:29PM CET, joe.eykholt@gmail.com wrote:
>> On 2/28/11 9:15 AM, Jay Vosburgh wrote:
>>> Jiri Pirko<jpirko@redhat.com>   wrote:
>>>
>>>> Check for IFF_BONDING as this flag is set-up for all bonding devices.
>>>>
>>>> Signed-off-by: Jiri Pirko<jpirko@redhat.com>
>>>> ---
>>>> drivers/scsi/fcoe/fcoe.c |    4 +---
>>>> 1 files changed, 1 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
>>>> index 9f9600b..67714a4 100644
>>>> --- a/drivers/scsi/fcoe/fcoe.c
>>>> +++ b/drivers/scsi/fcoe/fcoe.c
>>>> @@ -285,9 +285,7 @@ static int fcoe_interface_setup(struct fcoe_interface *fcoe,
>>>> 	}
>>>>
>>>> 	/* Do not support for bonding device */
>>>> -	if ((netdev->priv_flags&   IFF_MASTER_ALB) ||
>>>> -	    (netdev->priv_flags&   IFF_SLAVE_INACTIVE) ||
>>>> -	    (netdev->priv_flags&   IFF_MASTER_8023AD)) {
>>>> +	if (netdev->priv_flags&   IFF_BONDING) {
>>>> 		FCOE_NETDEV_DBG(netdev, "Bonded interfaces not supported\n");
>>>> 		return -EOPNOTSUPP;
>>>> 	}
>>>
>>> 	Based on past discussions, I believe the intent of the code is
>>> to permit FCOE over bonding only for active-backup mode, and possibly
>>> for -xor/-rr as well.
>>>
>>> 	I'm not sure if the slave or the master is what's being tested
>>> here, so I'm not sure what the right thing to do is.  I suspect it's the
>>> master, as I recall discussion of one configuration involving
>>> active-backup mode balancing FCOE traffic over both the active and
>>> inactive slaves.  FCOE uses the "orig_dev" logic in __netif_receive_skb
>>> to have the packets delivered even on the nominally inactive slave.
>>>
>>> 	-J
>>>
>>> ---
>>> 	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
>>
>> Right.  That was the intent.  It should work on the physical dev, but probably
>> not on the master of the bond.
>>
>> If you have a master/slave bond for IPv4 between eth1 and eth2, say,
>> and they are going to two different DCE (FCoE) switches, presumably on
>> different VSANs but with ultimate access to the same disks,
>> then you want to split the FCoE traffic in active/active
>> mode using separate FCoE instances on eth1 and eth2 even though IP
>> is using active/standby on bond0.  This should work.  But, putting fcoe
>> on bond0 isn't going to do what you want.
>>
>> However, it seems like the check above shouldn't be checking
>> IFF_SLAVE_INACTIVE.   I can't test this.
>
> OK. So I guess the right check should be for:
> (netdev->priv_flags&  IFF_BONDING&&  netdev->flags&  IFF_MASTER)

I think that's OK.  How about just checking for MASTER?
When is MASTER going to be set without BONDING?

Otherwise I'd add some parens or I might code this as:

	if ((netdev->priv_flags & (IFF_BONDING | IFF_MASTER)) ==
	    (IFF_BONDING | IFF_MASTER))

Which is less clear, I know, but used to generate better code.
The compiler might generate the same code these days.
Not that this is performance-critical or anything.

> This would disable adding all bond devices (like bond0 etc) and allows
> to use enslaved physdevs.
>
> Note that checking for mode is irrelevant here. Mode could be easily
> changed later without fcoe knowing that.
>
> Jirka

	Cheers,
	Joe