correct behavior when modify primary via sysfs

If we modify primary via sysfs and it is not a valid slave, we should record it for future use, and this behavior is the same with bond_check_params().

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -1

index aef42f0..485bedb 100644
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
  		}
  	}
 
-	pr_info("%s: Unable to set %.*s as primary slave.\n",

Quoted 18 lines · Weiping Pan

If we modify primary via sysfs and it is not a valid slave, we should record it for future use, and this behavior is the same with bond_check_params().

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -1

index aef42f0..485bedb 100644
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
  		}
  	}
 
-	pr_info("%s: Unable to set %.*s as primary slave.\n",

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1093

Quoted 10 lines

drivers/net/bonding/bond_sysfs.c +6 -1

-		bond->dev->name, (int)strlen(buf) - 1, buf);
+	strncpy(bond->params.primary, ifname, IFNAMSIZ);
+	bond->params.primary[IFNAMSIZ - 1] = 0;
+
+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet.\n",
+		bond->dev->name, ifname, bond->dev->name);
  out:
  	write_unlock_bh(&bond->curr_slave_lock);
  	read_unlock(&bond->lock);

I like this one, because it tend to relax the current constraints one should respect on the order to write into sysfs to setup bonding.

May I suggest we have a better info message, suggesting there might have a typo on the name of the primary ?

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1090

Quoted 3 lines

drivers/net/bonding/bond_sysfs.c +3 -0

+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet. Possible typo?\n",
+		bond->dev->name, ifname, bond->dev->name);

Except from this cosmetic,

Acked-by Nicolas de Pesloüan <[email protected]>

Quoted 35 lines · Nicolas de Pesloüan

Quoted 18 lines · Weiping Pan

If we modify primary via sysfs and it is not a valid slave, we should record it for future use, and this behavior is the same with bond_check_params().

Signed-off-by Weiping Pan<[email protected]>

---
   drivers/net/bonding/bond_sysfs.c |    8 ++++++--
   1 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -1

index aef42f0..485bedb 100644
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
   		}
   	}
 
-	pr_info("%s: Unable to set %.*s as primary slave.\n",

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1093

Quoted 10 lines

drivers/net/bonding/bond_sysfs.c +6 -1

-		bond->dev->name, (int)strlen(buf) - 1, buf);
+	strncpy(bond->params.primary, ifname, IFNAMSIZ);
+	bond->params.primary[IFNAMSIZ - 1] = 0;
+
+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet.\n",
+		bond->dev->name, ifname, bond->dev->name);
   out:
   	write_unlock_bh(&bond->curr_slave_lock);
   	read_unlock(&bond->lock);

I like this one, because it tend to relax the current constraints one should respect on the order to write into sysfs to setup bonding.

May I suggest we have a better info message, suggesting there might have a typo on the name of the primary ?

Quoted 6 lines

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1090

Quoted 3 lines

drivers/net/bonding/bond_sysfs.c +3 -0

+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet. Possible typo?\n",
+		bond->dev->name, ifname, bond->dev->name);

Except from this cosmetic,

Acked-by Nicolas de Pesloüan<[email protected]>

Agreed, except that I can go either way on the "typo" warning.

Signed-off-by Jay Vosburgh<[email protected]>

	-J

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

--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 42 lines · Nicolas de Pesloüan

Quoted 18 lines · Weiping Pan

If we modify primary via sysfs and it is not a valid slave, we should record it for future use, and this behavior is the same with bond_check_params().

Signed-off-by Weiping Pan<[email protected]>

---
   drivers/net/bonding/bond_sysfs.c |    8 ++++++--
   1 files changed, 6 insertions(+), 2 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -1

index aef42f0..485bedb 100644
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
   		}
   	}
 
-	pr_info("%s: Unable to set %.*s as primary slave.\n",

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1093

Quoted 10 lines

drivers/net/bonding/bond_sysfs.c +6 -1

-		bond->dev->name, (int)strlen(buf) - 1, buf);
+	strncpy(bond->params.primary, ifname, IFNAMSIZ);
+	bond->params.primary[IFNAMSIZ - 1] = 0;
+
+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet.\n",
+		bond->dev->name, ifname, bond->dev->name);
   out:
   	write_unlock_bh(&bond->curr_slave_lock);
   	read_unlock(&bond->lock);

I like this one, because it tend to relax the current constraints one should respect on the order to write into sysfs to setup bonding.

May I suggest we have a better info message, suggesting there might have a typo on the name of the primary ?

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1090

Quoted 3 lines

drivers/net/bonding/bond_sysfs.c +3 -0

+	pr_info("%s: Recording %s as primary, "
+		"but it has not been enslaved to %s yet. Possible typo?\n",
+		bond->dev->name, ifname, bond->dev->name);

Except from this cosmetic,

Quoted 1 line

Acked-by Nicolas de Pesloüan<[email protected]>

Agreed, except that I can go either way on the "typo" warning.

Signed-off-by Jay Vosburgh<[email protected]>

I think this patch (http://patchwork.ozlabs.org/patch/164100/) was erroneously flagged as "Changes Requested". Despite my suggestion to add a "possible typo" warning, I acked the patch and so do Jay. We eventually decided not to add the "possible typo" warning, so the patch should be accepted.

Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -0

index 485bedb..1b0f3cd 100644
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
  	if (!rtnl_trylock())
  		return restart_syscall();
 

Quoted 15 lines · Weiping Pan

Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -0

index 485bedb..1b0f3cd 100644
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
  	if (!rtnl_trylock())
  		return restart_syscall();
 

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary_reselect(struct device *d, · line 1135

Quoted 10 lines

drivers/net/bonding/bond_sysfs.c +7 -0

+	if (!USES_PRIMARY(bond->params.mode)) {
+		pr_err("%s: Unable to set primary_reselect; %s is in mode %d\n",
+			bond->dev->name, bond->dev->name, bond->params.mode);
+		ret = -EINVAL;
+		goto out;
+	}
+
  	new_value = bond_parse_parm(buf, pri_reselect_tbl);
  	if (new_value<  0)  {
  		pr_err("%s: Ignoring invalid primary_reselect value %.*s.\n",

May I suggest we only issue a warning, store the new value for primary_reselect, and avoid calling bond_select_active_slave(bond), if !USE_PRIMARY(bond->params.mode)?

That way, we do not add one more constraint on the order one must write into sysfs.

Quoted 32 lines · Nicolas de Pesloüan

Quoted 15 lines · Weiping Pan

Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by Weiping Pan<[email protected]>

---
   drivers/net/bonding/bond_sysfs.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -0

index 485bedb..1b0f3cd 100644
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
   	if (!rtnl_trylock())
   		return restart_syscall();
 

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary_reselect(struct device *d, · line 1135

Quoted 10 lines

drivers/net/bonding/bond_sysfs.c +7 -0

+	if (!USES_PRIMARY(bond->params.mode)) {
+		pr_err("%s: Unable to set primary_reselect; %s is in mode %d\n",
+			bond->dev->name, bond->dev->name, bond->params.mode);
+		ret = -EINVAL;
+		goto out;
+	}
+
   	new_value = bond_parse_parm(buf, pri_reselect_tbl);
   	if (new_value<   0)  {
   		pr_err("%s: Ignoring invalid primary_reselect value %.*s.\n",

May I suggest we only issue a warning, store the new value for primary_reselect, and avoid calling bond_select_active_slave(bond), if !USE_PRIMARY(bond->params.mode)?

Quoted 1 line

That way, we do not add one more constraint on the order one must write into sysfs.

I'm not in favor of changing anything here. There's already a
message that primary_reselect is being changed, I think that's sufficient. The other similar cases don't issue warnings, e.g., setting xmit_hash_policy doesn't complain if the mode is not one that utilizes the hash.

Quoted 1 line · Jay Vosburgh

Nicolas de Pesloüan <[email protected]> wrote:

Quoted 11 lines

Quoted 5 lines

May I suggest we only issue a warning, store the new value for primary_reselect, and avoid calling bond_select_active_slave(bond), if !USE_PRIMARY(bond->params.mode)?

That way, we do not add one more constraint on the order one must write into sysfs.

I'm not in favor of changing anything here. There's already a
message that primary_reselect is being changed, I think that's sufficient. The other similar cases don't issue warnings, e.g., setting xmit_hash_policy doesn't complain if the mode is not one that utilizes the hash.

Agreed. Calling bond_select_active_slave(bond) looks safe, even for mode that does not use primary, so we don't need to change anything.

Would you support other patch similar to 1/3 in this thread, that try to relax the order to write into sysfs for bonding?

When we set primary slave with module parameters, bond will always use this primary slave as active slave.

But when we modify primary slave via sysfs, it will call bond_should_change_active() and take into account primary_reselect.

And I think we should use the new primary slave as the new active slave regardless of the value of primary_reselect.
Thus the behavior is the same with module parameters and meets the administrator's expectation.

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -0

index 1b0f3cd..7256ae4 100644

Quoted 21 lines · Weiping Pan

When we set primary slave with module parameters, bond will always use this primary slave as active slave.

But when we modify primary slave via sysfs, it will call bond_should_change_active() and take into account primary_reselect.

And I think we should use the new primary slave as the new active slave regardless of the value of primary_reselect.
Thus the behavior is the same with module parameters and meets the administrator's expectation.

Signed-off-by Weiping Pan<[email protected]>

---
  drivers/net/bonding/bond_sysfs.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)

drivers/net/bonding/bond_sysfs.c +0 -0

index 1b0f3cd..7256ae4 100644

drivers/net/bonding/bond_sysfs.c static ssize_t bonding_store_primary(struct device *d, · line 1083

Quoted 7 lines

drivers/net/bonding/bond_sysfs.c +1 -0

@@ -1077,6 +1077,7 @@ static ssize_t bonding_store_primary(struct device *d,
  				bond->dev->name, slave->dev->name);
  			bond->primary_slave = slave;
  			strcpy(bond->params.primary, slave->dev->name);
+			bond->force_primary = true;
  			bond_select_active_slave(bond);
  			goto out;
  		}

Not sure this is the right behavior. One may want to change the primary without causing a switch to this primary if another slave is already active, and setup primary_reselect to failure or better for that reason. The administrator still have the option to write into active_slave, to force the new active slave after changing the primary.

Arguably, this should be documented.

When we set primary slave with module parameters, bond will always use this primary slave as active slave.

But when we modify primary slave via sysfs, it will call bond_should_change_active() and take into account primary_reselect.

And I think we should use the new primary slave as the new active slave regardless of the value of primary_reselect, since primary slave really should have priority than other slaves.

primary_reselect is introduced to handle the failure or recovery of primary slave, but when we modify primary slave via sysfs, we want to give it higher priority, and it may or may not be a failure or recovery slave.

Thus the behavior is the same with module parameters and meets the administrator's expectation.

Quoted 9 lines · Weiping Pan

When we set primary slave with module parameters, bond will always use this primary slave as active slave.

But when we modify primary slave via sysfs, it will call bond_should_change_active() and take into account primary_reselect.

And I think we should use the new primary slave as the new active slave regardless of the value of primary_reselect, since primary slave really should have priority than other slaves.

The whole point of primary_reselect is that the primary slave
does not have priority unless it meets the reselect criteria, or it is being enslaved.

Quoted 6 lines

primary_reselect is introduced to handle the failure or recovery of primary slave, but when we modify primary slave via sysfs, we want to give it higher priority, and it may or may not be a failure or recovery slave.

Thus the behavior is the same with module parameters and meets the administrator's expectation.

I still disagree with this patch. My comments regarding the
prior version were intended to mean that we should document the current behavior, not change the behavior and document the new behavior.

If an administrator wishes for the newly set primary to
immediately become the active slave, they can either leave primary_reselect at its default setting or utilize the available mechanism to change the active slave. Applying this patch eliminates the ability to alter the primary slave setting without simultaneously changing the active slave.

Further, the default value for primary_reselect already does
this (change to the new primary immediately); this patch only affects the case that primary_reselect is set to a non-default value. In my mind, this reinforces that the current behavior is correct, and that the primary_reselect setting should apply to the newly selected primary (because the administrator has explicitly chosen that behavior).

	-J

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