[Libguestfs] [PATCH nbdkit v2 6/9] retry-request: Allow get_size operation to be retried

[Richard W.M. Jones]

On Mon, Jul 31, 2023 at 09:18:10AM -0500, Eric Blake wrote:
> On Fri, Jul 28, 2023 at 06:17:50PM +0100, Richard W.M. Jones wrote:
> > This plugin operation might need to do some real work (instead of just
> > fetching a number from memory), and so it might have to be retried.
> > 
> > In particular, changes to the curl plugin make .get_size into a
> > heavyweight operation, where previously it was done as a side-effect
> > of .open.  And so we must allow .get_size to be retried independent of
> > .open.
> > ---
> >  filters/retry-request/retry-request.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> 
> It feels a bit odd that .open isn't calling .get_size to cache it, but
> it's hard to state whether that's a bug in the filter (for not
> pre-caching .get_size), in the plugin (for not having .get_size ready
> to go by the end of .open), in whatever other filter is stacked on top
> of retry-request and not caching .get_size during its .open, or not a
> bug at all.  Thus, while I'm slightly worried that this patch may be
> papering over something instead of addressing root cause, I can't
> pinpoint anything in specific where we might be going against our
> documentation, and I can live with this going in.

The common code that starts protocol negotiation calls backend_open,
then backend_prepare, then backend_get_size:

https://gitlab.com/nbdkit/nbdkit/-/blob/4c527063336ccf14d286ef7db5766369e1b23845/server/protocol-handshake.c#L75

So from the point of view of a filter (but *not* from the point of
view of nbdkit), .open and .get_size are separate operations, and
.open doesn't imply .get_size has been called.

backend_get_size caches the result in the backend handle, so multiple
successful calls to backend_get_size will be ignored.  Failures are
not cached, so they will get retried.

This does imply that at some point in the future we might need to also
retry .prepare & .finalize in the retry-request filter, but that
doesn't affect us now.

> > 
> > diff --git a/filters/retry-request/retry-request.c b/filters/retry-request/retry-request.c
> > index e5b8344cd..8e3dd8246 100644
> > --- a/filters/retry-request/retry-request.c
> > +++ b/filters/retry-request/retry-request.c
> > @@ -141,6 +141,18 @@ retry_request_open (nbdkit_next_open *next, nbdkit_context *nxdata,
> >    return r == 0 ? NBDKIT_HANDLE_NOT_NEEDED : NULL;
> >  }
> >  
> > +static int64_t
> > +retry_request_get_size (nbdkit_next *next, void *handle)
> > +{
> > +  int64_t r;
> > +  int *err = &errno;          /* used by the RETRY_* macros */
> 
> This is the reason why I'm reluctant to say whether this is the right
> approach - if .get_size() is encountered during a .pread, there is no
> guarantee that we pass a correct errno value back if the pread fails
> because .get_size failed.  Ensuring that .get_size is cached during
> .open guarantees that we have a valid size for all subsequent
> operations, without needing to worry about what happens to errno.

Yes, I believe the error is lost in this case, which is
unfortunate.

How about a patch to enhance the RETRY_START macro so that it prints
*err (decoded) here.  At least it would ensure that the error is never
lost:

https://gitlab.com/nbdkit/nbdkit/-/blob/4c527063336ccf14d286ef7db5766369e1b23845/filters/retry-request/retry-request.c#L110

Rich.

> > +
> > +  RETRY_START("get_size")
> > +    r = next->get_size (next);
> > +  RETRY_END;
> > +  return r;
> > +}
> > +
> >  static int
> >  retry_request_pread (nbdkit_next *next,
> >                       void *handle, void *buf, uint32_t count, uint64_t offset,
> > @@ -267,6 +279,7 @@ static struct nbdkit_filter filter = {
> >    .config            = retry_request_config,
> >    .config_help       = retry_request_config_help,
> >    .open              = retry_request_open,
> > +  .get_size          = retry_request_get_size,
> >    .pread             = retry_request_pread,
> >    .pwrite            = retry_request_pwrite,
> >    .trim              = retry_request_trim,
> > -- 
> > 2.41.0
> > 
> > _______________________________________________
> > Libguestfs mailing list
> > Libguestfs at redhat.com
> > https://listman.redhat.com/mailman/listinfo/libguestfs
> > 
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top