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

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

[Weiping Pan]

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

[Weiping Pan]

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

Signed-off-by: Weiping Pan <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

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

[Nicolas de Pesloüan]

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.

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

[Jay Vosburgh]

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

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

[Weiping Pan]

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

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

[Nicolas de Pesloüan]

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.

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

[David Miller]

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.

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

[Weiping Pan]

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

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

[Nicolas de Pesloüan]

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.

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

[Jay Vosburgh]

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

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

[Nicolas de Pesloüan]

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

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

[Jay Vosburgh]

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

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

[Weiping Pan]

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

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

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

Signed-off-by: Weiping Pan <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

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

[Nicolas de Pesloüan]

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.

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

[Jay Vosburgh]

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

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

[Weiping Pan]

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

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

And I think we should use the new primary slave as the new active slave
regardless of the value of primary_reselect, since primary slave really should
have priority than other slaves.
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

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

[Jay Vosburgh]

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

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

[Weiping Pan]

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
>