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