* [PATCH net-next 0/2] bonding: fix bond recovery in mode 2 @ 2022-11-18 20:30 Jonathan Toppins 2022-11-18 20:30 ` [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping Jonathan Toppins 2022-11-18 20:30 ` [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero Jonathan Toppins 0 siblings, 2 replies; 11+ messages in thread From: Jonathan Toppins @ 2022-11-18 20:30 UTC (permalink / raw) To: netdev, Jay Vosburgh When a bond is configured with a non-zero updelay and in mode 2 the bond never recovers after all slaves lose link. The first patch adds selftests that demonstrate the issue and the second patch fixes the issue by ignoring the updelay when there are no usable slaves. Jonathan Toppins (2): selftests: bonding: up/down delay w/ slave link flapping bonding: fix link recovery in mode 2 when updelay is nonzero drivers/net/bonding/bond_main.c | 11 +- .../selftests/drivers/net/bonding/Makefile | 4 +- .../selftests/drivers/net/bonding/lag_lib.sh | 107 ++++++++++++++++++ .../net/bonding/mode-1-recovery-updelay.sh | 45 ++++++++ .../net/bonding/mode-2-recovery-updelay.sh | 45 ++++++++ .../selftests/drivers/net/bonding/settings | 2 +- 6 files changed, 211 insertions(+), 3 deletions(-) create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh -- 2.31.1 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping 2022-11-18 20:30 [PATCH net-next 0/2] bonding: fix bond recovery in mode 2 Jonathan Toppins @ 2022-11-18 20:30 ` Jonathan Toppins 2022-11-22 10:53 ` Paolo Abeni 2022-11-18 20:30 ` [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero Jonathan Toppins 1 sibling, 1 reply; 11+ messages in thread From: Jonathan Toppins @ 2022-11-18 20:30 UTC (permalink / raw) To: netdev, Jay Vosburgh Cc: Liang Li, Veaceslav Falico, Andy Gospodarek, Shuah Khan, linux-kernel, linux-kselftest Verify when a bond is configured with {up,down}delay and the link state of slave members flaps if there are no remaining members up the bond should immediately select a member to bring up. (from bonding.txt section 13.1 paragraph 4) Suggested-by: Liang Li <liali@redhat•com> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> --- .../selftests/drivers/net/bonding/Makefile | 4 +- .../selftests/drivers/net/bonding/lag_lib.sh | 107 ++++++++++++++++++ .../net/bonding/mode-1-recovery-updelay.sh | 45 ++++++++ .../net/bonding/mode-2-recovery-updelay.sh | 45 ++++++++ .../selftests/drivers/net/bonding/settings | 2 +- 5 files changed, 201 insertions(+), 2 deletions(-) create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile index 6b8d2e2f23c2..0f3921908b07 100644 --- a/tools/testing/selftests/drivers/net/bonding/Makefile +++ b/tools/testing/selftests/drivers/net/bonding/Makefile @@ -5,7 +5,9 @@ TEST_PROGS := \ bond-arp-interval-causes-panic.sh \ bond-break-lacpdu-tx.sh \ bond-lladdr-target.sh \ - dev_addr_lists.sh + dev_addr_lists.sh \ + mode-1-recovery-updelay.sh \ + mode-2-recovery-updelay.sh TEST_FILES := \ lag_lib.sh \ diff --git a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh index 16c7fb858ac1..6dc9af1f2428 100644 --- a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh +++ b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh @@ -1,6 +1,8 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +NAMESPACES="" + # Test that a link aggregation device (bonding, team) removes the hardware # addresses that it adds on its underlying devices. test_LAG_cleanup() @@ -59,3 +61,108 @@ test_LAG_cleanup() log_test "$driver cleanup mode $mode" } + +# Build a generic 2 node net namespace with 2 connections +# between the namespaces +# +# +-----------+ +-----------+ +# | node1 | | node2 | +# | | | | +# | | | | +# | eth0 +-------+ eth0 | +# | | | | +# | eth1 +-------+ eth1 | +# | | | | +# +-----------+ +-----------+ +lag_setup2x2() +{ + local state=${1:-down} + local namespaces="lag_node1 lag_node2" + + # create namespaces + for n in ${namespaces}; do + ip netns add ${n} + done + + # wire up namespaces + ip link add name lag1 type veth peer name lag1-end + ip link set dev lag1 netns lag_node1 $state name eth0 + ip link set dev lag1-end netns lag_node2 $state name eth0 + + ip link add name lag1 type veth peer name lag1-end + ip link set dev lag1 netns lag_node1 $state name eth1 + ip link set dev lag1-end netns lag_node2 $state name eth1 + + NAMESPACES="${namespaces}" +} + +# cleanup all lag related namespaces and remove the bonding module +lag_cleanup() +{ + for n in ${NAMESPACES}; do + ip netns delete ${n} >/dev/null 2>&1 || true + done + modprobe -r bonding +} + +SWITCH="lag_node1" +CLIENT="lag_node2" +CLIENTIP="172.20.2.1" +SWITCHIP="172.20.2.2" + +lag_setup_network() +{ + lag_setup2x2 "down" + + # create switch + ip netns exec ${SWITCH} ip link add br0 up type bridge + ip netns exec ${SWITCH} ip link set eth0 master br0 up + ip netns exec ${SWITCH} ip link set eth1 master br0 up + ip netns exec ${SWITCH} ip addr add ${SWITCHIP}/24 dev br0 +} + +lag_reset_network() +{ + ip netns exec ${CLIENT} ip link del bond0 + ip netns exec ${SWITCH} ip link set eth0 up + ip netns exec ${SWITCH} ip link set eth1 up +} + +create_bond() +{ + # create client + ip netns exec ${CLIENT} ip link set eth0 down + ip netns exec ${CLIENT} ip link set eth1 down + + ip netns exec ${CLIENT} ip link add bond0 type bond $@ + ip netns exec ${CLIENT} ip link set eth0 master bond0 + ip netns exec ${CLIENT} ip link set eth1 master bond0 + ip netns exec ${CLIENT} ip link set bond0 up + ip netns exec ${CLIENT} ip addr add ${CLIENTIP}/24 dev bond0 +} + +test_bond_recovery() +{ + RET=0 + + create_bond $@ + + # verify connectivity + ip netns exec ${CLIENT} ping ${SWITCHIP} -c 5 >/dev/null 2>&1 + check_err $? "No connectivity" + + # force the links of the bond down + ip netns exec ${SWITCH} ip link set eth0 down + sleep 2 + ip netns exec ${SWITCH} ip link set eth0 up + ip netns exec ${SWITCH} ip link set eth1 down + + # re-verify connectivity + ip netns exec ${CLIENT} ping ${SWITCHIP} -c 5 >/dev/null 2>&1 + + local rc=$? + check_err $rc "Bond failed to recover" + log_test "$1 ($2) bond recovery" + lag_reset_network + return 0 +} diff --git a/tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh b/tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh new file mode 100755 index 000000000000..ad4c845a4ac7 --- /dev/null +++ b/tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh @@ -0,0 +1,45 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +# Regression Test: +# When the bond is configured with down/updelay and the link state of +# slave members flaps if there are no remaining members up the bond +# should immediately select a member to bring up. (from bonding.txt +# section 13.1 paragraph 4) +# +# +-------------+ +-----------+ +# | client | | switch | +# | | | | +# | +--------| link1 |-----+ | +# | | +-------+ | | +# | | | | | | +# | | +-------+ | | +# | | bond | link2 | Br0 | | +# +-------------+ +-----------+ +# 172.20.2.1 172.20.2.2 + + +REQUIRE_MZ=no +REQUIRE_JQ=no +NUM_NETIFS=0 +lib_dir=$(dirname "$0") +source "$lib_dir"/net_forwarding_lib.sh +source "$lib_dir"/lag_lib.sh + +cleanup() +{ + lag_cleanup +} + +trap cleanup 0 1 2 + +lag_setup_network +test_bond_recovery mode 1 miimon 100 updelay 0 +test_bond_recovery mode 1 miimon 100 updelay 200 +test_bond_recovery mode 1 miimon 100 updelay 500 +test_bond_recovery mode 1 miimon 100 updelay 1000 +test_bond_recovery mode 1 miimon 100 updelay 2000 +test_bond_recovery mode 1 miimon 100 updelay 5000 +test_bond_recovery mode 1 miimon 100 updelay 10000 + +exit "$EXIT_STATUS" diff --git a/tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh b/tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh new file mode 100755 index 000000000000..2330d37453f9 --- /dev/null +++ b/tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh @@ -0,0 +1,45 @@ +#!/bin/sh +# SPDX-License-Identifier: GPL-2.0 + +# Regression Test: +# When the bond is configured with down/updelay and the link state of +# slave members flaps if there are no remaining members up the bond +# should immediately select a member to bring up. (from bonding.txt +# section 13.1 paragraph 4) +# +# +-------------+ +-----------+ +# | client | | switch | +# | | | | +# | +--------| link1 |-----+ | +# | | +-------+ | | +# | | | | | | +# | | +-------+ | | +# | | bond | link2 | Br0 | | +# +-------------+ +-----------+ +# 172.20.2.1 172.20.2.2 + + +REQUIRE_MZ=no +REQUIRE_JQ=no +NUM_NETIFS=0 +lib_dir=$(dirname "$0") +source "$lib_dir"/net_forwarding_lib.sh +source "$lib_dir"/lag_lib.sh + +cleanup() +{ + lag_cleanup +} + +trap cleanup 0 1 2 + +lag_setup_network +test_bond_recovery mode 2 miimon 100 updelay 0 +test_bond_recovery mode 2 miimon 100 updelay 200 +test_bond_recovery mode 2 miimon 100 updelay 500 +test_bond_recovery mode 2 miimon 100 updelay 1000 +test_bond_recovery mode 2 miimon 100 updelay 2000 +test_bond_recovery mode 2 miimon 100 updelay 5000 +test_bond_recovery mode 2 miimon 100 updelay 10000 + +exit "$EXIT_STATUS" diff --git a/tools/testing/selftests/drivers/net/bonding/settings b/tools/testing/selftests/drivers/net/bonding/settings index 867e118223cd..6091b45d226b 100644 --- a/tools/testing/selftests/drivers/net/bonding/settings +++ b/tools/testing/selftests/drivers/net/bonding/settings @@ -1 +1 @@ -timeout=60 +timeout=120 -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping 2022-11-18 20:30 ` [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping Jonathan Toppins @ 2022-11-22 10:53 ` Paolo Abeni 0 siblings, 0 replies; 11+ messages in thread From: Paolo Abeni @ 2022-11-22 10:53 UTC (permalink / raw) To: Jonathan Toppins, netdev, Jay Vosburgh Cc: Liang Li, Veaceslav Falico, Andy Gospodarek, Shuah Khan, linux-kernel, linux-kselftest On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: > Verify when a bond is configured with {up,down}delay and the link state > of slave members flaps if there are no remaining members up the bond > should immediately select a member to bring up. (from bonding.txt > section 13.1 paragraph 4) > > Suggested-by: Liang Li <liali@redhat•com> > Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> > --- > .../selftests/drivers/net/bonding/Makefile | 4 +- > .../selftests/drivers/net/bonding/lag_lib.sh | 107 ++++++++++++++++++ > .../net/bonding/mode-1-recovery-updelay.sh | 45 ++++++++ > .../net/bonding/mode-2-recovery-updelay.sh | 45 ++++++++ > .../selftests/drivers/net/bonding/settings | 2 +- > 5 files changed, 201 insertions(+), 2 deletions(-) > create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-1-recovery-updelay.sh > create mode 100755 tools/testing/selftests/drivers/net/bonding/mode-2-recovery-updelay.sh > > diff --git a/tools/testing/selftests/drivers/net/bonding/Makefile b/tools/testing/selftests/drivers/net/bonding/Makefile > index 6b8d2e2f23c2..0f3921908b07 100644 > --- a/tools/testing/selftests/drivers/net/bonding/Makefile > +++ b/tools/testing/selftests/drivers/net/bonding/Makefile > @@ -5,7 +5,9 @@ TEST_PROGS := \ > bond-arp-interval-causes-panic.sh \ > bond-break-lacpdu-tx.sh \ > bond-lladdr-target.sh \ > - dev_addr_lists.sh > + dev_addr_lists.sh \ > + mode-1-recovery-updelay.sh \ > + mode-2-recovery-updelay.sh > > TEST_FILES := \ > lag_lib.sh \ > diff --git a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh > index 16c7fb858ac1..6dc9af1f2428 100644 > --- a/tools/testing/selftests/drivers/net/bonding/lag_lib.sh > +++ b/tools/testing/selftests/drivers/net/bonding/lag_lib.sh > @@ -1,6 +1,8 @@ > #!/bin/bash > # SPDX-License-Identifier: GPL-2.0 > > +NAMESPACES="" > + > # Test that a link aggregation device (bonding, team) removes the hardware > # addresses that it adds on its underlying devices. > test_LAG_cleanup() > @@ -59,3 +61,108 @@ test_LAG_cleanup() > > log_test "$driver cleanup mode $mode" > } > + > +# Build a generic 2 node net namespace with 2 connections > +# between the namespaces > +# > +# +-----------+ +-----------+ > +# | node1 | | node2 | > +# | | | | > +# | | | | > +# | eth0 +-------+ eth0 | > +# | | | | > +# | eth1 +-------+ eth1 | > +# | | | | > +# +-----------+ +-----------+ > +lag_setup2x2() > +{ > + local state=${1:-down} > + local namespaces="lag_node1 lag_node2" > + > + # create namespaces > + for n in ${namespaces}; do > + ip netns add ${n} > + done > + > + # wire up namespaces > + ip link add name lag1 type veth peer name lag1-end > + ip link set dev lag1 netns lag_node1 $state name eth0 > + ip link set dev lag1-end netns lag_node2 $state name eth0 > + > + ip link add name lag1 type veth peer name lag1-end > + ip link set dev lag1 netns lag_node1 $state name eth1 > + ip link set dev lag1-end netns lag_node2 $state name eth1 > + > + NAMESPACES="${namespaces}" > +} > + > +# cleanup all lag related namespaces and remove the bonding module > +lag_cleanup() > +{ > + for n in ${NAMESPACES}; do > + ip netns delete ${n} >/dev/null 2>&1 || true > + done > + modprobe -r bonding > +} > + > +SWITCH="lag_node1" > +CLIENT="lag_node2" > +CLIENTIP="172.20.2.1" > +SWITCHIP="172.20.2.2" > + > +lag_setup_network() > +{ > + lag_setup2x2 "down" > + > + # create switch > + ip netns exec ${SWITCH} ip link add br0 up type bridge > + ip netns exec ${SWITCH} ip link set eth0 master br0 up > + ip netns exec ${SWITCH} ip link set eth1 master br0 up > + ip netns exec ${SWITCH} ip addr add ${SWITCHIP}/24 dev br0 > +} > + > +lag_reset_network() > +{ > + ip netns exec ${CLIENT} ip link del bond0 > + ip netns exec ${SWITCH} ip link set eth0 up > + ip netns exec ${SWITCH} ip link set eth1 up > +} > + > +create_bond() > +{ > + # create client > + ip netns exec ${CLIENT} ip link set eth0 down > + ip netns exec ${CLIENT} ip link set eth1 down > + > + ip netns exec ${CLIENT} ip link add bond0 type bond $@ > + ip netns exec ${CLIENT} ip link set eth0 master bond0 > + ip netns exec ${CLIENT} ip link set eth1 master bond0 > + ip netns exec ${CLIENT} ip link set bond0 up > + ip netns exec ${CLIENT} ip addr add ${CLIENTIP}/24 dev bond0 > +} > + > +test_bond_recovery() > +{ > + RET=0 > + > + create_bond $@ > + > + # verify connectivity > + ip netns exec ${CLIENT} ping ${SWITCHIP} -c 5 >/dev/null 2>&1 Minor nit: here and below you reduce the count number, to shorten significantly the tests runtime. > + check_err $? "No connectivity" > + > + # force the links of the bond down > + ip netns exec ${SWITCH} ip link set eth0 down > + sleep 2 > + ip netns exec ${SWITCH} ip link set eth0 up > + ip netns exec ${SWITCH} ip link set eth1 down > + > + # re-verify connectivity > + ip netns exec ${CLIENT} ping ${SWITCHIP} -c 5 >/dev/null 2>&1 > + > + local rc=$? > + check_err $rc "Bond failed to recover" > + log_test "$1 ($2) bond recovery" > + lag_reset_network > + return 0 Minor nit: the return statement is not needed here. Cheers, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-18 20:30 [PATCH net-next 0/2] bonding: fix bond recovery in mode 2 Jonathan Toppins 2022-11-18 20:30 ` [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping Jonathan Toppins @ 2022-11-18 20:30 ` Jonathan Toppins 2022-11-22 10:59 ` Paolo Abeni 1 sibling, 1 reply; 11+ messages in thread From: Jonathan Toppins @ 2022-11-18 20:30 UTC (permalink / raw) To: netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel Before this change when a bond in mode 2 lost link, all of its slaves lost link, the bonding device would never recover even after the expiration of updelay. This change removes the updelay when the bond currently has no usable links. Conforming to bonding.txt section 13.1 paragraph 4. Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> --- drivers/net/bonding/bond_main.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index 1cd4e71916f8..6c4348245d1f 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -2529,7 +2529,16 @@ static int bond_miimon_inspect(struct bonding *bond) struct slave *slave; bool ignore_updelay; - ignore_updelay = !rcu_dereference(bond->curr_active_slave); + if (BOND_MODE(bond) == BOND_MODE_ACTIVEBACKUP) { + ignore_updelay = !rcu_dereference(bond->curr_active_slave); + } else { + struct bond_up_slave *usable_slaves; + + usable_slaves = rcu_dereference(bond->usable_slaves); + + if (usable_slaves && usable_slaves->count == 0) + ignore_updelay = true; + } bond_for_each_slave_rcu(bond, slave, iter) { bond_propose_link_state(slave, BOND_LINK_NOCHANGE); -- 2.31.1 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-18 20:30 ` [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero Jonathan Toppins @ 2022-11-22 10:59 ` Paolo Abeni 2022-11-22 13:36 ` Jonathan Toppins 0 siblings, 1 reply; 11+ messages in thread From: Paolo Abeni @ 2022-11-22 10:59 UTC (permalink / raw) To: Jonathan Toppins, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel Hello, On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: > Before this change when a bond in mode 2 lost link, all of its slaves > lost link, the bonding device would never recover even after the > expiration of updelay. This change removes the updelay when the bond > currently has no usable links. Conforming to bonding.txt section 13.1 > paragraph 4. > > Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> Why are you targeting net-next? This looks like something suitable to the -net tree to me. If, so could you please include a Fixes tag? Note that we can add new self-tests even via the -net tree. Thanks, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 10:59 ` Paolo Abeni @ 2022-11-22 13:36 ` Jonathan Toppins 2022-11-22 14:45 ` Paolo Abeni 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Toppins @ 2022-11-22 13:36 UTC (permalink / raw) To: Paolo Abeni, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On 11/22/22 05:59, Paolo Abeni wrote: > Hello, > > On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: >> Before this change when a bond in mode 2 lost link, all of its slaves >> lost link, the bonding device would never recover even after the >> expiration of updelay. This change removes the updelay when the bond >> currently has no usable links. Conforming to bonding.txt section 13.1 >> paragraph 4. >> >> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> > > Why are you targeting net-next? This looks like something suitable to > the -net tree to me. If, so could you please include a Fixes tag? > > Note that we can add new self-tests even via the -net tree. > I could not find a reasonable fixes tag for this, hence why I targeted the net-next tree. -Jon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 13:36 ` Jonathan Toppins @ 2022-11-22 14:45 ` Paolo Abeni 2022-11-22 15:37 ` Jonathan Toppins 0 siblings, 1 reply; 11+ messages in thread From: Paolo Abeni @ 2022-11-22 14:45 UTC (permalink / raw) To: Jonathan Toppins, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On Tue, 2022-11-22 at 08:36 -0500, Jonathan Toppins wrote: > On 11/22/22 05:59, Paolo Abeni wrote: > > Hello, > > > > On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: > > > Before this change when a bond in mode 2 lost link, all of its slaves > > > lost link, the bonding device would never recover even after the > > > expiration of updelay. This change removes the updelay when the bond > > > currently has no usable links. Conforming to bonding.txt section 13.1 > > > paragraph 4. > > > > > > Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> > > > > Why are you targeting net-next? This looks like something suitable to > > the -net tree to me. If, so could you please include a Fixes tag? > > > > Note that we can add new self-tests even via the -net tree. > > > > I could not find a reasonable fixes tag for this, hence why I targeted > the net-next tree. When in doubt I think it's preferrable to point out a commit surely affected by the issue - even if that is possibly not the one introducing the issue - than no Fixes as all. The lack of tag will make more difficult the work for stable teams. In this specific case I think that: Fixes: 41f891004063 ("bonding: ignore updelay param when there is no active slave") should be ok, WDYT? if you agree would you mind repost for -net? Thanks, Paolo ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 14:45 ` Paolo Abeni @ 2022-11-22 15:37 ` Jonathan Toppins 2022-11-22 21:12 ` Nikolay Aleksandrov 0 siblings, 1 reply; 11+ messages in thread From: Jonathan Toppins @ 2022-11-22 15:37 UTC (permalink / raw) To: Paolo Abeni, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On 11/22/22 09:45, Paolo Abeni wrote: > On Tue, 2022-11-22 at 08:36 -0500, Jonathan Toppins wrote: >> On 11/22/22 05:59, Paolo Abeni wrote: >>> Hello, >>> >>> On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: >>>> Before this change when a bond in mode 2 lost link, all of its slaves >>>> lost link, the bonding device would never recover even after the >>>> expiration of updelay. This change removes the updelay when the bond >>>> currently has no usable links. Conforming to bonding.txt section 13.1 >>>> paragraph 4. >>>> >>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> >>> >>> Why are you targeting net-next? This looks like something suitable to >>> the -net tree to me. If, so could you please include a Fixes tag? >>> >>> Note that we can add new self-tests even via the -net tree. >>> >> >> I could not find a reasonable fixes tag for this, hence why I targeted >> the net-next tree. > > When in doubt I think it's preferrable to point out a commit surely > affected by the issue - even if that is possibly not the one > introducing the issue - than no Fixes as all. The lack of tag will make > more difficult the work for stable teams. > > In this specific case I think that: > > Fixes: 41f891004063 ("bonding: ignore updelay param when there is no active slave") > > should be ok, WDYT? if you agree would you mind repost for -net? > > Thanks, > > Paolo > Yes that looks like a good one. I will repost to -net a v2 that includes changes to reduce the number of icmp echos sent before failing the test. Thanks, -Jon ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 15:37 ` Jonathan Toppins @ 2022-11-22 21:12 ` Nikolay Aleksandrov 2022-11-22 21:15 ` Nikolay Aleksandrov 0 siblings, 1 reply; 11+ messages in thread From: Nikolay Aleksandrov @ 2022-11-22 21:12 UTC (permalink / raw) To: Jonathan Toppins, Paolo Abeni, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On 22/11/2022 17:37, Jonathan Toppins wrote: > On 11/22/22 09:45, Paolo Abeni wrote: >> On Tue, 2022-11-22 at 08:36 -0500, Jonathan Toppins wrote: >>> On 11/22/22 05:59, Paolo Abeni wrote: >>>> Hello, >>>> >>>> On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: >>>>> Before this change when a bond in mode 2 lost link, all of its slaves >>>>> lost link, the bonding device would never recover even after the >>>>> expiration of updelay. This change removes the updelay when the bond >>>>> currently has no usable links. Conforming to bonding.txt section 13.1 >>>>> paragraph 4. >>>>> >>>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> >>>> >>>> Why are you targeting net-next? This looks like something suitable to >>>> the -net tree to me. If, so could you please include a Fixes tag? >>>> >>>> Note that we can add new self-tests even via the -net tree. >>>> >>> >>> I could not find a reasonable fixes tag for this, hence why I targeted >>> the net-next tree. >> >> When in doubt I think it's preferrable to point out a commit surely >> affected by the issue - even if that is possibly not the one >> introducing the issue - than no Fixes as all. The lack of tag will make >> more difficult the work for stable teams. >> >> In this specific case I think that: >> >> Fixes: 41f891004063 ("bonding: ignore updelay param when there is no active slave") >> >> should be ok, WDYT? if you agree would you mind repost for -net? >> >> Thanks, >> >> Paolo >> > > Yes that looks like a good one. I will repost to -net a v2 that includes changes to reduce the number of icmp echos sent before failing the test. > > Thanks, > -Jon > One minor nit - could you please change "mode 2" to "mode balance-xor" ? It saves reviewers some grepping around the code to see what is mode 2. Obviously one has to dig in the code to see how it's affected, but still it is a bit more understandable. It'd be nice to add more as to why the link is not recovered, I get it after reading the code, but it would be nice to include a more detailed explanation in the commit message as well. Thanks, Nik ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 21:12 ` Nikolay Aleksandrov @ 2022-11-22 21:15 ` Nikolay Aleksandrov 2022-11-22 21:17 ` Jonathan Toppins 0 siblings, 1 reply; 11+ messages in thread From: Nikolay Aleksandrov @ 2022-11-22 21:15 UTC (permalink / raw) To: Jonathan Toppins, Paolo Abeni, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On 22/11/2022 23:12, Nikolay Aleksandrov wrote: > On 22/11/2022 17:37, Jonathan Toppins wrote: >> On 11/22/22 09:45, Paolo Abeni wrote: >>> On Tue, 2022-11-22 at 08:36 -0500, Jonathan Toppins wrote: >>>> On 11/22/22 05:59, Paolo Abeni wrote: >>>>> Hello, >>>>> >>>>> On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: >>>>>> Before this change when a bond in mode 2 lost link, all of its slaves >>>>>> lost link, the bonding device would never recover even after the >>>>>> expiration of updelay. This change removes the updelay when the bond >>>>>> currently has no usable links. Conforming to bonding.txt section 13.1 >>>>>> paragraph 4. >>>>>> >>>>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> >>>>> >>>>> Why are you targeting net-next? This looks like something suitable to >>>>> the -net tree to me. If, so could you please include a Fixes tag? >>>>> >>>>> Note that we can add new self-tests even via the -net tree. >>>>> >>>> >>>> I could not find a reasonable fixes tag for this, hence why I targeted >>>> the net-next tree. >>> >>> When in doubt I think it's preferrable to point out a commit surely >>> affected by the issue - even if that is possibly not the one >>> introducing the issue - than no Fixes as all. The lack of tag will make >>> more difficult the work for stable teams. >>> >>> In this specific case I think that: >>> >>> Fixes: 41f891004063 ("bonding: ignore updelay param when there is no active slave") >>> >>> should be ok, WDYT? if you agree would you mind repost for -net? >>> >>> Thanks, >>> >>> Paolo >>> >> >> Yes that looks like a good one. I will repost to -net a v2 that includes changes to reduce the number of icmp echos sent before failing the test. >> >> Thanks, >> -Jon >> > > One minor nit - could you please change "mode 2" to "mode balance-xor" ? > It saves reviewers some grepping around the code to see what is mode 2. > Obviously one has to dig in the code to see how it's affected, but still > it is a bit more understandable. It'd be nice to add more as to why the link is not recovered, > I get it after reading the code, but it would be nice to include a more detailed explanation in the > commit message as well. > > Thanks, > Nik > Ah, I just noticed I'm late to the party. :) Nevermind my comments, no need for a v3. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero 2022-11-22 21:15 ` Nikolay Aleksandrov @ 2022-11-22 21:17 ` Jonathan Toppins 0 siblings, 0 replies; 11+ messages in thread From: Jonathan Toppins @ 2022-11-22 21:17 UTC (permalink / raw) To: Nikolay Aleksandrov, Paolo Abeni, netdev, Jay Vosburgh Cc: Veaceslav Falico, Andy Gospodarek, David S. Miller, Eric Dumazet, Jakub Kicinski, linux-kernel On 11/22/22 16:15, Nikolay Aleksandrov wrote: > On 22/11/2022 23:12, Nikolay Aleksandrov wrote: >> On 22/11/2022 17:37, Jonathan Toppins wrote: >>> On 11/22/22 09:45, Paolo Abeni wrote: >>>> On Tue, 2022-11-22 at 08:36 -0500, Jonathan Toppins wrote: >>>>> On 11/22/22 05:59, Paolo Abeni wrote: >>>>>> Hello, >>>>>> >>>>>> On Fri, 2022-11-18 at 15:30 -0500, Jonathan Toppins wrote: >>>>>>> Before this change when a bond in mode 2 lost link, all of its slaves >>>>>>> lost link, the bonding device would never recover even after the >>>>>>> expiration of updelay. This change removes the updelay when the bond >>>>>>> currently has no usable links. Conforming to bonding.txt section 13.1 >>>>>>> paragraph 4. >>>>>>> >>>>>>> Signed-off-by: Jonathan Toppins <jtoppins@redhat•com> >>>>>> >>>>>> Why are you targeting net-next? This looks like something suitable to >>>>>> the -net tree to me. If, so could you please include a Fixes tag? >>>>>> >>>>>> Note that we can add new self-tests even via the -net tree. >>>>>> >>>>> >>>>> I could not find a reasonable fixes tag for this, hence why I targeted >>>>> the net-next tree. >>>> >>>> When in doubt I think it's preferrable to point out a commit surely >>>> affected by the issue - even if that is possibly not the one >>>> introducing the issue - than no Fixes as all. The lack of tag will make >>>> more difficult the work for stable teams. >>>> >>>> In this specific case I think that: >>>> >>>> Fixes: 41f891004063 ("bonding: ignore updelay param when there is no active slave") >>>> >>>> should be ok, WDYT? if you agree would you mind repost for -net? >>>> >>>> Thanks, >>>> >>>> Paolo >>>> >>> >>> Yes that looks like a good one. I will repost to -net a v2 that includes changes to reduce the number of icmp echos sent before failing the test. >>> >>> Thanks, >>> -Jon >>> >> >> One minor nit - could you please change "mode 2" to "mode balance-xor" ? >> It saves reviewers some grepping around the code to see what is mode 2. >> Obviously one has to dig in the code to see how it's affected, but still >> it is a bit more understandable. It'd be nice to add more as to why the link is not recovered, >> I get it after reading the code, but it would be nice to include a more detailed explanation in the >> commit message as well. >> >> Thanks, >> Nik >> > > Ah, I just noticed I'm late to the party. :) > Nevermind my comments, no need for a v3. > If there are other issues with v2. I will gladly include these comments in a v3. Thanks, -Jon ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-22 21:18 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-11-18 20:30 [PATCH net-next 0/2] bonding: fix bond recovery in mode 2 Jonathan Toppins 2022-11-18 20:30 ` [PATCH net-next 1/2] selftests: bonding: up/down delay w/ slave link flapping Jonathan Toppins 2022-11-22 10:53 ` Paolo Abeni 2022-11-18 20:30 ` [PATCH net-next 2/2] bonding: fix link recovery in mode 2 when updelay is nonzero Jonathan Toppins 2022-11-22 10:59 ` Paolo Abeni 2022-11-22 13:36 ` Jonathan Toppins 2022-11-22 14:45 ` Paolo Abeni 2022-11-22 15:37 ` Jonathan Toppins 2022-11-22 21:12 ` Nikolay Aleksandrov 2022-11-22 21:15 ` Nikolay Aleksandrov 2022-11-22 21:17 ` Jonathan Toppins
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox