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

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

[Mahesh Bandewar]

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);
 			}
 
 			slave_info(bond->dev, slave->dev, "link status definitely up, %u Mbps %s duplex\n",
-- 
2.24.0.393.g34dc348eaf-goog

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

[Jay Vosburgh]

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.

	-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

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

[Mahesh Bandewar (महेश बंडेवार)]

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.

The steps to reproduce this issue is straightforward and happens 100%
of the time (I used two mlx interfaces but that shouldn't matter).

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

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

[Mahesh Bandewar (महेश बंडेवार)]

On Mon, Dec 9, 2019 at 10:41 AM Mahesh Bandewar (महेश बंडेवार)
<maheshb@google.com> 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.
>
> The steps to reproduce this issue is straightforward and happens 100%
> of the time (I used two mlx interfaces but that shouldn't matter).
>
A friendly ping.

> 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

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

[Jay Vosburgh]

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.

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.

	-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

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

[Mahesh Bandewar (महेश बंडेवार)]

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

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

[Jay Vosburgh]

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

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

[Jakub Kicinski]

On Fri, 13 Dec 2019 12:28:37 -0800, Jay Vosburgh wrote:
> 	Ok, I think I understand, and am fine with the patch as-is.
> 
> Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>

Okay, applied then, and queued for 4.14+ stable.

Mahesh, I reworded the commit message slightly, checkpatch insists
that the word "commit" occurs before the hash when quoting.

Thanks!

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

[Mahesh Bandewar (महेश बंडेवार)]

On Sat, Dec 14, 2019 at 4:29 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 13 Dec 2019 12:28:37 -0800, Jay Vosburgh wrote:
> >       Ok, I think I understand, and am fine with the patch as-is.
> >
> > Acked-by: Jay Vosburgh <jay.vosburgh@canonical.com>
>
> Okay, applied then, and queued for 4.14+ stable.
>
> Mahesh, I reworded the commit message slightly, checkpatch insists
> that the word "commit" occurs before the hash when quoting.
>
Thanks and good to know, I'll keep that in mind.

> Thanks!