Date:	Fri, 30 May 2008 00:15:45 -0700
From:	Jay Vosburgh <fubar@...ibm.com>
To:	Or Gerlitz <ogerlitz@...taire.com>
cc:	Jeff Garzik <jgarzik@...ox.com>, netdev@...r.kernel.org
Subject: Re: [PATCH RFC] net/bonding: announce fail-over for the active-backup mode

Or Gerlitz <ogerlitz@...taire.com> wrote:

>Jay Vosburgh wrote:
[...]
>> 	Your case is similar: you want to issue a notifier call during
>> an active-backup failover, so that notifier call will have to be made
>> holding RTNL and no other locks.
>>
>> 	I think the most maintainable way to do that is to convert the
>> remaining callers of bond_change_active_slave to hold the correct set of
>> locks, and then have bond_change_active_slave drop down to just RTNL at
>> the appropriate place to make the notifier call.  That may not be as
>> simple as it sounds, as it may open race windows.
>>
>Lets say that everyone calls bond_change_active_slave with the correct
>locks taken and the code that delivers the event, unlocks these two locks,
>call to the notifier chain through dev_set_xxx() and then locks them
>again. These locks were there in the first place to protect on something,
>so generally speaking I don't see why its allowed to unlock them for some
>window of time... is it some "best effort" compromise?

	Not exactly.  The unlock/lock dance doesn't release RTNL, so we
know that slaves cannot be added or removed (because that requires
RTNL).  Generally speaking, slaves can't have their up/down status
changed, either, since that would also acquire RTNL.

	However, the curr_slave_lock is still needed, particularly for
the alb mode.  All of the paths that can change the curr_active_slave
now hold RTNL (which is reasonable, as it's generally a pretty rare
event).  However, not all of the paths that inspect curr_active_slave
hold RTNL, so some places (alb mode in particular) need a mutex to keep
the curr_active_slave from changing during various activities, but
holding RTNL for those things is unreasonable (e.g., bond_alb_xmit would
need it for every packet).

	It's probably possible to do away with the curr_slave_lock
entirely (and just use the bond->lock).  That will still have some
windows like this, since there will be places that hold bond->lock for
read and have to temporarily upgrade it to a write lock (which I think
would be all of the places that now hold curr_slave_lock for write).

>Second, if it makes sense to have this window at time where the other two
>locks are not taken and only the RTNL one is taken. Is there any reason I
>can't take the approach of bond_alb_handle_active_change() which as you
>pointed out, releases the locks, delivers the event and take them again?
>is there something different between the possible calls under the
>active-backup mode vs the ALB mode that requires to do this deeper fix?

	I'm suggesting you do exactly that: release the locks, do your
stuff, then reacquire the locks.

	The concern for you is that there are still a few calls to
bond_change_active_slave that will never enter the ALB path, and those
calls to bond_change_active slave don't necessarily have the correct set
of locks at present.  

	The location of your new notifier call is such that there's no
way for it to know whether bond_change_active_slave was called with the
full set of locks (e.g., from one of the link monitors), or some other
set (as from bond_release_all), so some of the remaining calls to
bond_change_active_slave will likely need to be updated with regard to
locking.

	I took a look, and I think this is how it stands: I recently
refactored the active-backup ARP monitor, and I believe that all of its
calls are now correct; this would have been the difficult one to fix, so
you timed this just right.  The load-balance ARP monitor is still wrong,
but you don't care about that one since it isn't used for active-backup
mode.

	I think the only cases that will require fixing for you are the
bond_release and bond_release_all.

	For bond_release, there are two calls to change_active: one sets
it to NULL, and the second sets to to a slave.  The first of those calls
has nominally incorrect locking; the second has proper locking.
Changing the first call to mimic the methodology of the second call
should be pretty straightforward.  Alternately, if your notifier doesn't
want to log "change to nothing" events (i.e., put it inside the "if
(new_active)" block), then you could probably get away with not fixing
the first call.

	For bond_release_all, there's just one call, and it sets the
current slave to NULL (and then goes off and frees all of the slaves).
The same caveats from the bond_release case apply here.

	-J

---
	-Jay Vosburgh, IBM Linux Technology Center, fubar@...ibm.com
--
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