Re: [PATCH net] bonding: fix active-backup transition after link failure

[Jay Vosburgh <jay.vosburgh@canonical.com>]

Mahesh Bandewar (महेश बंडेवार) wrote:

>On Wed, Dec 11, 2019 at 10:39 PM Jay Vosburgh
><jay.vosburgh@canonical.com> wrote:
>>
>> Mahesh Bandewar (महेश बंडेवार) wrote:
>>
>> >On Sat, Dec 7, 2019 at 2:09 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote:
>> >>
>> >> Mahesh Bandewar <maheshb@google.com> wrote:
>> >>
>> >> >After the recent fix 1899bb325149 ("bonding: fix state transition
>> >> >issue in link monitoring"), the active-backup mode with miimon
>> >> >initially come-up fine but after a link-failure, both members
>> >> >transition into backup state.
>> >> >
>> >> >Following steps to reproduce the scenario (eth1 and eth2 are the
>> >> >slaves of the bond):
>> >> >
>> >> >    ip link set eth1 up
>> >> >    ip link set eth2 down
>> >> >    sleep 1
>> >> >    ip link set eth2 up
>> >> >    ip link set eth1 down
>> >> >    cat /sys/class/net/eth1/bonding_slave/state
>> >> >    cat /sys/class/net/eth2/bonding_slave/state
>> >> >
>> >> >Fixes: 1899bb325149 ("bonding: fix state transition issue in link monitoring")
>> >> >CC: Jay Vosburgh <jay.vosburgh@canonical.com>
>> >> >Signed-off-by: Mahesh Bandewar <maheshb@google.com>
>> >> >---
>> >> > drivers/net/bonding/bond_main.c | 3 ---
>> >> > 1 file changed, 3 deletions(-)
>> >> >
>> >> >diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>> >> >index fcb7c2f7f001..ad9906c102b4 100644
>> >> >--- a/drivers/net/bonding/bond_main.c
>> >> >+++ b/drivers/net/bonding/bond_main.c
>> >> >@@ -2272,9 +2272,6 @@ static void bond_miimon_commit(struct bonding *bond)
>> >> >                       } else if (BOND_MODE(bond) != BOND_MODE_ACTIVEBACKUP) {
>> >> >                               /* make it immediately active */
>> >> >                               bond_set_active_slave(slave);
>> >> >-                      } else if (slave != primary) {
>> >> >-                              /* prevent it from being the active one */
>> >> >-                              bond_set_backup_slave(slave);
>> >>
>> >>         How does this fix things?  Doesn't bond_select_active_slave() ->
>> >> bond_change_active_slave() set the backup flag correctly via a call to
>> >> bond_set_slave_active_flags() when it sets a slave to be the active
>> >> slave?  If this change resolves the problem, I'm not sure how this ever
>> >> worked correctly, even prior to 1899bb325149.
>> >>
>> >Hi Jay, I used kprobes to figure out the brokenness this patch fixes.
>> >Prior to your patch this call would not happen but with the patch,
>> >this extra call will put the master into the backup mode erroneously
>> >(in fact both members would be in backup state). The mechanics you
>> >have mentioned works correctly except that in the prior case, the
>> >switch statement was using new_link which was not same as
>> >link_new_state. The miimon_inspect will update new_link which is what
>> >was used in miimon_commit code. The link_new_state was used only to
>> >mitigate the rtnl-lock issue which would update the "link". Hence in
>> >the prior code, this path would never get executed.
>>
>>         I'm looking at the old code (prior to 1899bb325149), and I don't
>> see a path to what you're describing for the down to up transition in
>> active-backup mode.
>>
>I was referring to the code where bond_miimon_inspect() switches using
>bond->link and bond_miimon_commit() (which happens after inspect)
>switches using bond->new_link. inspect doesn't touch new_link unless
>delay is set which is a corner case and probably ignore for this
>purpose since it's just postponing the behavior.
>bond->link_new_state was brought in to mitigate RTNL issue and affects
>only bond->link, if it can acquire RTNL. So irrespective of what
>bond_miimon_inspect() does for bond->link or bond->link_new_state the
>bond->new_link was maintained and then used in the bond_miimon_commit.
>Because of this the wrong transition wouldn't happen.
>
>Once the new_link and link_new_state is merged, the state that
>bond_miimon_inspect() sets for bond->link_new_state *is* used in
>bond_miimon_commit() (which is after the fact) and hence (I believe)
>the erroneous transition.
>
>Having said that, the fix that you put in is necessary to close the
>window between link_propose() and link_commit() but the side effect of
>that was the situation that I explained
>above which is what this patch fixes it.

	Ok, I think I understand, and am fine with the patch as-is.

Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

	-J

>> bond_miimon_inspect enters switch, slave->link == BOND_LINK_DOWN.
>>
>> link_state is nonzero, call bond_propose_link_state(BOND_LINK_BACK),
>> which sets slave->link_new_state to _BACK.
>>
>> Fall through to BOND_LINK_BACK case, set slave->new_link = BOND_LINK_UP
>>
>> bond_mii_monitor then calls bond_commit_link_state, which sets
>> slave->link to BOND_LINK_BACK
>>
>> Enter bond_miimon_commit switch (new_link), which is BOND_LINK_UP
>>
>> In "case BOND_LINK_UP:" there is no way out of this block, and it should
>> proceed to call bond_set_backup_slave for active-backup mode every time.
>>
>> >The steps to reproduce this issue is straightforward and happens 100%
>> >of the time (I used two mlx interfaces but that shouldn't matter).
>>
>>         Yes, I've been able to reproduce it locally (with igb, FWIW).  I
>> think the patch is likely ok, I'm just mystified as to how the backup
>> setting could have worked prior to 1899bb325149, so perhaps the Fixes
>> tag doesn't go back far enough.
>>
>Well, I added fixes-tag since the behavior started as soon as the
>1899bb325149 was added. I don't see the issue if I revert
>1899bb325149.
>
>
>>         -J
>>
>> >thanks,
>> >--mahesh..
>> >>         -J
>> >>
>> >> >                       }
>> >> >
>> >> >                       slave_info(bond->dev, slave->dev, "link status definitely up, %u Mbps %s duplex\n",
>> >> >--
>> >> >2.24.0.393.g34dc348eaf-goog

---
	-Jay Vosburgh, jay.vosburgh@canonical.com