Libguestfs - Jul 2023 - [libnbd PATCH 1/2] api: Tighten rules on completion.callback

On 7/14/2023 4:13 PM, Eric Blake wrote:
> On Fri, Jul 14, 2023 at 09:13:42AM +0200, Laszlo Ersek wrote:
>> On 7/13/23 21:29, Eric Blake wrote:
>>> The documentation has claimed since commit 6f4dcdab that any
>>> completion callback will be called exactly once; but this is not
>>> consistent with the code: if nbd_aio_* itself returns an error,
then
>>> nothing is queued and the user does not need to wait for a
completion
>>> callback to know how the command failed.  We could tweak the
generator
>>> to call completion.callback no matter what, but since the
>>> completion.free callback already serves that role, it's easier
to fix
>>> the documentation to match reality.  After all, one only needs
>>> completion status if an aio command returned success (if it
returned
>>> failure, we know that there is nothing that is going to complete
>>> later).
>>>
>>> However, there was one place where we indeed fail to call
>>> completion.callback, even though the corresponding aio call
returned
>>> success, which can strand a user that was depending on the callback
to
>>> know that the pending aio command failed after all.  That's
when a
>>> call to nbd_close() interrupts a connection while commands are in
>>> flight.  This problem appears to have been around even before
commit
>>> 52b9b492 (v0.9.8) when we finally settled on having .free callbacks
in
>>> the first place.
>>>
>>> Beef up the closure-lifetimes unit test to more robustly check the
>>> various conditions guaranteed by the updated documentation, and to
>>> expose the previous skip of a completion callback during nbd_close.
>>>
>>> In summary, the behavior we want (where sequence is important) is:
>>>
>>> - aio command fails:
>>> mid-command .callback: 0 calls
>>> completion .callback: 0 calls
>>> mid-command .free: 1 call
>>> completion .free: 1 call
>>>
>>> - aio command succeeds:
>>> mid-command .callback: 0, 1, or multiple calls
>>> completion .callback: 1 call
>>> mid-command .free: 1 call
>>> completion .free: 1 call
>>>
>>> Reported-by: Tage Johansson <tage.j.lists at posteo.net>
>>> Fixes: 6f4dcdab ("docs: Clarify how callbacks should handle
errors", v1.11.8)
>>> Signed-off-by: Eric Blake <eblake at redhat.com>
>>> ---
>>>   docs/libnbd.pod           | 26 ++++++++++++++++----------
>>>   lib/handle.c              |  3 +++
>>>   tests/closure-lifetimes.c | 14 ++++++++++++++
>>>   3 files changed, 33 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/docs/libnbd.pod b/docs/libnbd.pod
>>> index 72f74053..433479e6 100644
>>> --- a/docs/libnbd.pod
>>> +++ b/docs/libnbd.pod
>>> @@ -904,9 +904,12 @@ same nbd object, as it would cause deadlock.
>>>   =head2 Completion callbacks
>>>
>>>   All of the asychronous commands have an optional completion
callback
>>> -function that is used right before the command is marked complete,
>>> -after any mid-command callbacks have finished, and before any free
>>> -functions.
>>> +function that is used if the asynchronous command succeeded, right
>>> +before the command is marked complete, after any mid-command
callbacks
>>> +have finished, and before any free functions.  The completion
callback
>>> +is not reached if the asynchronous command itself fails, while
free
>>> +functions are reached regardless of the initial result of the
>>> +asynchronous command.
>>>
>>>   When the completion callback returns C<1>, the command is
>>>   automatically retired (there is no need to call
>> I agree with this approach (i.e., with adapting the documentation to
>> reality), but I find the language somewhat confusing. We have three
terms:
>>
>> - "asynchronous command succeeds"
>> - "command is marked complete"
>> - "command is retired"
>>
>> The last two are mostly interchangeable in my view, and are also *not*
>> confusing in the documentation. But the first term is confusing, IMO;
it
>> can easily be mistaken for meanings #2/#3. What we mean by #1 instead
is
>> the successful queueing (or submission) of the command. The text below
>> does say "queued", but the text above doesn't. So I'd
suggest replacing
>> term#1 above with "call to asynchronous API succeeds" or
"asynchronous
>> command is successfully submitted" or something like that.
>>
>> (Side comment: I'd kinda prefer an all-or-nothing approach for
async
>> APIs. If the API fails at once, it should not take ownership of
>> anything; i.e., it shouldn't call either completion or free
callbacks.
>> And if it succeeds, then it should take complete ownership. I'm not
>> suggesting to rework callbacks whole-sale for this, though.)
>>
>> With the language clarified a bit:
>>
>> Acked-by: Laszlo Ersek <lersek at redhat.com>
>>
>> Thanks
>> Laszlo
>>
> Here's what I squashed in, before pushing the series as
> dfa9473c..28134d9e
>
> diff --git i/docs/libnbd.pod w/docs/libnbd.pod
> index 433479e6..d8b22107 100644
> --- i/docs/libnbd.pod
> +++ w/docs/libnbd.pod
> @@ -171,8 +171,12 @@ There are several things to note here:
>
>   =item *
>
> -This only starts the command.  The command is still in flight when the
> -call returns.
> +This only starts the command.  The command is (usually) still in
> +flight when the call returns success, where you must rely on
> +subsequent API calls for learning the final command outcome and
> +trigger any remaining callbacks.  However, you must also be able to
> +handle the case where system load allows the state machine to advance
> +far enough to invoke callbacks before the asynchronous API returns.
>
>   =item *
>
> @@ -194,7 +198,10 @@ calls.  The cookie is unique (per libnbd handle) and
E<ge> 1.
>
>   You may register a function which is called when the command
>   completes, see L</Completion callbacks> below.  In this case we
have
> -specified a null completion callback.
> +specified a null completion callback.  If a completion callback is
> +specified, it will only be called if the asynchronous command was
> +sucessfully submitted (if the asynchronous API itself returns an

Should probably be "successfully" instead of "sucessfully".

> +error, there is nothing further to be completed).
>
>   =back
>
> @@ -897,19 +904,25 @@ asynchronous commands are retired.
>
>   =head2 Callbacks and locking
>
> -The callbacks are invoked at a point where the libnbd lock is held; as
> -such, it is unsafe for the callback to call any C<nbd_*> APIs on the
> -same nbd object, as it would cause deadlock.
> +The callbacks are invoked at a point where the libnbd lock is held,
> +typically during a call to C<nbd_aio_notify_read>,
> +C<nbd_aio_notify_write>, C<nbd_aio_poll>, or other call that
can
> +advance libnbd's state machine.  Depending on system load, it is even
> +possible for a callback to reached before completion of the

Shouldn't it be "to be reached" instead of "to reached"?


Best regards,

Tage

> +C<nbd_aio_*> call that specified the callback.  As such, it is
unsafe
> +for the callback to call any C<nbd_*> APIs on the same nbd object,
as
> +it would cause deadlock.
>
>   =head2 Completion callbacks
>
>   All of the asychronous commands have an optional completion callback
> -function that is used if the asynchronous command succeeded, right
> -before the command is marked complete, after any mid-command callbacks
> +function that is used if the call to the asynchronous API reports
> +success.  The completion callback is invoked when the submitted
> +command is eventually marked complete, after any mid-command callbacks
>   have finished, and before any free functions.  The completion callback
> -is not reached if the asynchronous command itself fails, while free
> -functions are reached regardless of the initial result of the
> -asynchronous command.
> +is not reached if the asynchronous API itself fails, while free
> +callbacks are reached regardless of the result of the initial
> +asynchronous API.
>
>   When the completion callback returns C<1>, the command is
>   automatically retired (there is no need to call
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0xBFBE172D49294052.asc
Type: application/pgp-keys
Size: 2432 bytes
Desc: OpenPGP public key
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20230716/dc118859/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 665 bytes
Desc: OpenPGP digital signature
URL:
<http://listman.redhat.com/archives/libguestfs/attachments/20230716/dc118859/attachment.sig>

On Sun, Jul 16, 2023 at 04:39:18PM +0000, Tage Johansson
wrote:
> > @@ -194,7 +198,10 @@ calls.  The cookie is unique (per libnbd handle)
and E<ge> 1.
> > 
> >   You may register a function which is called when the command
> >   completes, see L</Completion callbacks> below.  In this case
we have
> > -specified a null completion callback.
> > +specified a null completion callback.  If a completion callback is
> > +specified, it will only be called if the asynchronous command was
> > +sucessfully submitted (if the asynchronous API itself returns an
> 
> 
> Should probably be "successfully" instead of
"sucessfully".
> 
> 
> > +error, there is nothing further to be completed).
> > 
> >   =back
> > 
> > @@ -897,19 +904,25 @@ asynchronous commands are retired.
> > 
> >   =head2 Callbacks and locking
> > 
> > -The callbacks are invoked at a point where the libnbd lock is held;
as
> > -such, it is unsafe for the callback to call any C<nbd_*> APIs
on the
> > -same nbd object, as it would cause deadlock.
> > +The callbacks are invoked at a point where the libnbd lock is held,
> > +typically during a call to C<nbd_aio_notify_read>,
> > +C<nbd_aio_notify_write>, C<nbd_aio_poll>, or other call
that can
> > +advance libnbd's state machine.  Depending on system load, it is
even
> > +possible for a callback to reached before completion of the
> 
> 
> Shouldn't it be "to be reached" instead of "to
reached"?
Yep on both counts; will fix.  Thanks for a careful review.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org