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
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(-)
@@ -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);
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(-)
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,if(!rtnl_trylock())returnrestart_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;+gotoout;+}+new_value=bond_parse_parm(buf,pri_reselect_tbl);if(new_value<0){pr_err("%s: Ignoring invalid primary_reselect value %.*s.\n",
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(-)
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(-)
@@ -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.
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(-)
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,if(!rtnl_trylock())returnrestart_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;+gotoout;+}+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.
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(-)
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.
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(-)
@@ -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
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(-)
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,if(!rtnl_trylock())returnrestart_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;+gotoout;+}+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
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(-)
@@ -1123,6 +1123,13 @@ static ssize_t bonding_store_primary_reselect(struct device *d,if(!rtnl_trylock())returnrestart_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;+gotoout;+}+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
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(-)
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
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
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(-)
@@ -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
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(-)
@@ -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
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
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
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(-)
@@ -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.
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.