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

[Jay Vosburgh <fubar@us.ibm.com>]

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