* Re: [PATCH] bonding: ban stacked bonding support
2015-02-20 23:14 ` Jay Vosburgh
@ 2015-02-21 2:55 ` Andy Gospodarek
2015-02-21 16:59 ` Jiri Pirko
2015-03-20 17:43 ` Alexey Dobriyan
2 siblings, 0 replies; 9+ messages in thread
From: Andy Gospodarek @ 2015-02-21 2:55 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Alexey Dobriyan, davem, vfalico, andy, netdev
On Fri, Feb 20, 2015 at 03:14:00PM -0800, Jay Vosburgh wrote:
> Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
> >Does Linux support it at all?
> >
> >In short: if you add bonding master as a slave, and then release it,
> >it will no longer be a IFF_BONDING creating problems like described at
> >https://bugzilla.kernel.org/show_bug.cgi?id=89541
> >
> > echo +bond1 >/sys/class/net/bonding_masters
> > echo 1 >/sys/class/net/bond1/bonding/mode
> > echo +bond2 >/sys/class/net/bonding_masters
> > echo +bond2 >/sys/class/net/bond1/bonding/slaves
> > echo -bond2 >/sys/class/net/bond1/bonding/slaves
> > echo -bond2 >/sys/class/net/bonding_masters
> >
> > cat /proc/net/bonding/bond2 # should not exist
> > [oops]
> >
> >Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>
> I think it's time to disallow stacking like this; it never
> really worked quite right as far as I can remember, and I thought it was
> disallowed at some point in the past. I don't believe the stacked bonds
> function correctly for receive in the current kernel, either, although
> I'd have to test it again to confirm that.
>
> The usual case for desiring to stack bonds is an active-backup
> pair of LACP / 802.3ad bonds (such as the bugzilla referenced above),
> but the 802.3ad mode handles this situation internally, so no stack is
> necessary.
Completely agree.
>
> >---
> >
> > drivers/net/bonding/bond_main.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> >--- a/drivers/net/bonding/bond_main.c
> >+++ b/drivers/net/bonding/bond_main.c
> >@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
> > slave_dev->name);
> > }
> >
> >+ if (slave_dev->flags & IFF_MASTER) {
> >+ netdev_dbg(bond_dev, "stacked bonding not supported\n");
> >+ return -EBUSY;
> >+ }
> >+
> > /* already enslaved */
> > if (slave_dev->flags & IFF_SLAVE) {
> > netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>
> Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
> could be replaced with something like:
>
> if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
> netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");
>
> With that caveat:
I would say adding the check for netif_is_bond_master() is critical
since a check for only IFF_MASTER would fail when trying to add a bridge
to a bond and that failure would not be desireable.
Fix that and you can also add:
Signed-off-by: Andy Gospodarek <gospo@cumulusnetworks.com>
to your next post.
>
> Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>
> This is probably a good candidate for -stable as well.
Agree
(Though netif_is_bond_master() was added in 3.10; luckily it would be an
easy backport.)
>
> -J
>
> ---
> -Jay Vosburgh, jay.vosburgh@canonical.com
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] bonding: ban stacked bonding support
2015-02-20 23:14 ` Jay Vosburgh
2015-02-21 2:55 ` Andy Gospodarek
@ 2015-02-21 16:59 ` Jiri Pirko
2015-03-20 17:43 ` Alexey Dobriyan
2 siblings, 0 replies; 9+ messages in thread
From: Jiri Pirko @ 2015-02-21 16:59 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Alexey Dobriyan, davem, vfalico, andy, netdev
Sat, Feb 21, 2015 at 12:14:00AM CET, jay.vosburgh@canonical.com wrote:
>Alexey Dobriyan <adobriyan@gmail.com> wrote:
>
>>Does Linux support it at all?
>>
>>In short: if you add bonding master as a slave, and then release it,
>>it will no longer be a IFF_BONDING creating problems like described at
>>https://bugzilla.kernel.org/show_bug.cgi?id=89541
>>
>> echo +bond1 >/sys/class/net/bonding_masters
>> echo 1 >/sys/class/net/bond1/bonding/mode
>> echo +bond2 >/sys/class/net/bonding_masters
>> echo +bond2 >/sys/class/net/bond1/bonding/slaves
>> echo -bond2 >/sys/class/net/bond1/bonding/slaves
>> echo -bond2 >/sys/class/net/bonding_masters
>>
>> cat /proc/net/bonding/bond2 # should not exist
>> [oops]
>>
>>Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>
> I think it's time to disallow stacking like this; it never
>really worked quite right as far as I can remember, and I thought it was
>disallowed at some point in the past. I don't believe the stacked bonds
>function correctly for receive in the current kernel, either, although
>I'd have to test it again to confirm that.
>
> The usual case for desiring to stack bonds is an active-backup
>pair of LACP / 802.3ad bonds (such as the bugzilla referenced above),
>but the 802.3ad mode handles this situation internally, so no stack is
>necessary.
Exactly. There is no real use-case for stacked bonding.
>
>>---
>>
>> drivers/net/bonding/bond_main.c | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>>--- a/drivers/net/bonding/bond_main.c
>>+++ b/drivers/net/bonding/bond_main.c
>>@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
>> slave_dev->name);
>> }
>>
>>+ if (slave_dev->flags & IFF_MASTER) {
>>+ netdev_dbg(bond_dev, "stacked bonding not supported\n");
>>+ return -EBUSY;
>>+ }
>>+
>> /* already enslaved */
>> if (slave_dev->flags & IFF_SLAVE) {
>> netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
>
> Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
>could be replaced with something like:
>
> if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
> netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");
>
> With that caveat:
>
>Signed-off-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>
> This is probably a good candidate for -stable as well.
>
> -J
>
>---
> -Jay Vosburgh, jay.vosburgh@canonical.com
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] bonding: ban stacked bonding support
2015-02-20 23:14 ` Jay Vosburgh
2015-02-21 2:55 ` Andy Gospodarek
2015-02-21 16:59 ` Jiri Pirko
@ 2015-03-20 17:43 ` Alexey Dobriyan
2015-03-20 17:46 ` [PATCH v2] " Alexey Dobriyan
2 siblings, 1 reply; 9+ messages in thread
From: Alexey Dobriyan @ 2015-03-20 17:43 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: davem, vfalico, andy, netdev
On Fri, Feb 20, 2015 at 03:14:00PM -0800, Jay Vosburgh wrote:
> Instead of a separate block for IFF_MASTER, the IFF_SLAVE line
> could be replaced with something like:
>
> if (netif_is_bond_slave(slave_dev) || netif_is_bond_master(slave_dev)) {
> netdev_dbg(bond_dev, "Error: Device is bond slave or master\n");
I think separate error messages are better (kernel can distinguish between
two anyway).
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2] bonding: ban stacked bonding support
2015-03-20 17:43 ` Alexey Dobriyan
@ 2015-03-20 17:46 ` Alexey Dobriyan
2015-03-20 20:38 ` David Miller
0 siblings, 1 reply; 9+ messages in thread
From: Alexey Dobriyan @ 2015-03-20 17:46 UTC (permalink / raw)
To: davem; +Cc: vfalico, andy, netdev, jay.vosburgh
If you add bonding master as a slave, and then release it,
it will no longer be an IFF_BONDING creating problems like described at
https://bugzilla.kernel.org/show_bug.cgi?id=89541
echo +bond1 >/sys/class/net/bonding_masters
echo 1 >/sys/class/net/bond1/bonding/mode
echo +bond2 >/sys/class/net/bonding_masters
echo +bond2 >/sys/class/net/bond1/bonding/slaves
echo -bond2 >/sys/class/net/bond1/bonding/slaves
echo -bond2 >/sys/class/net/bonding_masters
cat /proc/net/bonding/bond2 # should not exist
[oops]
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
drivers/net/bonding/bond_main.c | 5 +++++
1 file changed, 5 insertions(+)
--- a/drivers/net/bonding/bond_main.c
+++ b/drivers/net/bonding/bond_main.c
@@ -1248,6 +1248,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
slave_dev->name);
}
+ if (netif_is_bond_master(slave_dev)) {
+ netdev_err(bond_dev, "device is bond master\n");
+ return -EBUSY;
+ }
+
/* already enslaved */
if (slave_dev->flags & IFF_SLAVE) {
netdev_dbg(bond_dev, "Error: Device was already enslaved\n");
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH v2] bonding: ban stacked bonding support
2015-03-20 17:46 ` [PATCH v2] " Alexey Dobriyan
@ 2015-03-20 20:38 ` David Miller
2015-03-20 21:02 ` Jay Vosburgh
0 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2015-03-20 20:38 UTC (permalink / raw)
To: adobriyan; +Cc: vfalico, andy, netdev, jay.vosburgh
From: Alexey Dobriyan <adobriyan@gmail.com>
Date: Fri, 20 Mar 2015 20:46:38 +0300
> If you add bonding master as a slave, and then release it,
> it will no longer be an IFF_BONDING creating problems like described at
> https://bugzilla.kernel.org/show_bug.cgi?id=89541
>
> echo +bond1 >/sys/class/net/bonding_masters
> echo 1 >/sys/class/net/bond1/bonding/mode
> echo +bond2 >/sys/class/net/bonding_masters
> echo +bond2 >/sys/class/net/bond1/bonding/slaves
> echo -bond2 >/sys/class/net/bond1/bonding/slaves
> echo -bond2 >/sys/class/net/bonding_masters
>
> cat /proc/net/bonding/bond2 # should not exist
> [oops]
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
I feel like this has been brought up before and it was stated that
some people are actually using things like this.
I could be mistaken.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] bonding: ban stacked bonding support
2015-03-20 20:38 ` David Miller
@ 2015-03-20 21:02 ` Jay Vosburgh
2015-03-20 22:30 ` Jay Vosburgh
0 siblings, 1 reply; 9+ messages in thread
From: Jay Vosburgh @ 2015-03-20 21:02 UTC (permalink / raw)
To: David Miller; +Cc: adobriyan, vfalico, andy, netdev
David Miller <davem@davemloft.net> wrote:
>From: Alexey Dobriyan <adobriyan@gmail.com>
>Date: Fri, 20 Mar 2015 20:46:38 +0300
>
>> If you add bonding master as a slave, and then release it,
>> it will no longer be an IFF_BONDING creating problems like described at
>> https://bugzilla.kernel.org/show_bug.cgi?id=89541
>>
>> echo +bond1 >/sys/class/net/bonding_masters
>> echo 1 >/sys/class/net/bond1/bonding/mode
>> echo +bond2 >/sys/class/net/bonding_masters
>> echo +bond2 >/sys/class/net/bond1/bonding/slaves
>> echo -bond2 >/sys/class/net/bond1/bonding/slaves
>> echo -bond2 >/sys/class/net/bonding_masters
>>
>> cat /proc/net/bonding/bond2 # should not exist
>> [oops]
>>
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>
>I feel like this has been brought up before and it was stated that
>some people are actually using things like this.
>
>I could be mistaken.
I don't think you are. I did a bit of checking after the
discussion last month and found a few relatively recent statements that
people were nesting bonds and it was apparently working, e.g.,
http://www.alexwitherspoon.com/debian-nested-bonded-interfaces/
which, ironically, is exactly the case that would benefit from
not nesting the bonds, as 802.3ad would handle multiple aggregators
itself.
However, there is also this discussion
http://lists.openwall.net/netdev/2011/01/22/66
from netdev in 2011 that states that the ingress path of nested
bonds does not work, at least for the case described. Perhaps some
configurations work and some don't.
Let me see if I can run a quick test and see if this actually
works for me...
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] bonding: ban stacked bonding support
2015-03-20 21:02 ` Jay Vosburgh
@ 2015-03-20 22:30 ` Jay Vosburgh
0 siblings, 0 replies; 9+ messages in thread
From: Jay Vosburgh @ 2015-03-20 22:30 UTC (permalink / raw)
To: netdev; +Cc: David Miller, adobriyan, vfalico, andy
Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>David Miller <davem@davemloft.net> wrote:
>
>From: Alexey Dobriyan <adobriyan@gmail.com>
>>Date: Fri, 20 Mar 2015 20:46:38 +0300
>>
>>> If you add bonding master as a slave, and then release it,
>>> it will no longer be an IFF_BONDING creating problems like described at
>>> https://bugzilla.kernel.org/show_bug.cgi?id=89541
>>>
>>> echo +bond1 >/sys/class/net/bonding_masters
>>> echo 1 >/sys/class/net/bond1/bonding/mode
>>> echo +bond2 >/sys/class/net/bonding_masters
>>> echo +bond2 >/sys/class/net/bond1/bonding/slaves
>>> echo -bond2 >/sys/class/net/bond1/bonding/slaves
>>> echo -bond2 >/sys/class/net/bonding_masters
>>>
>>> cat /proc/net/bonding/bond2 # should not exist
>>> [oops]
>>>
>>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>>
>>I feel like this has been brought up before and it was stated that
>>some people are actually using things like this.
>>
>>I could be mistaken.
>
> I don't think you are. I did a bit of checking after the
>discussion last month and found a few relatively recent statements that
>people were nesting bonds and it was apparently working, e.g.,
>
>http://www.alexwitherspoon.com/debian-nested-bonded-interfaces/
>
> which, ironically, is exactly the case that would benefit from
>not nesting the bonds, as 802.3ad would handle multiple aggregators
>itself.
>
> However, there is also this discussion
>
>http://lists.openwall.net/netdev/2011/01/22/66
>
> from netdev in 2011 that states that the ingress path of nested
>bonds does not work, at least for the case described. Perhaps some
>configurations work and some don't.
>
> Let me see if I can run a quick test and see if this actually
>works for me...
I ran a few tests against net-next from a couple of days ago.
A simple test of two balance-rr mode bonds nested below an
active-backup mode bond appears to function, passing traffic in both
directions. I didn't test extensively, but ingress does not appear to
be broken as the 2011 netdev discussion indicates.
The sequence supplied by Alexey does reveal a bug, in that the
bond2 /proc file isn't removed when it should be. In light of the
above, however, this will have to be fixed some way other than
disallowing nesting.
-J
---
-Jay Vosburgh, jay.vosburgh@canonical.com
^ permalink raw reply [flat|nested] 9+ messages in thread