10:57 a.m.
On 3/1/21 9:53 AM, Richard W.M. Jones wrote:
So ... this is all complicated ... And reverts some changes which I
found simplified things.
Stepping back here, is it the case that next-ops is really a hack, and
what filters should actually get is some kind of connection object
(let's call it that, even if it doesn't exactly correspond to "struct
connection"), on which they can make general calls to the layer
beneath.
I'm almost thinking of calling it a 'context object', but yes, that's
the idea.
Then we allow filters to open and close new connection objects.
Yes.
How do filters get these connection objects? I would say that unlike
next-ops, filters should not get a connection object with each
callback. Instead in filter_open the filter should explicitly open a
new connection, and in filter_close explicitly close it. Then
filter_pread would make a pread() call on the connection object which
the filter has saved in the filter's handle.
My questions would be, assuming a design like this magically existed:
(1) Does it solve the problem for multi-conn filter?
(2) Does it solve the problem for background threads?
Right now, I'm still experimenting with design on top of this series
which will get a working background thread setup (with a
proof-of-concept for that using the ext2 filter), but here's my thoughts
so far:
A filter needs to know which backend it is (or which backend is next);
right now, we use 'void *nxdata' to encapsulate struct backend*, but
could make 'void*' more typesafe at the expense of a lot of mechanical
churn in the filters.
At any point in .after_fork(), a filter can call:
nbdkit_context *nbdkit_context_open (void *nxdata, int readonly, const
char *exportname);
to open up a new context visiting the appropriate backend stored in
nxdata (but leaving nxdata unchanged), along with a matching:
void nbdkit_context_close (nbdkit_context *context);
to close it. Most filters want 1:1 context per connection, which means
we want a way to associate a just-opened context into the usual handle
so that next_ops->FOO(nxdata) works as before. I'm thinking:
int nbdkit_context_associate (void *nxdata, nbdkit_context *context);
which returns 0 on success, -1 on failure, and updates nxdata so that
future calls to next_ops->FOO(nxdata) now operate on the context. The
existing:
filter_open (nbdkit_next_open *next_open, void *nxdata, ...)
where a filter calls next_open (nxdata, ...) remains as shorthand for
calling both nbdkit_context_open() and nbdkit_context_associate().
Filters like ext2 that want to share a single context among all
connections never call next_open() or nbdkit_context_associate(), but
instead need a way to operate on the single context opened during
.after_fork(); this could look like:
int nbdkit_context_next_ops (nbdkit_context *context, struct
nbdkit_next_ops **next_ops, void **nxdata);
which populates a filter-global next_ops and nxdata pointer so the
filter can later call shared_next_ops->FOO(shared_nxdata, ...) as
needed. Filters that do not call nbdkit_context_associate() (including
implicitly via .open's next_open) will be passed next_ops==NULL in their
other various .foo functions (to force them to call through their
shared_next_ops object obtained earlier).
The reopen filter currently calls next_ops->reopen() to first close out
the existing context and then attempt to reopen a new one in its place.
But it seems a bit more logical that we would get rid of
next_ops->reopen(), and instead have .reopen first attempt to use
nbdkit_context_open() to open a new context, and if successful, then
call nbdkit_context_close() on the old context, and
nbdkit_context_associate (nxdata) to associate the new context into place.
The multi-conn filter still needs to track a list of all open contexts,
in order to call list[n]->next_ops->flush(list[n]->nxdata) across each
context.
I also note that at present, the 'struct nbdkit_next_ops * next_ops' we
pass to all filters is constant (just a list of function pointers, no
per-connection state); all the per-connection details are shared between
a 'struct backend *' (which backend to call into, part of nxdata both
before and after this patch) and a 'struct handle *' (state of whether
we are safe, and what pointer to pass to that backend - stored in TLS
prior to this series, but explicitly in nxdata afterwards). The
combination of struct backend and struct handle is what forms an
nbdkit_context. And since the function pointers in next_ops are always
the same, we could change the signature for ALL filters to have just one
pointer instead of 2 as the parameter to all .foo callbacks, changing:
int filter_pread (struct nbdkit_next_ops *next_ops, void *nxdata,
void *handle, ...)
into:
int filter_pread (nbdkit_context *nxdata, void *handle, ...)
where nxdata is NULL if nothing was associated during .open, and when
non-NULL, the filter uses nxdata->pread(nxdata, ...) to call into the
next context. But that extra type-safety and change in signatures is
more churn across all the filters. It would also mean we don't need
nbdkit_context_next_ops() proposed above.
I hope to post some patches later this week (maybe today?) that
demonstrate where I'm headed.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org