Message-ID: <20111116220200.GF25132@gospo.rdu.redhat.com>
Date:	Wed, 16 Nov 2011 17:02:00 -0500
From:	Andy Gospodarek <andy@...yhouse.net>
To:	Nicolas de Pesloüan 
	<nicolas.2p.debian@...il.com>
Cc:	Andy Gospodarek <andy@...yhouse.net>,
	Veaceslav Falico <vfalico@...hat.com>, netdev@...r.kernel.org,
	Jay Vosburgh <fubar@...ibm.com>, debian-kernel@...ts.debian.org
Subject: Re: [PATCH] bonding: Don't allow mode change via sysfs with slaves
	present

On Wed, Nov 16, 2011 at 01:02:21PM +0100, Nicolas de Pesloüan wrote:
> 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 was looking at ifenslave 1.1.0-20.  If you look at Debian bug #641250
you will see a very similar report to what prompted Veaceslav to come up
with this patch and post it here.

ifenslave-2.6 (1.1.0-20) unstable; urgency=low

  * Use dashes consistently for bonding options in README.Debian.
    Closes: #639244
  * Enslave slaves only after fully setting up the master. Closes: #641250
  * Add build-arch and build-indep targets to debian/rules.

 -- Guus Sliepen <guus@...ian.org>  Mon, 14 Nov 2011 11:36:21 +0100

ifenslave-2.6 (1.1.0-19) unstable; urgency=low

  * Don't bother trying to move configuration files anymore. This is not an
    issue anymore in for the next stable release, and it was broken anyway.
    Closes: #626959
  * Bump Standards-Version.

 -- Guus Sliepen <guus@...ian.org>  Wed, 25 May 2011 18:42:32 +0200

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.

  * Upload to experimental due to the freeze.

 -- Guus Sliepen <guus@...ian.org>  Tue, 21 Dec 2010 12:46:04 +0100

> 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.
>

I think Veaceslav is working on this, but there is significant
re-organization that is needed to make it work properly and make sure it
is tested.  I could be wrong about how long it will take him, but to
test it properly it will take some time.

Since this problem seems like a pretty major problem and now Debian,
Fedora, RHEL, and Ubuntu all seem to have proper initialization scripts
to handle it, I stand behind my original ACK.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists