* [PATCH net 0/3] correct behavior when modify primary via sysfs
@ 2012-06-11 9:00 Weiping Pan
2012-06-11 9:00 ` [PATCH net 1/3] bonding:record primary when modify it " Weiping Pan
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Weiping Pan @ 2012-06-11 9:00 UTC (permalink / raw)
To: netdev
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-11 9:00 [PATCH net 0/3] correct behavior when modify primary via sysfs Weiping Pan
@ 2012-06-11 9:00 ` Weiping Pan
2012-06-11 19:38 ` Nicolas de Pesloüan
2012-06-11 9:00 ` [PATCH net 2/3] bonding:check mode when modify primary_reselect Weiping Pan
2012-06-11 9:00 ` [PATCH net 3/3] bonding:force to use primary slave Weiping Pan
2 siblings, 1 reply; 18+ messages in thread
From: Weiping Pan @ 2012-06-11 9:00 UTC (permalink / raw)
To: netdev
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 <wpan@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH net 2/3] bonding:check mode when modify primary_reselect
2012-06-11 9:00 [PATCH net 0/3] correct behavior when modify primary via sysfs Weiping Pan
2012-06-11 9:00 ` [PATCH net 1/3] bonding:record primary when modify it " Weiping Pan
@ 2012-06-11 9:00 ` Weiping Pan
2012-06-11 19:42 ` Nicolas de Pesloüan
2012-06-11 9:00 ` [PATCH net 3/3] bonding:force to use primary slave Weiping Pan
2 siblings, 1 reply; 18+ messages in thread
From: Weiping Pan @ 2012-06-11 9:00 UTC (permalink / raw)
To: netdev
Using a primary_reselect only makes sense in active backup, TLB or ALB modes.
Signed-off-by: Weiping Pan <wpan@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH net 3/3] bonding:force to use primary slave
2012-06-11 9:00 [PATCH net 0/3] correct behavior when modify primary via sysfs Weiping Pan
2012-06-11 9:00 ` [PATCH net 1/3] bonding:record primary when modify it " Weiping Pan
2012-06-11 9:00 ` [PATCH net 2/3] bonding:check mode when modify primary_reselect Weiping Pan
@ 2012-06-11 9:00 ` Weiping Pan
2012-06-11 19:49 ` Nicolas de Pesloüan
2 siblings, 1 reply; 18+ messages in thread
From: Weiping Pan @ 2012-06-11 9:00 UTC (permalink / raw)
To: netdev
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 <wpan@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-11 9:00 ` [PATCH net 1/3] bonding:record primary when modify it " Weiping Pan
@ 2012-06-11 19:38 ` Nicolas de Pesloüan
2012-06-11 20:48 ` Jay Vosburgh
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas de Pesloüan @ 2012-06-11 19:38 UTC (permalink / raw)
To: Weiping Pan; +Cc: netdev
Le 11/06/2012 11:00, Weiping Pan a écrit :
> 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<wpan@redhat.com>
> ---
> 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 <nicolas.2p.debian@free.fr>
Nicolas.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect
2012-06-11 9:00 ` [PATCH net 2/3] bonding:check mode when modify primary_reselect Weiping Pan
@ 2012-06-11 19:42 ` Nicolas de Pesloüan
2012-06-11 20:56 ` Jay Vosburgh
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas de Pesloüan @ 2012-06-11 19:42 UTC (permalink / raw)
To: Weiping Pan; +Cc: netdev
Le 11/06/2012 11:00, Weiping Pan a écrit :
> Using a primary_reselect only makes sense in active backup, TLB or ALB modes.
>
> Signed-off-by: Weiping Pan<wpan@redhat.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 3/3] bonding:force to use primary slave
2012-06-11 9:00 ` [PATCH net 3/3] bonding:force to use primary slave Weiping Pan
@ 2012-06-11 19:49 ` Nicolas de Pesloüan
2012-06-11 21:17 ` Jay Vosburgh
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas de Pesloüan @ 2012-06-11 19:49 UTC (permalink / raw)
To: Weiping Pan; +Cc: netdev
Le 11/06/2012 11:00, Weiping Pan a écrit :
> 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<wpan@redhat.com>
> ---
> 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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-11 19:38 ` Nicolas de Pesloüan
@ 2012-06-11 20:48 ` Jay Vosburgh
2012-06-12 3:38 ` Weiping Pan
2012-06-12 20:05 ` Nicolas de Pesloüan
0 siblings, 2 replies; 18+ messages in thread
From: Jay Vosburgh @ 2012-06-11 20:48 UTC (permalink / raw)
To: =?UTF-8?B?Tmljb2xhcyBkZSBQZXNsb8O8YW4=?=; +Cc: Weiping Pan, netdev
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 11/06/2012 11:00, Weiping Pan a écrit :
>> 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<wpan@redhat.com>
>> ---
>> 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 <nicolas.2p.debian@free.fr>
Agreed, except that I can go either way on the "typo" warning.
Signed-off-by: Jay Vosburgh <fubar@us.ibm.com>
-J
---
-Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect
2012-06-11 19:42 ` Nicolas de Pesloüan
@ 2012-06-11 20:56 ` Jay Vosburgh
2012-06-11 21:13 ` Nicolas de Pesloüan
0 siblings, 1 reply; 18+ messages in thread
From: Jay Vosburgh @ 2012-06-11 20:56 UTC (permalink / raw)
To: =?UTF-8?B?Tmljb2xhcyBkZSBQZXNsb8O8YW4=?=; +Cc: Weiping Pan, netdev
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 11/06/2012 11:00, Weiping Pan a écrit :
>> Using a primary_reselect only makes sense in active backup, TLB or ALB modes.
>>
>> Signed-off-by: Weiping Pan<wpan@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect
2012-06-11 20:56 ` Jay Vosburgh
@ 2012-06-11 21:13 ` Nicolas de Pesloüan
2012-06-11 21:28 ` Jay Vosburgh
0 siblings, 1 reply; 18+ messages in thread
From: Nicolas de Pesloüan @ 2012-06-11 21:13 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Weiping Pan, netdev
Le 11/06/2012 22:56, Jay Vosburgh a écrit :
> Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>
>> Le 11/06/2012 11:00, Weiping Pan a écrit :
>>> Using a primary_reselect only makes sense in active backup, TLB or ALB modes.
>>>
>>> Signed-off-by: Weiping Pan<wpan@redhat.com>
>>> ---
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 3/3] bonding:force to use primary slave
2012-06-11 19:49 ` Nicolas de Pesloüan
@ 2012-06-11 21:17 ` Jay Vosburgh
2012-06-12 3:35 ` [PATCH net V2] " Weiping Pan
0 siblings, 1 reply; 18+ messages in thread
From: Jay Vosburgh @ 2012-06-11 21:17 UTC (permalink / raw)
To: =?UTF-8?B?Tmljb2xhcyBkZSBQZXNsb8O8YW4=?=; +Cc: Weiping Pan, netdev
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 11/06/2012 11:00, Weiping Pan a écrit :
>> 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<wpan@redhat.com>
>> ---
>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 2/3] bonding:check mode when modify primary_reselect
2012-06-11 21:13 ` Nicolas de Pesloüan
@ 2012-06-11 21:28 ` Jay Vosburgh
0 siblings, 0 replies; 18+ messages in thread
From: Jay Vosburgh @ 2012-06-11 21:28 UTC (permalink / raw)
To: =?UTF-8?B?Tmljb2xhcyBkZSBQZXNsb8O8YW4=?=; +Cc: Weiping Pan, netdev
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>Le 11/06/2012 22:56, Jay Vosburgh a écrit :
>> Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
[...]
>>> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH net V2] bonding:force to use primary slave
2012-06-11 21:17 ` Jay Vosburgh
@ 2012-06-12 3:35 ` Weiping Pan
2012-06-12 5:00 ` Jay Vosburgh
0 siblings, 1 reply; 18+ messages in thread
From: Weiping Pan @ 2012-06-12 3:35 UTC (permalink / raw)
To: netdev; +Cc: nicolas.2p.debian, fubar
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 <wpan@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-11 20:48 ` Jay Vosburgh
@ 2012-06-12 3:38 ` Weiping Pan
2012-06-12 20:05 ` Nicolas de Pesloüan
1 sibling, 0 replies; 18+ messages in thread
From: Weiping Pan @ 2012-06-12 3:38 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: Nicolas de Pesloüan, Weiping Pan, netdev
On 06/12/2012 04:48 AM, Jay Vosburgh wrote:
> Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>
>> Le 11/06/2012 11:00, Weiping Pan a écrit :
>>> 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<wpan@redhat.com>
>>> ---
>>> 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<nicolas.2p.debian@free.fr>
> 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<fubar@us.ibm.com>
>
> -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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net V2] bonding:force to use primary slave
2012-06-12 3:35 ` [PATCH net V2] " Weiping Pan
@ 2012-06-12 5:00 ` Jay Vosburgh
2012-06-12 6:37 ` Weiping Pan
0 siblings, 1 reply; 18+ messages in thread
From: Jay Vosburgh @ 2012-06-12 5:00 UTC (permalink / raw)
To: Weiping Pan; +Cc: netdev, nicolas.2p.debian
Weiping Pan <wpan@redhat.com> 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net V2] bonding:force to use primary slave
2012-06-12 5:00 ` Jay Vosburgh
@ 2012-06-12 6:37 ` Weiping Pan
0 siblings, 0 replies; 18+ messages in thread
From: Weiping Pan @ 2012-06-12 6:37 UTC (permalink / raw)
To: Jay Vosburgh; +Cc: netdev, nicolas.2p.debian
On 06/12/2012 01:00 PM, Jay Vosburgh wrote:
> Weiping Pan<wpan@redhat.com> 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.
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-11 20:48 ` Jay Vosburgh
2012-06-12 3:38 ` Weiping Pan
@ 2012-06-12 20:05 ` Nicolas de Pesloüan
2012-06-12 22:24 ` David Miller
1 sibling, 1 reply; 18+ messages in thread
From: Nicolas de Pesloüan @ 2012-06-12 20:05 UTC (permalink / raw)
To: David Miller; +Cc: Jay Vosburgh, Weiping Pan, netdev
Le 11/06/2012 22:48, Jay Vosburgh a écrit :
> Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote:
>
>> Le 11/06/2012 11:00, Weiping Pan a écrit :
>>> 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<wpan@redhat.com>
>>> ---
>>> 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<nicolas.2p.debian@free.fr>
>
> Agreed, except that I can go either way on the "typo" warning.
>
> Signed-off-by: Jay Vosburgh<fubar@us.ibm.com>
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH net 1/3] bonding:record primary when modify it via sysfs
2012-06-12 20:05 ` Nicolas de Pesloüan
@ 2012-06-12 22:24 ` David Miller
0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2012-06-12 22:24 UTC (permalink / raw)
To: nicolas.2p.debian; +Cc: fubar, wpan, netdev
From: Nicolas de Pesloüan <nicolas.2p.debian@gmail.com>
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.
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2012-06-12 22:24 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-11 9:00 [PATCH net 0/3] correct behavior when modify primary via sysfs Weiping Pan
2012-06-11 9:00 ` [PATCH net 1/3] bonding:record primary when modify it " Weiping Pan
2012-06-11 19:38 ` Nicolas de Pesloüan
2012-06-11 20:48 ` Jay Vosburgh
2012-06-12 3:38 ` Weiping Pan
2012-06-12 20:05 ` Nicolas de Pesloüan
2012-06-12 22:24 ` David Miller
2012-06-11 9:00 ` [PATCH net 2/3] bonding:check mode when modify primary_reselect Weiping Pan
2012-06-11 19:42 ` Nicolas de Pesloüan
2012-06-11 20:56 ` Jay Vosburgh
2012-06-11 21:13 ` Nicolas de Pesloüan
2012-06-11 21:28 ` Jay Vosburgh
2012-06-11 9:00 ` [PATCH net 3/3] bonding:force to use primary slave Weiping Pan
2012-06-11 19:49 ` Nicolas de Pesloüan
2012-06-11 21:17 ` Jay Vosburgh
2012-06-12 3:35 ` [PATCH net V2] " Weiping Pan
2012-06-12 5:00 ` Jay Vosburgh
2012-06-12 6:37 ` Weiping Pan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.