Skip to contentSkip to conversation

bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

PATCH In review Ding Tianhong 2 participants 3 messages Jan 21, 2014 – Jan 22, 2014

Subsystems (3)
Networking Network drivers Bonding driver
lore
Share & tools
lore

Ding Tianhong

author
#0 lore raw

PATCH 1/5 bonding: The fail_over_mac should be set only in ACTIVE_BACKUP mode

According the bonding.txt, the option fail_over_mac only affect for AB mode, but in currect code, the parameter could be set to active or follow in every mode, this will cause bonding could not set all slaves of an RR or XOR mode to the same MAC address at enslavement time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.

Fix the wrong variables for pr_err().

Signed-off-by Ding Tianhong <[email protected]>
---
 drivers/net/bonding/bond_main.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/bonding/bond_main.c +5 -3
index 3220b48..ecff04e 100644
@@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
 						      fail_over_mac_tbl);
 		if (fail_over_mac_value == -1) {
 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
-			       arp_validate == NULL ? "NULL" : arp_validate);
+			       fail_over_mac == NULL ? "NULL" : fail_over_mac);
 			return -EINVAL;
 		}
 
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
-			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
+		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
+			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
+			fail_over_mac_value = BOND_FOM_NONE;
+		}
 	} else {
 		fail_over_mac_value = BOND_FOM_NONE;
 	}
Signature · 1 line
1.8.0
#1 lore raw
Quoted 5 lines · Ding Tianhong

According the bonding.txt, the option fail_over_mac only affect for AB mode, but in currect code, the parameter could be set to active or follow in every mode, this will cause bonding could not set all slaves of an RR or XOR mode to the same MAC address at enslavement time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.

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.

Quoted 12 lines

Fix the wrong variables for pr_err().

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 8 +++++---

1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/bonding/bond_main.c +0 -0
index 3220b48..ecff04e 100644

drivers/net/bonding/bond_main.c static int bond_check_params(struct bond_params *params) · line 4310

Quoted 5 lines
drivers/net/bonding/bond_main.c +1 -1
@@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
 						      fail_over_mac_tbl);
 		if (fail_over_mac_value == -1) {
 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
-			       arp_validate == NULL ? "NULL" : arp_validate);
+			       fail_over_mac == NULL ? "NULL" : fail_over_mac);

This part is ok.

drivers/net/bonding/bond_main.c static int bond_check_params(struct bond_params *params) · line 4317

Quoted 9 lines
drivers/net/bonding/bond_main.c +4 -2
 			return -EINVAL;
 		}
 
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
-			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
+		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
+			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
+			fail_over_mac_value = BOND_FOM_NONE;
+		}

This part is not.

I would additionally NAK patches 2, 3, and 4 (noting that 4
inhibits the change to fail_over_mac, but not the warning message that we're changing it).

Patch 5 is ok, although it really has nothing to do with
fail_over_mac.

	-J
Quoted 5 lines
drivers/net/bonding/bond_main.c +0 -0
 	} else {
 		fail_over_mac_value = BOND_FOM_NONE;
 	}
Signature · 1 line
1.8.0

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

author
#2 lore raw
Quoted 8 lines · Jay Vosburgh
Quoted 5 lines · Ding Tianhong

According the bonding.txt, the option fail_over_mac only affect for AB mode, but in currect code, the parameter could be set to active or follow in every mode, this will cause bonding could not set all slaves of an RR or XOR mode to the same MAC address at enslavement time, so reset fail_over_mac to 0 if the mode is not ACTIVE_BACKUP.

Quoted 4 lines

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.

ok

Quoted 6 lines

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.

agree, the current modify actually more redundant.

Quoted 32 lines
Quoted 12 lines

Fix the wrong variables for pr_err().

Signed-off-by Ding Tianhong <[email protected]>

---
drivers/net/bonding/bond_main.c | 8 +++++---

1 file changed, 5 insertions(+), 3 deletions(-)
drivers/net/bonding/bond_main.c +0 -0
index 3220b48..ecff04e 100644

drivers/net/bonding/bond_main.c static int bond_check_params(struct bond_params *params) · line 4310

Quoted 5 lines
drivers/net/bonding/bond_main.c +1 -1
@@ -4307,12 +4307,14 @@ static int bond_check_params(struct bond_params *params)
 						      fail_over_mac_tbl);
 		if (fail_over_mac_value == -1) {
 			pr_err("Error: invalid fail_over_mac \"%s\"\n",
-			       arp_validate == NULL ? "NULL" : arp_validate);
+			       fail_over_mac == NULL ? "NULL" : fail_over_mac);

This part is ok.

drivers/net/bonding/bond_main.c static int bond_check_params(struct bond_params *params) · line 4317

Quoted 9 lines
drivers/net/bonding/bond_main.c +4 -2
 			return -EINVAL;
 		}
 
-		if (bond_mode != BOND_MODE_ACTIVEBACKUP)
-			pr_warning("Warning: fail_over_mac only affects active-backup mode.\n");
+		if (bond_mode != BOND_MODE_ACTIVEBACKUP) {
+			pr_warning("Warning: fail_over_mac only affects active-backup mode, set it to 0.\n");
+			fail_over_mac_value = BOND_FOM_NONE;
+		}

This part is not.

Quoted 9 lines

I would additionally NAK patches 2, 3, and 4 (noting that 4
inhibits the change to fail_over_mac, but not the warning message that we're changing it).

Patch 5 is ok, although it really has nothing to do with
fail_over_mac.

	-J

The whole patchset need to be rebuild, thanks for reviewing.

Regards
Ding

Quoted 11 lines
Quoted 5 lines
drivers/net/bonding/bond_main.c +0 -0
 	} else {
 		fail_over_mac_value = BOND_FOM_NONE;
 	}
Signature · 1 line
1.8.0

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]