1:34 p.m.
On 7/17/19 8:40 AM, Eric Blake wrote:
It may be possible in rare situations that the libnbd state machine can
send() the command AND see data ready to recv() (perhaps from a previous
command still in flight), where it ends up read()ing until blocking and
happens to get the server's reply to this command and fire off the
callback, all prior to the nbd_aio_pread_callback() returning. If that
ever happens, then buffers[i].rcookie will still be unset at the time
the callback fires...
> +/* This callback is called from libnbd when any read command finishes. */
> +static int
> +finished_read (void *vp, int64_t rcookie, int *error)
> +{
> + size_t i;
> +
> + if (gssrc == NULL)
> + return 0;
> +
> + DEBUG (gssrc, "finished_read: read completed");
> +
> + /* Find the corresponding buffer and mark it as completed. */
> + for (i = 0; i < nr_buffers; ++i) {
> + if (buffers[i].rcookie == rcookie)
> + goto found;
> + }
> + /* This should never happen. */
> + abort ();
...such that we can hit this abort(). (Same for the write callback).
But I don't know how likely that scenario is in practice, and don't see
it as a reason to defer committing this patch as-is. I don't know if
there is any way to rework the state machine to guarantee that a
completion callback will not fire during the same aio_FOO_callback that
registered the callback (that is, to guarantee that h->lock will be
dropped and must be re-obtained prior to firing off a callback), but
it's worth thinking about.
An idea we had on IRC: instead of having nbd_aio_pread_callback which
both creates the cookie and submits the command with callback, we could
instead have: nbd_aio_pread(...) creates a command and returns a cookie
but does NOT send it; then nbd_aio_submit(nbd, cookie, callback, opaque)
which can be used to submit ANY nbd_aio_FOO command based on its cookie.
That solves the race for the callback ever being fired before the user
has had a chance to track the cookie, and makes it so that only one
command has to register callback functions (rather than one counterpart
per aio_FOO command).
Or maybe even three parts:
nbd_aio_pread_structured() => creates cookie, called once
nbd_aio_add_callback(cookie, callback, user_data) - called 0 or more
times before submit; callbacks run in LIFO order at command completion
nbd_aio_submit(cookie) => kicks off the command
Why do it this way? Because in the Python bindings, right now we have to
malloc() a C structure that tracks the Python Callable associated with
any Closure, and currently don't free that C struct until nbd_close.
Basically, we have:
nbd_internal_py_aio_pread_structured
data = malloc()
nbd_aio_pread_structured(, chunk_wrapper, data)
nbd_add_close_callback(data_free, data)
nbd_internal_py_aio_pread_structured_callback
data = malloc()
nbd_aio_pread_structured_callback(, chunk_wrapper, callback_wrapper, data)
nbd_add_close_callback(data_free, data)
where there is no convenient way for the generator to insert cleanup for
the data malloc'd in aio_pread_structured (for
aio_pread_structured_callback, it could use the callback_wrapper - but
handling the two APIs differently requires hairy generator refactoring).
But with a three-part split, we could make the generator produce:
nbd_internal_py_aio_pread_structured
data1 = malloc()
cookie = nbd_aio_pread_structured(, chunk_wrapper, data1)
nbd_aio_add_callback(cookie, data_free, data1)
nbd_internal_py_aio_add_callback
data2 = malloc()
nbd_aio_add_callback(cookie, data_free, data2)
nbd_aio_add_callback(cookie, callback_wrapper, data2)
nbd_internal_py_aio_submit
nbd_aio_submit(cookie)
where, during the course of the command being in flight, we call
chunk_wrapper(data1) as many times as needed, then when the command
completes, we call the callback wrappers in reverse order:
callback_wrapper(data2)
data_free(data2)
data_free(data1)
and thus avoid the need to use nbd_add_close_callback. We'd have to
tweak the Closure element to take an enum of dispositions instead of its
current bool persistent (use stack storage, dispose via
add_close_callback, dispose via aio_add_callback after returned cookie
obtained from underlying API, dispose via aio_add_callback using
incoming cookie argument prior to underlying API), but that still seems
easier than cross-linking the code for Python aio_pread_structured to
call the C aio_pread_structured_callback under the hood, and offers
enough separation that the generator does not have to be aware of
cross-API ties.
It may also simplify the generator in that we'd be back to never having
more than one Closure per function (compared to our current two
functions in nbd_aio_pread_structured_callback and
nbd_aio_block_status_callback), where handling a Closure in the
generator doesn't need List.iter.
The other thing this example pointed out was that in a single-threaded
main loop, the callback function can't retire a command itself, but it
is hairy to wire up anything else to retire the command and nothing
further is to be learned by retiring it. One idea was to change the
callback semantics: if a callback returns 1, the command is
auto-retired; if it returns 0 or -1, you still have to use
nbd_aio_command_completed to retire the command (works with the existing
API when there is at most one callback, but gets trickier with my above
split of allowing more than one LIFO callback addition: if the first
callback returns 1, but a later callback returns -1, does the command
still need to be auto-retired?). Or, instead of making the callback
return value special, we could add a flag to the proposed
nbd_aio_submit(cookie, bool) where the bool determines if the command
should be auto-retired.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org