Tue, Mar 01, 2011 at 07:06:02PM CET, fubar@xxxxxxxxxx wrote:
>Joe Eykholt <joe.eykholt@xxxxxxxxx> wrote:
>
>>On 2/28/11 10:37 PM, Jiri Pirko wrote:
>>> Mon, Feb 28, 2011 at 06:54:29PM CET, joe.eykholt@xxxxxxxxx wrote:
>>>> On 2/28/11 9:15 AM, Jay Vosburgh wrote:
>>>>> Jiri Pirko<jpirko@xxxxxxxxxx> wrote:
>>>>>
>>>>>> Check for IFF_BONDING as this flag is set-up for all bonding devices.
>>>>>>
>>>>>> Signed-off-by: Jiri Pirko<jpirko@xxxxxxxxxx>
>>>>>> ---
>>>>>> 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@xxxxxxxxxx
>>>>
>>>> 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?
>
> One or two other things besides bonding use IFF_MASTER, but
>IFF_BONDING is only for 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))
>
> This doesn't work because the flags are kept in different
>places, IFF_MASTER is in flags and IFF_BONDING in priv_flags.
Exactly - I'm going to send corrected patch very soon.
Jirka
>
> -J
>
>>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.
>
> This is also true.
>
> -J
>
>---
> -Jay Vosburgh, IBM Linux Technology Center, fubar@xxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
