Re: [PATCH] bonding: Don't allow mode change via sysfs with slaves present

["Nicolas de Pesloüan" <nicolas.2p.debian@gmail.com>]

Le 15/11/2011 21:47, Andy Gospodarek a écrit :
> Nicolas,
>
> I took a look at the ifenslave package for debian more closely and it
> actually looks like devices are enslaved last, after mode is set.  Can
> you please take a look at this package and confirm what I'm seeing in
> the 'pre-up' script.
>
> It appears to me that setup_master sets the mode and enslave_slaves is
> called after and enslaves the devices:
>
> # Option slaves deprecated, replaced by bond-slaves, but still supported
> # for backward compatibility.
> IF_BOND_SLAVES=${IF_BOND_SLAVES:-$IF_SLAVES}
>
> if [ "$IF_BOND_MASTER" ] ; then
>          BOND_MASTER="$IF_BOND_MASTER"
>          BOND_SLAVES="$IFACE"
> else
>          if [ "$IF_BOND_SLAVES" ] ; then
>                  BOND_MASTER="$IFACE"
>                  BOND_SLAVES="$IF_BOND_SLAVES"
>          fi
> fi
>
> # Exit if nothing to do...
> [ -z "$BOND_MASTER$BOND_SLAVES" ]&&  exit
>
> add_master
> early_setup_master
> setup_master
> enslave_slaves
> exit 0

Andy,

I'm really surprise by this extract. In the most up to date version of the ifenslave-2.6 package 
(1.1.0-19), the order is:

add_master
early_setup_master
enslave_slaves
setup_master

early_setup_master was created to be able to do things that absolutely need to be done before 
enslavement. (See the comment just before this function). The idea was to do every possible setup in 
setup_master, after enslavement, except those that need to be done in early_setup_master. So having 
enslave_slaves after setup_master instead of before is definitely a mistake. Some of the operations 
in setup_master must be done after enslavement, in particular selecting the primary slave.

In version 1.1.0-18 (change log below), I checked all the possible order constraints of the sysfs 
interface and totally reworked most of the setup code, putting everything in the right order to 
achieve consistent results.

	ifenslave-2.6 (1.1.0-18) experimental; urgency=low

	  * Apply patch from Nicolas de Pesloüan:

	    - Major change: Check and fix the order in which the configuration is
	      written into /sys files, to ensure reliable results, according to the
	      tests done in the kernel (in drivers/net/bonding/bond_sysfs.c).
	    - Ensure that master is properly brought down when changing a parameter
	      that require it to be down.
	    - Ensure the master is brought up at the end of the setup, in the case
	      where ifup won't bring it up because it is currently configuring a slave.
	    - Add support for some previously unsupported /sys files: ad_select,
	      num_grat_arp, num_unsol_na, primary_reselect and queue_id.
	    - Enhance the documentation (README.Debian), to describe all the currently
	      supported bond-* options.
	    - Many other changes in the documentation.
	    - Reverse the order of the arguments to most sysfs_* internal functions, for
	      better readability.

It was a hard work, because there really exist many such constraints. I fail to find enough time to 
insert the result of this work into Documentation/networking/bonding.txt, but still plan to do so, 
even if the result is documented in the script you looked at.

Of course, it is possible to change the scripts in ifenslave-2.6 package to arrange for one more 
constraint. For as far as I understand, this would cause the Debian kernel that introduce the change 
we discuss about and all the future Debian kernels to be flagged with 'Breaks: ifenslave-2.6 (<< 
1.1.0-20)'. I'm not really comfortable with this and the Debian kernel team need to be involved. I 
copied them.

All that being said, I really advocate for less constraints on the sysfs setup. This is not strictly 
related to sysfs setup. If we eventually add a NETLINK interface for all the things we can setup 
using sysfs, we will face the exact same problem.

I perfectly understand, as Veaceslav noted in another email that there are a lot of mode-specific 
initialization stuff that's present only in bond_enslave(), but I think this is what needs to be 
fixed... Those initialization stuff should be moved out of bond_enslave() and called at appropriate 
sime, from bond_enslave() and from other locations, in particular when changing mode.

	Nicolas.