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@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 _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
