[Date Prev][Date Next] [Thread Prev][Thread Next] [Date Index] [Thread Index]

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



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.





Reply to: