[PATCH] CTDB protocol and client fixes

Amitay Isaacs amitay at gmail.com
Fri Nov 20 21:59:03 UTC 2015


On Fri, Nov 20, 2015 at 11:43 PM, Volker Lendecke <Volker.Lendecke at sernet.de
> wrote:

> On Fri, Nov 20, 2015 at 11:25:58AM +1100, Amitay Isaacs wrote:
> > The premise here is the "nested event loops are bad".  With single event
> > context nesting is definitely evil.  However, using multiple event
> contexts
> > also leads to nesting, which is not identified as nesting in the tevent
> > framework.
>
> The idea is that with what we call nested event contexts is
> that in tevent_loop_once only the one event source can fire.
>
> > Creating temporary event context in the sync api essentially creates
> nested
> > event loops.  If the code requires the creation of nested event contexts,
>
> Just to get a common understanding: When I talk about nested
> event loops I always mean multiple tevent_loop_once calls on
> the stack for the same event context.
>
> > then it's just bad code and it should be rewritten using async api to get
> > rid of the nested event loops.
>
> 100% ack. If we were there, we would not need the sync
> wrappers anymore.
>
> > Also, the abstractions in the new ctdb client code (pkt_read, comm) are
> > designed with single event context in mind and they will not work with
> > temporary contexts.  To support nested event loops,  I will have to
> > redesign those abstractions.
>
> Just picking a random sync wrapper sample from source3/libsmb:
>
>         if (smbXcli_conn_has_async_calls(cli->conn)) {
>                 /*
>                  * Can't use sync call while an async call
>                  * is in flight
>                  */
>                 status = NT_STATUS_INVALID_PARAMETER;
>                 goto fail;
>         }
>
> It's right that it is *very* hard to support both sync and
> async calls simultaneously. In source3/libsmb we took the
> path to just bail if this happens.
>
> The real problem in the ctdb protocol is async messages. You
> do a sync call on a nested event *context*, and an async
> message comes in. What do you do? We need some provision for
> that. But apart from this, I do not see why having nested
> event contexts is really a problem.
>

Exactly.  I have written the whole new client API to take care of the async
messages which can be received while in sync call.  This requires that you
use the same event context where the callbacks are registered correctly to
handle all types of packets that can be received.  Question is what do you
want to do in the callback functions for these async messages when using
sync API.  If you want to do simple things like setting flags or change
some state, then you are in luck since that's how the new ctdb tool
re-implementation using new client API looks like.  However, if you want to
do more messaging from the callbacks then you are doing it wrong.  You
should not be using sync API in the first place.

If you are trying to solve the problem of how to allow the simultaneous use
of sync and async API, then I say don't do it, that's a very bad idea.
Either use sync API or async API.  I don't want to support consumers of
client API using both sync and async versions simultaneously.  From your
example above, you are also trying to avoid the mix of sync and async API
simultaneously.

CTDB client API is not yet a library for others to consume.  Only consumer
will be samba (for now).  So we can stick to using async API.  The only
reason for providing sync API is to make code very easy for trivial
applications (e.g. pcp-pmda sensor or ctdb tool re-implementation).
Anything more complex requires async API (e.g. recovery helper and
re-implementation of test code).


>
> > I don't see any benefit of using temporary event contexts.  Can you
> > elaborate why using nested event loops is a good thing?
>
> Nested event *loops* on the same event *context* are evil,
> essentially that's pthreads without mutexes. Nested event
> *contexts* are okay, they are a good way to limit the events
> that can fire to just the ones we expect and silence
> everything else.
>
> What I find difficult to wrap my head around is that you have a single fd
as a connection to ctdbd.  You cannot separate events by creating multiple
contexts on that single fd.  Worst case is you might end up ignoring some
incoming async messages because you are not interested in processing them
at a nested event context.  Also, there is the question of re-registering
callbacks for nested contexts.  If you are registering the same callbacks
at the nested context level, then you are going to process all the async
messages even in the nested event context.  So why bother?  There is no
difference between top-level event context and a nested event context as
far as processing packets from ctdb is concerned.  In fact using nested
contexts is more expensive since you have to create a new context and
re-register the callbacks.

If you want to keep a separate event context exclusively for dealing with
ctdb messaging, then that's different and that's fine.  You might not want
to keep any smb processing separate from ctdb messaging.  Then I fully
agree that using nested contexts can be helpful.  However, I am not
convinced that using nested event contexts on a single fd gets you any
benefit.

Hope that helps,
>

It doesn't.  I am happy to accept the terminology and not argue over
semantics of nested event loops.  However, I still don't understand how
nested contexts offer any benefit for ctdb client API.

If you ignore the programming idioms and focus on the state transitions,
then it doesn't really matter how the events happen.  You have to take care
of all events.  Whether you consider all the states globally (using a
single event context) or partition them into groups based on types of
events (e.g. ctdb messaging), you still have to deal with the same state
transitions.


> Volker
>

Amitay.


More information about the samba-technical mailing list