bonding: Set the correct value to fail_over_mac at enslavement
Subsystems (3)
PATCH 1/3 bonding: Set the correct value to fail_over_mac at enslavement
If the new slave don't support setting the MAC address, there are two ways to handle this situation:
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, set fail_over_mac to active, otherwise set fail_over_mac to none.
2). If the new slave is not the first slave and the fail_over_mac is
active, it means that the slave could work normally in active-backup mode, otherwise if the fail_over_mac is none, the slave could not work normally for no active-backup mode, so bond could not ensalve the new dev.
--- drivers/net/bonding/bond_main.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/net/bonding/bond_main.c +10 -2
@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)if (slave_ops->ndo_set_mac_address ==NULL ) {if (!bond_has_slaves (bond)) { -pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active." , +pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address.\n" , bond_dev->name); - bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) { + bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +pr_warning ("%s: Setting fail_over_mac to active for active-backup mode.\n" , + bond_dev->name); + }else { + bond->params.fail_over_mac =BOND_FOM_NONE ; +pr_warning ("%s: Setting fail_over_mac to none for no active-backup modes" , + bond_dev->name); + } }else if (bond->params.fail_over_mac !=BOND_FOM_ACTIVE ) {pr_err ("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n" , bond_dev->name);
Signature · 1 line
1.8.0
Quoted 29 lines · Ding Tianhong
If the new slave don't support setting the MAC address, there are two ways to handle this situation:
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, setfail_over_macto active, otherwise setfail_over_macto none.
2). If the new slave is not the first slave and thefail_over_macis
active, it means that the slave could work normally in active-backup mode, otherwise if thefail_over_macis none, the slave could not work normally for no active-backup mode, so bond could not ensalve the new dev.Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Cc Andy Gospodarek <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_main.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)drivers/net/bonding/bond_main.c +0
-1@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)if (slave_ops->ndo_set_mac_address ==NULL ) {if (!bond_has_slaves (bond)) { - pr_warning("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active.",
drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1345
Quoted 10 lines
drivers/net/bonding/bond_main.c +8
-1+pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address.\n" , bond_dev->name); - bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) { + bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +pr_warning ("%s: Setting fail_over_mac to active for active-backup mode.\n" , + bond_dev->name); + }else { + bond->params.fail_over_mac =BOND_FOM_NONE ; + pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",
At least make the warnings consistent: "... for no active-backup mode.\n". Also you might consider switching to pr_warn.
Quoted 6 lines
drivers/net/bonding/bond_main.c +2
-0+ bond_dev->name); + } }else if (bond->params.fail_over_mac !=BOND_FOM_ACTIVE ) {pr_err ("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n" , bond_dev->name);
Quoted 27 lines · Nikolay Aleksandrov
Quoted 26 lines · Ding Tianhong
If the new slave don't support setting the MAC address, there are two ways to handle this situation:
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, setfail_over_macto active, otherwise setfail_over_macto none.
2). If the new slave is not the first slave and thefail_over_macis
active, it means that the slave could work normally in active-backup mode, otherwise if thefail_over_macis none, the slave could not work normally for no active-backup mode, so bond could not ensalve the new dev.Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Cc Andy Gospodarek <[email protected]>Signed-off-by Ding Tianhong <[email protected]>--- drivers/net/bonding/bond_main.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)drivers/net/bonding/bond_main.c +0
-0@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)
Quoted 16 lines
drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1345
Quoted 13 lines
drivers/net/bonding/bond_main.c +8
-2if (slave_ops->ndo_set_mac_address ==NULL ) {if (!bond_has_slaves (bond)) { -pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active." , +pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address.\n" , bond_dev->name); - bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) { + bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +pr_warning ("%s: Setting fail_over_mac to active for active-backup mode.\n" , + bond_dev->name); + }else { + bond->params.fail_over_mac =BOND_FOM_NONE ; + pr_warning("%s: Setting fail_over_mac to none for no active-backup modes",At least make the warnings consistent: "... for no active-backup mode.\n". Also you might consider switching to
pr_warn.
ok, thanks.
Quoted 10 lines
drivers/net/bonding/bond_main.c int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev) · line 1350
Quoted 6 lines
drivers/net/bonding/bond_main.c +2
-0+ bond_dev->name); + } }else if (bond->params.fail_over_mac !=BOND_FOM_ACTIVE ) {pr_err ("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n" , bond_dev->name);.
Quoted 6 lines · Ding Tianhong
If the new slave don't support setting the MAC address, there are two ways to handle this situation:
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, setfail_over_macto active, otherwise setfail_over_macto none.
This should be "if the mode is active-backup, set fail_over_mac
to active, otherwise do not change fail_over_mac." Setting to none here would undo any setting of fail_over_mac that the user had set prior to adding the first slave.
Quoted 5 lines
2). If the new slave is not the first slave and the
fail_over_macis
active, it means that the slave could work normally in active-backup mode, otherwise if thefail_over_macis none, the slave could not work normally for no active-backup mode, so bond could not ensalve the new dev.
This (#2) is not a code change, correct? You're just restating
the existing behavior of the code, right?
Also, I don't see where this patch set updates the slave removal
processing where the slave's original MAC is restored. At present, this is done by a test against fail_over_mac, but should be tested against the mode and fail_over_mac.
My comment to the prior version of this patchset, again:
The correct way to fix this in general is to permit setting an
option at any time, but only have it take effect in active-backup mode. This minimizes ordering requirements when setting options.
I would instead modify the bond enslave and removal processing
to check the mode in addition to fail_over_mac when setting a slave's MAC during enslavement. The change active slave processing already only calls the fail_over_mac function when in active-backup mode. This should also be a simpler change set.
These comments still apply to this version of the patchset. -J
Quoted 34 lines
Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Cc Andy Gospodarek <[email protected]>Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 12 ++++++++++--1 file changed, 10 insertions(+), 2 deletions(-)drivers/net/bonding/bond_main.c +10
-2@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)if (slave_ops->ndo_set_mac_address ==NULL ) {if (!bond_has_slaves (bond)) { -pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active." , +pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address.\n" , bond_dev->name); - bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) { + bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +pr_warning ("%s: Setting fail_over_mac to active for active-backup mode.\n" , + bond_dev->name); + }else { + bond->params.fail_over_mac =BOND_FOM_NONE ; +pr_warning ("%s: Setting fail_over_mac to none for no active-backup modes" , + bond_dev->name); + } }else if (bond->params.fail_over_mac !=BOND_FOM_ACTIVE ) {pr_err ("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n" , bond_dev->name);Signature · 1 line
1.8.0
---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]
Quoted 5 lines · Jay Vosburgh
Quoted 3 lines · Ding Tianhong
If the new slave don't support setting the MAC address, there are two ways to handle this situation:
Quoted 9 lines
Quoted 3 lines
1). If the new slave is the first slave, set bond to the new slave's
MAC address, if the mode is active-backup, setfail_over_macto active, otherwise setfail_over_macto none.This should be "if the mode is active-backup, set
fail_over_mac
to active, otherwise do not changefail_over_mac." Setting to none here would undo any setting offail_over_macthat the user had set prior to adding the first slave.
I thought about this question for a long time, I still can not think clearly.
ex: the first slave did not support setting MAC, and mode is RR, fail_over_mac=1(user set it
when load the driver), so the after the enslavement, the fail_over_mac is still 1, then the second slave added, if the second slave did not support setting MAC, it will pass the check and go ahead, until the dev_set_mac_address(), and return error, so I think set it to none may cause the second slave return early, but no substance logic change. I will modify this place as your opinion.
Quoted 26 lines
Quoted 5 lines
2). If the new slave is not the first slave and the
fail_over_macis
active, it means that the slave could work normally in active-backup mode, otherwise if thefail_over_macis none, the slave could not work normally for no active-backup mode, so bond could not ensalve the new dev.This (#2) is not a code change, correct? You're just restating
the existing behavior of the code, right?Also, I don't see where this patch set updates the slave removal
processing where the slave's original MAC is restored. At present, this is done by a test againstfail_over_mac, but should be tested against the mode andfail_over_mac.My comment to the prior version of this patchset, again:
The correct way to fix this in general is to permit setting an
option at any time, but only have it take effect in active-backup mode. This minimizes ordering requirements when setting options.I would instead modify the bond enslave and removal processing
to check the mode in addition tofail_over_macwhen setting a slave's MAC during enslavement. The change active slave processing already only calls thefail_over_macfunction when in active-backup mode. This should also be a simpler change set.
Quoted 4 lines
These comments still apply to this version of the patchset. -J
OK.
Regards
Ding
Quoted 41 lines
Quoted 34 lines
Cc Jay Vosburgh <[email protected]>Cc Veaceslav Falico <[email protected]>Cc Andy Gospodarek <[email protected]>Signed-off-by Ding Tianhong <[email protected]>---
drivers/net/bonding/bond_main.c| 12 ++++++++++--1 file changed, 10 insertions(+), 2 deletions(-)drivers/net/bonding/bond_main.c +10
-2@@ -1334,9 +1334,17 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev)if (slave_ops->ndo_set_mac_address ==NULL ) {if (!bond_has_slaves (bond)) { -pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address. Setting fail_over_mac to active." , +pr_warning ("%s: Warning: The first slave device specified does not support setting the MAC address.\n" , bond_dev->name); - bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +if (bond->params.mode ==BOND_MODE_ACTIVEBACKUP ) { + bond->params.fail_over_mac =BOND_FOM_ACTIVE ; +pr_warning ("%s: Setting fail_over_mac to active for active-backup mode.\n" , + bond_dev->name); + }else { + bond->params.fail_over_mac =BOND_FOM_NONE ; +pr_warning ("%s: Setting fail_over_mac to none for no active-backup modes" , + bond_dev->name); + } }else if (bond->params.fail_over_mac !=BOND_FOM_ACTIVE ) {pr_err ("%s: Error: The slave device specified does not support setting the MAC address, but fail_over_mac is not set to active.\n" , bond_dev->name);Signature · 1 line
1.8.0---
-Jay Vosburgh, IBM Linux Technology Center, [email protected].