[PATCH net 0/3] correct behavior when modify primary via sysfs

STALE5201d

18 messages, 5 authors, 2012-06-12 · open the first message on its own page

[PATCH net 0/3] correct behavior when modify primary via sysfs

From: Weiping Pan <hidden>
Date: 2012-06-11 08:55:48

There is a problem that 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.

Weiping Pan (3):
  bonding:record primary when modify it via sysfs
  bonding:check mode when modify primary_reselect
  bonding:force to use primary slave

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

-- 
1.7.4

[PATCH net 1/3] bonding:record primary when modify it via sysfs

From: Weiping Pan <hidden>
Date: 2012-06-11 08:55:48

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 <redacted>
---
 drivers/net/bonding/bond_sysfs.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index aef42f0..485bedb 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
 		}
 	}
 
-	pr_info("%s: Unable to set %.*s as primary slave.\n",
-		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);
-- 
1.7.4

[PATCH net 2/3] bonding:check mode when modify primary_reselect

From: Weiping Pan <hidden>
Date: 2012-06-11 08:55:49

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

Signed-off-by: Weiping Pan <redacted>
---
 drivers/net/bonding/bond_sysfs.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 485bedb..1b0f3cd 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
 	if (!rtnl_trylock())
 		return restart_syscall();
 
+	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",
-- 
1.7.4

[PATCH net 3/3] bonding:force to use primary slave

From: Weiping Pan <hidden>
Date: 2012-06-11 08:55:50

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 <redacted>
---
 drivers/net/bonding/bond_sysfs.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 1b0f3cd..7256ae4 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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;
 		}
-- 
1.7.4

Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs

From: Nicolas de Pesloüan <hidden>
Date: 2012-06-11 19:37:39

Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted hunk
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<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index aef42f0..485bedb 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
  		}
  	}

-	pr_info("%s: Unable to set %.*s as primary slave.\n",
-		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 ?

 > +	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 <redacted>

	Nicolas.

Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect

From: Nicolas de Pesloüan <hidden>
Date: 2012-06-11 19:41:23

Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted hunk
Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by: Weiping Pan<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 485bedb..1b0f3cd 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
  	if (!rtnl_trylock())
  		return restart_syscall();

+	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.

	Nicolas.

Re: [PATCH net 3/3] bonding:force to use primary slave

From: Nicolas de Pesloüan <hidden>
Date: 2012-06-11 19:48:53

Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted hunk
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<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 1b0f3cd..7256ae4 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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.

	Nicolas.

Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs

From: Jay Vosburgh <hidden>
Date: 2012-06-11 20:50:15

Nicolas de Pesloüan 	[off-list ref] wrote:
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
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<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    8 ++++++--
  1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index aef42f0..485bedb 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
  		}
  	}

-	pr_info("%s: Unable to set %.*s as primary slave.\n",
-		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
+	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 <redacted>
	Agreed, except that I can go either way on the "typo" warning.

Signed-off-by: Jay Vosburgh <redacted>

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect

From: Jay Vosburgh <hidden>
Date: 2012-06-11 20:56:58

Nicolas de Pesloüan 	[off-list ref] wrote:
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by: Weiping Pan<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    7 +++++++
  1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 485bedb..1b0f3cd 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
  	if (!rtnl_trylock())
  		return restart_syscall();

+	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.
	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.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect

From: Nicolas de Pesloüan <hidden>
Date: 2012-06-11 21:12:34

Le 11/06/2012 22:56, Jay Vosburgh a écrit :
Nicolas de Pesloüan 	[off-list ref]  wrote:
quoted
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
Using a primary_reselect only makes sense in active backup, TLB or ALB modes.

Signed-off-by: Weiping Pan<redacted>
---
   drivers/net/bonding/bond_sysfs.c |    7 +++++++
   1 files changed, 7 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 485bedb..1b0f3cd 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,
   	if (!rtnl_trylock())
   		return restart_syscall();

+	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.
	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?

	Nicolas

Re: [PATCH net 3/3] bonding:force to use primary slave

From: Jay Vosburgh <hidden>
Date: 2012-06-11 21:17:34

Nicolas de Pesloüan 	[off-list ref] wrote:
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
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<redacted>
---
  drivers/net/bonding/bond_sysfs.c |    1 +
  1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 1b0f3cd..7256ae4 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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.
	I suspect it is obeying the documented behavior, at least for
the behaviors that are documented.

	The documentation already says that "When initially enslaved,
the primary slave is always made the active slave."  That's probably
what covers the "module param" case, because the options are all set
prior to any slaves being added, so when the primary slave is later
enslaved, it is made the active slave immediately.

	Now, the documentation for the primary option itself also says:

	"The specified device will always be the active slave while it
	is available.  Only when the primary is off-line will alternate
	devices be used."

	Which is the old behavior, prior to primary_reselect being added
(although it is still the default behavior).  This should probably
change to reflect the actual behavior, i.e., that changing the primary
option in real time results in a reselection of the primary according to
the policy specified by primary_reselect.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect

From: Jay Vosburgh <hidden>
Date: 2012-06-11 21:29:28

Nicolas de Pesloüan [off-list ref] wrote:
Le 11/06/2012 22:56, Jay Vosburgh a écrit :
quoted
Nicolas de Pesloüan 	[off-list ref]  wrote:
[...]
quoted
quoted
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?
	Yes.  As long as the setting takes effect when it should, I see
no problem with permitting options that are currently not applicable to
be changed at any time.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

[PATCH net V2] bonding:force to use primary slave

From: Weiping Pan <hidden>
Date: 2012-06-12 03:30:27

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.

Changelog:
V2:modify document

Signed-off-by: Weiping Pan <redacted>
---
 Documentation/networking/bonding.txt |    8 ++++++--
 drivers/net/bonding/bond_sysfs.c     |    1 +
 2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt
index bfea8a3..9130050 100644
--- a/Documentation/networking/bonding.txt
+++ b/Documentation/networking/bonding.txt
@@ -652,7 +652,8 @@ primary
 
 	A string (eth0, eth2, etc) specifying which slave is the
 	primary device.  The specified device will always be the
-	active slave while it is available.  Only when the primary is
+	active slave while it is available.  Changing it via sysfs will make
+	it to be used as active slave immediately.  Only when the primary is
 	off-line will alternate devices be used.  This is useful when
 	one slave is preferred over another, e.g., when one slave has
 	higher throughput than another.
@@ -684,7 +685,7 @@ primary_reselect
 		The primary slave becomes the active slave only if the
 		current active slave fails and the primary slave is up.
 
-	The primary_reselect setting is ignored in two cases:
+	The primary_reselect setting is ignored in three cases:
 
 		If no slaves are active, the first slave to recover is
 		made the active slave.
@@ -692,6 +693,9 @@ primary_reselect
 		When initially enslaved, the primary slave is always made
 		the active slave.
 
+		When changing primary slave via sysfs, and if the primary
+		slave is up, bonding will use it as active slave immediately.
+
 	Changing the primary_reselect policy via sysfs will cause an
 	immediate selection of the best active slave according to the new
 	policy.  This may or may not result in a change of the active
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 1b0f3cd..7256ae4 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -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;
 		}
-- 
1.7.4

Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs

From: Weiping Pan <hidden>
Date: 2012-06-12 03:33:21

On 06/12/2012 04:48 AM, Jay Vosburgh wrote:
Nicolas de Pesloüan 	[off-list ref]  wrote:
quoted
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
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<redacted>
---
   drivers/net/bonding/bond_sysfs.c |    8 ++++++--
   1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index aef42f0..485bedb 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
   		}
   	}

-	pr_info("%s: Unable to set %.*s as primary slave.\n",
-		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
+	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<redacted>
	Agreed, except that I can go either way on the "typo" warning.
I prefer not to add 'typo' since I think the log is enough.

thanks
Weiping Pan
Signed-off-by: Jay Vosburgh<redacted>

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com

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

Re: [PATCH net V2] bonding:force to use primary slave

From: Jay Vosburgh <hidden>
Date: 2012-06-12 05:01:17

Weiping Pan [off-list ref] wrote:
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.
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, fubar@us.ibm.com

Re: [PATCH net V2] bonding:force to use primary slave

From: Weiping Pan <hidden>
Date: 2012-06-12 06:32:52

On 06/12/2012 01:00 PM, Jay Vosburgh wrote:
Weiping Pan[off-list ref]  wrote:
quoted
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
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.
Yes, this side effect is not good.

Thanks for your comments.

Weiping Pan
	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, fubar@us.ibm.com

Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs

From: Nicolas de Pesloüan <hidden>
Date: 2012-06-12 20:04:45

Le 11/06/2012 22:48, Jay Vosburgh a écrit :
Nicolas de Pesloüan 	[off-list ref]  wrote:
quoted
Le 11/06/2012 11:00, Weiping Pan a écrit :
quoted
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<redacted>
---
   drivers/net/bonding/bond_sysfs.c |    8 ++++++--
   1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index aef42f0..485bedb 100644
--- a/drivers/net/bonding/bond_sysfs.c
+++ b/drivers/net/bonding/bond_sysfs.c
@@ -1082,8 +1082,12 @@ static ssize_t bonding_store_primary(struct device *d,
   		}
   	}

-	pr_info("%s: Unable to set %.*s as primary slave.\n",
-		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
+	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<redacted>
	Agreed, except that I can go either way on the "typo" warning.

Signed-off-by: Jay Vosburgh<redacted>
David,

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.

Thanks,

	Nicolas.

Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs

From: David Miller <davem@davemloft.net>
Date: 2012-06-12 22:24:29

From: Nicolas de Pesloüan <redacted>
Date: Tue, 12 Jun 2012 22:05:37 +0200
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.
Thanks for bringing this to my attention.

Applied and pushed out.
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help