Libguestfs - Jul 2019 - Re: [PATCH libnbd 2/3] lib: Implement closure lifetimes.

On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> Previously closures had a crude flag which tells if they are
> persistent or transient.  Transient closures (flag = false) last for
> the lifetime of the currently called libnbd function.  Persistent
> closures had an indefinite lifetime which could last for as long as
> the handle.  In language bindings handling persistent closures was
> wasteful as we needed to register a "close callback" to free the
> closure when the handle is closed.  But if you had submitted thousands
> of asynchronous commands you would end up registering thousands of
> close callbacks.
Our mails are crossing; I had typed this up (so I'm sending now) even
though I already see that you've tweaked things in v2.  I've got two
threads of thoughts below.
> 
> (Note it is also possible for the library to call the callback with
> valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning it's
the
> last valid call.)
[1] In my previous reply, I assumed that the nbd_aio_FOO_callback
function would use this paradigm...

> +++ b/examples/strict-structured-reads.c
> @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv, size_t
count, uint64_t offset,
>  }
>  
>  static int
> -read_verify (void *opaque, int64_t cookie, int *error)
> +read_verify (int valid_flag, void *opaque, int64_t cookie, int *error)
>  {
>    struct data *data = opaque;
>    int ret = -1;
>  
> +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> +    return 0;
> +
[1] which in turn affected this code (calling free(data) on the first
VALID call rather than on the FREE call is fishy), but now that I've
actually read the rest of the patch...

> +++ b/generator/states-reply-simple.c
> @@ -64,7 +64,8 @@
>        int error = 0;
>  
>        assert (cmd->error == 0);
> -      if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data,
cmd->count,
> +      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
> +                           cmd->data, cmd->count,
>                             cmd->offset, LIBNBD_READ_DATA, &error)
== -1)
[2] Here, we know we are calling the read callback exactly once, so we
could pass DATA|FREE here.
>          cmd->error = error ? error : EPROTO;
>      }
> diff --git a/generator/states-reply-structured.c
b/generator/states-reply-structured.c
> index f60232e..2ef8d20 100644
> --- a/generator/states-reply-structured.c
> +++ b/generator/states-reply-structured.c
> @@ -298,7 +298,7 @@
>           * current error rather than any earlier one. If the callback
fails
>           * without setting errno, then use the server's error below.
>           */
> -        if (cmd->cb.fn.read (cmd->cb.fn_user_data,
> +        if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               cmd->data + (offset - cmd->offset),
[2] Here, we know if we will NOT call the read callback again if
NBD_REPLY_FLAG_DONE is set (so if it is set, we could pass VALID|FREE
here; but if it is not set, we still have to take care of passing FREE
later).
> @@ -499,7 +499,7 @@
>        /* Call the caller's extent function. */
>        int error = cmd->error;
>  
> -      if (cmd->cb.fn.extent (cmd->cb.fn_user_data,
> +      if (cmd->cb.fn.extent (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
>                               meta_context->name, cmd->offset,
>                               &h->bs_entries[1], (length-4) / 4,
&error) == -1)
[2] Same - if NBD_REPLY_FLAG_DONE is set, we could pass VALID|FREE here,
but if it is clear, we have to defer FREE to later.  Note that state
REPLY.STRUCTURED_REPLY.FINISH would be such a spot where we could call
the FREE flag. On the other hand, if we let that state always call with
FREE, it's easier to code that than it is to code it to call FREE only
if the earlier code for chunk/extents did not call VALID|FREE. So for
the structured reply spots marked [2] above, calling with just VALID and
then having FINISH do FREE unconditionally is worth considering (the
simple reply can still do VALID|FREE at once).  But if we do that...
>          if (cmd->error == 0)
> diff --git a/generator/states-reply.c b/generator/states-reply.c
> index 1a0c149..b11479c 100644
> --- a/generator/states-reply.c
> +++ b/generator/states-reply.c
> @@ -184,7 +184,8 @@ save_reply_state (struct nbd_handle *h)
>    if (cmd->cb.callback) {
>      int error = cmd->error;
>  
> -    if (cmd->cb.callback (cmd->cb.user_data, cookie, &error) ==
-1 && error)
> +    if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
> +                          cmd->cb.user_data, cookie, &error) == -1
&& error)
[1] ...we know that this is the only time we'll invoke this callback, so
we could have passed VALID|FREE here.  (And this part definitely means I
have to respin my proposal to let the callback return 1 to auto-retire).
>        cmd->error = error;
>    }
>  
> diff --git a/generator/states.c b/generator/states.c
> index 69aa431..374b8c5 100644
> --- a/generator/states.c
> +++ b/generator/states.c
> @@ -124,7 +124,8 @@ void abort_commands (struct nbd_handle *h,
>        int error = cmd->error ? cmd->error : ENOTCONN;
>  
>        assert (cmd->type != NBD_CMD_DISC);
> -      if (cmd->cb.callback (cmd->cb.user_data, cmd->cookie,
> +      if (cmd->cb.callback (LIBNBD_CALLBACK_VALID,
> +                            cmd->cb.user_data, cmd->cookie,
[1] another spot where we only call things once, and so we could pass
VALID|FREE.

> +++ b/lib/aio.c
> @@ -90,6 +90,17 @@ nbd_unlocked_aio_command_completed (struct nbd_handle
*h,
>    else
>      h->cmds_done = cmd->next;
>  
> +  /* Free the callbacks. */
> +  if (cmd->type != NBD_CMD_READ && cmd->cb.fn.extent)
> +    cmd->cb.fn.extent (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                       NULL, 0, NULL, 0, NULL);
> +  if (cmd->type == NBD_CMD_READ && cmd->cb.fn.read)
> +    cmd->cb.fn.read (LIBNBD_CALLBACK_FREE, cmd->cb.fn_user_data,
> +                     NULL, 0, 0, 0, NULL);
[2] ...these FREE calls could instead live in REPLY.STRUCTURED_REPLY.FINISH
> +  if (cmd->cb.callback)
> +    cmd->cb.callback (LIBNBD_CALLBACK_FREE, cmd->cb.user_data,
> +                      0, NULL);
[1] and this spot is useless for my auto-retire patch, hence my idea of
using VALID|FREE at both spots where we utilize the callback, rather
than here where we retire the callback.

I'll have to look at your v2, and my ideas for rearranging the callbacks
could still be done as a patch on top of your initial implementation,
rather than making you have to spin a v3, since our mails have crossed.
> +
>    free (cmd);
>  
>    /* If the command was successful, return true. */
> diff --git a/lib/debug.c b/lib/debug.c
> index 12c10f7..56a9455 100644
> --- a/lib/debug.c
> +++ b/lib/debug.c
> @@ -40,9 +40,12 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
>  
>  int
>  nbd_unlocked_set_debug_callback (struct nbd_handle *h,
> -                                 int (*debug_fn) (void *, const char *,
const char *),
> +                                 int (*debug_fn) (int, void *, const char
*, const char *),
>                                   void *data)
>  {
> +  if (h->debug_fn)
> +    /* ignore return value */
> +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL, NULL);
This frees the first debug callback when installing a second; but where
does nbd_close free the second?  Also, do we allow C code to pass NULL
to uninstall a handler? Should other languages be able to clear out a
callback?

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

On Wed, Jul 24, 2019 at 12:21:37PM -0500, Eric Blake
wrote:
> On 7/24/19 7:17 AM, Richard W.M. Jones wrote:
> > Previously closures had a crude flag which tells if they are
> > persistent or transient.  Transient closures (flag = false) last for
> > the lifetime of the currently called libnbd function.  Persistent
> > closures had an indefinite lifetime which could last for as long as
> > the handle.  In language bindings handling persistent closures was
> > wasteful as we needed to register a "close callback" to free
the
> > closure when the handle is closed.  But if you had submitted thousands
> > of asynchronous commands you would end up registering thousands of
> > close callbacks.
> 
> Our mails are crossing; I had typed this up (so I'm sending now) even
> though I already see that you've tweaked things in v2.  I've got
two
> threads of thoughts below.
> 
> > 
> > (Note it is also possible for the library to call the callback with
> > valid_flag == LIBNBD_CALLBACK_VALID|LIBNBD_CALLBACK_FREE, meaning
it's the
> > last valid call.)
> 
> [1] In my previous reply, I assumed that the nbd_aio_FOO_callback
> function would use this paradigm...
> 
> 
> > +++ b/examples/strict-structured-reads.c
> 
> > @@ -127,11 +131,14 @@ read_chunk (void *opaque, const void *bufv,
size_t count, uint64_t offset,
> >  }
> >  
> >  static int
> > -read_verify (void *opaque, int64_t cookie, int *error)
> > +read_verify (int valid_flag, void *opaque, int64_t cookie, int
*error)
> >  {
> >    struct data *data = opaque;
> >    int ret = -1;
> >  
> > +  if (!(valid_flag & LIBNBD_CALLBACK_VALID))
> > +    return 0;
> > +
> 
> [1] which in turn affected this code (calling free(data) on the first
> VALID call rather than on the FREE call is fishy), but now that I've
> actually read the rest of the patch...
This is fixed in v2, but ...
> > +++ b/generator/states-reply-simple.c
> > @@ -64,7 +64,8 @@
> >        int error = 0;
> >  
> >        assert (cmd->error == 0);
> > -      if (cmd->cb.fn.read (cmd->cb.fn_user_data, cmd->data,
cmd->count,
> > +      if (cmd->cb.fn.read (LIBNBD_CALLBACK_VALID,
cmd->cb.fn_user_data,
> > +                           cmd->data, cmd->count,
> >                             cmd->offset, LIBNBD_READ_DATA,
&error) == -1)
> 
> [2] Here, we know we are calling the read callback exactly once, so we
> could pass DATA|FREE here.
That was actually my first version of the patch.  However it runs into
the problem that we need to call the FREE version when a command is
aborted.  It's complicated to know if we called cb.fn.read already.
But thinking about this a bit more, maybe we should call VALID|FREE
here and set cmd->cb.fn.read = NULL afterwards.

[...]
> > diff --git a/lib/debug.c b/lib/debug.c
> > index 12c10f7..56a9455 100644
> > --- a/lib/debug.c
> > +++ b/lib/debug.c
> > @@ -40,9 +40,12 @@ nbd_unlocked_get_debug (struct nbd_handle *h)
> >  
> >  int
> >  nbd_unlocked_set_debug_callback (struct nbd_handle *h,
> > -                                 int (*debug_fn) (void *, const char
*, const char *),
> > +                                 int (*debug_fn) (int, void *, const
char *, const char *),
> >                                   void *data)
> >  {
> > +  if (h->debug_fn)
> > +    /* ignore return value */
> > +    h->debug_fn (LIBNBD_CALLBACK_FREE, h->debug_data, NULL,
NULL);
> 
> This frees the first debug callback when installing a second; but where
> does nbd_close free the second?  Also, do we allow C code to pass NULL
> to uninstall a handler? Should other languages be able to clear out a
> callback?
Yes nbd_close should check and call the callback again with FREE.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

On 7/24/19 12:48 PM, Richard W.M. Jones wrote:
>> [2] Here, we know we are calling the read callback exactly once, so we
>> could pass DATA|FREE here.
> 
> That was actually my first version of the patch.  However it runs into
> the problem that we need to call the FREE version when a command is
> aborted.  It's complicated to know if we called cb.fn.read already.
> But thinking about this a bit more, maybe we should call VALID|FREE
> here and set cmd->cb.fn.read = NULL afterwards.
Indeed, that's a nice trick; it also solves the question for structured
replies (if we pass VALID|FREE when we see NBD_REPLY_FLAG_DONE, then set
the callback to NULL, then we still need to manage calling plain FREE
later on when finally retiring the command, but only if the callback is
still set).  I'll fold that in to my followup, as having fewer callback
invocations is always worthwhile (after all, indirect functions got more
expensive thanks to Spectre), even if it is probably not the bottleneck
here.

>> This frees the first debug callback when installing a second; but where
>> does nbd_close free the second?  Also, do we allow C code to pass NULL
>> to uninstall a handler? Should other languages be able to clear out a
>> callback?
> 
> Yes nbd_close should check and call the callback again with FREE.
I still didn't see that in v2; but we'll get it fixed soon enough.

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