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

["Mahesh Bandewar (महेश बंडेवार)" <maheshb@google.com>]

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.

> 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