[PATCH] CTDB protocol and client fixes

[Amitay Isaacs]

On Tue, Nov 24, 2015 at 6:45 PM, Volker Lendecke <Volker.Lendecke at sernet.de>
wrote:

> On Tue, Nov 24, 2015 at 06:04:40PM +1100, Amitay Isaacs wrote:
> > On Tue, Nov 24, 2015 at 5:48 PM, Volker Lendecke <
> Volker.Lendecke at sernet.de>
> > wrote:
> >
> > > On Tue, Nov 24, 2015 at 05:33:23PM +1100, Amitay Isaacs wrote:
> > > > Since no one has replied to this email, let me summarize.
> > > >
> > > > 1. For some reason I cannot fathom, async processing is treated
> > > *different*
> > > > from sync processing.  A global event context for all async
> processing is
> > > > ok, but for sync processing need lots of temporary contexts.
> > >
> > > When you use tevent_loop_once deep inside the call stack where some
> > > other tevent_loop_once is also active, anything can happen. As long as
> > > callers work without tevent_loop_allow_nesting then it might be
> workable.
> > >
> > > > 2. I can drop the tevent_context from sync API, but it will still
> use the
> > > > cached event context rather than creating a new one for every sync
> API
> > > > call.  This is because of the current design of the client API.  It
> was
> > > > designed to not use tevent_add_fd() for reading each individual
> packet
> > > from
> > > > an fd.
> > >
> > > "cached event context" is something that I am worried about using in
> > > smbd and winbind.  We have gone through a lot of work to remove this
> from
> > > the core smb libraries. You might prove me wrong by finding spots where
> > > we are not there yet, but in the SMB libraries I would see that as
> > > a bug that needs fixing.
> > >
> >
> > The cached event context arises only in sync API which will be primarily
> > used in the ctdb tool and some of the test code.  Samba would never use
> > CTDB's sync API, but (I am hoping) will be converted to use CTDB's async
> > client API.
> >
> > Questions are:
> >
> > 1. Should I keep the sync API completely separate, so it's not used by
> any
> > code other than CTDB.  As I mentioned before, the new client API is not a
> > public library yet.  When we make CTDB client API public, then it can
> only
> > publish the async version and not the sync version.
> >
> > 2. More fundamental question: Should I get rid of the optimization of not
> > using tevent_add_fd() for reading each individual packet from an fd?
> This
> > avoids the extra system call, but requires the use of same event context.
> > If no one else cares about it, I would be willing to get rid of it.
>
> If you have a message-listener always active, you might live
> with one tevent_fd always active. You then of course have
> the problem of dispatching to the right listener
> (message/correct control req), but with multiple active
> tevent_req's for controls you have that problem anyway.
> Sure, this lifts the problem up one level, but we keep it
> away from the base libraries.
>

In the client API (which is not very low level API), each control request
is tracked with unique reqid.  So there can be multiple tevent_reqs in
flight at the same time.  In fact, the parallel database recovery helper
relies on this to work correctly.  As you rightly identified, there is
always a message-listener active which uses the single tevent_fd to get all
the packets.

CTDB client API uses other abstractions (comm, read_pkt, write_pkt, reqid)
to implement the single tevent_fd based packet read and reqid based
de-multiplexer.


> If the sync APIs are only ever called inside ctdb tool or
> tests, why is the additional overhead a problem?
>

The problem is not of the overhead.  Since the API is designed around
single tevent_fd, you always need the same event context around.  Otherwise
you end up having to do tevent_add_fd() in each sync API.   This is even
trickier since the tevent_add_fd() is burried in a lower level abstraction
(comm).  That's why it's easier to implement sync API with single event
context.  And if I drop tevent_context from the API arguments, then I will
need to use the cached event context.

Amitay.