Eric Blake
2020-Mar-19 12:09 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On 3/18/20 8:21 PM, Eric Blake wrote:> In commit c306fa93ab and neighbors (v1.15.1), a concerted effort went > into caching the results of .can_FOO callbacks, with commit messages > demonstrating that a plugin with a slow callback should not have that > delay magnified multiple times. But nothing was added to the > testsuite at the time, and with the sh and eval plugins, we still have > two scenarios where we did not take advantage of the nbdkit cache > because we directly called things ourselves (one pre-existing since > v1.13.9, another introduced in v1.15.1 right after the cleanups). Fix > those spots by reworking the handle we pass to nbdkit, then enhance > test-eflags.sh to ensure we don't regress again. > > Note that nbdkit does not yet cache .thread_model; that will be > addressed in the next patch. Furthermore, we end up duplicating the > caching that nbdkit itself does, because it would be a layering > violation for us to have enough information to call into > backend_can_flush(). > > Fixes: 05c5eed6f2 > Fixes: 8490694d08 > Signed-off-by: Eric Blake <eblake@redhat.com> > ---> @@ -457,8 +473,14 @@ int > sh_can_flush (void *handle) > { > const char *method = "can_flush"; > - const char *script = get_script (method); > - return boolean_method (script, method, handle, 0); > + const char *script; > + struct sh_handle *h = handle; > + > + if (h->can_flush >= 0) > + return h->can_flush; > + > + script = get_script (method); > + return h->can_flush = boolean_method (script, method, handle, 0); > }Thinking about this more, maybe the real problem is that all language bindings that have to wrap scripts (OCaml and Rust are exceptions as they directly call into the C code; but lua, perl, python, ruby, tcl, and sh all fit into the category I'm describing) MUST define .can_FOO at the C level because they don't know a priori whether the script they will be loading will also provide a .can_FOO callback, so we have a lot of duplicated code in each language binding. Life might be easier if we change the C contract for the .can_FOO callbacks to support the special return value of -2 to behave the same as if .can_FOO were missing. In a way, it would make it easier for the remaining languages to behave more like sh's special MISSING return value (sh is an oddball because we can't even probe whether .FOO is missing so .can_FOO is mandatory in the script; but in eval, python, etc, it's easy to implement a C binding for .can_FOO that introspects the script). I'll spin up a v2 patch along those lines, so we can compare the difference. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Richard W.M. Jones
2020-Mar-19 19:08 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On Thu, Mar 19, 2020 at 07:09:32AM -0500, Eric Blake wrote:> Thinking about this more, maybe the real problem is that all > language bindings that have to wrap scripts (OCaml and Rust are > exceptions as they directly call into the C code; but lua, perl, > python, ruby, tcl, and sh all fit into the category I'm describing) > MUST define .can_FOO at the C level because they don't know a priori > whether the script they will be loading will also provide a .can_FOO > callback, so we have a lot of duplicated code in each language > binding. > > Life might be easier if we change the C contract for the .can_FOO > callbacks to support the special return value of -2 to behave the > same as if .can_FOO were missing. In a way, it would make it easier > for the remaining languages to behave more like sh's special MISSING > return value (sh is an oddball because we can't even probe whether > .FOO is missing so .can_FOO is mandatory in the script; but in eval, > python, etc, it's easy to implement a C binding for .can_FOO that > introspects the script). I'll spin up a v2 patch along those lines, > so we can compare the difference.What problem are we actually trying to solve here? If it calls the can_* functions in the plugin more than once it's not a big deal. Rich. -- 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
Eric Blake
2020-Mar-19 19:54 UTC
Re: [Libguestfs] [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On 3/19/20 2:08 PM, Richard W.M. Jones wrote:> On Thu, Mar 19, 2020 at 07:09:32AM -0500, Eric Blake wrote: >> Thinking about this more, maybe the real problem is that all >> language bindings that have to wrap scripts (OCaml and Rust are >> exceptions as they directly call into the C code; but lua, perl, >> python, ruby, tcl, and sh all fit into the category I'm describing) >> MUST define .can_FOO at the C level because they don't know a priori >> whether the script they will be loading will also provide a .can_FOO >> callback, so we have a lot of duplicated code in each language >> binding. >> >> Life might be easier if we change the C contract for the .can_FOO >> callbacks to support the special return value of -2 to behave the >> same as if .can_FOO were missing. In a way, it would make it easier >> for the remaining languages to behave more like sh's special MISSING >> return value (sh is an oddball because we can't even probe whether >> .FOO is missing so .can_FOO is mandatory in the script; but in eval, >> python, etc, it's easy to implement a C binding for .can_FOO that >> introspects the script). I'll spin up a v2 patch along those lines, >> so we can compare the difference. > > What problem are we actually trying to solve here? If it calls the > can_* functions in the plugin more than once it's not a big deal.Below is what I ended up with as a potential v2 after a bit of playing. It's smaller than the patch series we already reviewed, but it fails the test-eflags.sh additions (basically, it caused the sh plugin to always advertise FUA support when .can_fua was not implemented in the script, which ends up being different than the behavior for C plugins where FUA is advertised only when .flush exists). And anything else I can think of to fix that change in behavior is just going to add more code, compared to the ease of the fix of having the sh plugin do a bit of caching itself. So at this point, I'm ditching any further ideas to change plugins.c, and going ahead and committing the v1 patches as reviewed (with the change that I've now split the sh and python patches into two parts: handle refactoring with no semantic change vs. adding a cache variable into the handle). For the record, this is an additional idea I thought of, but considered too complex for the benefit: add a new callback .has_callback, with signature: bool has_callback(enum) Then, anywhere that nbdkit currently does 'if (p->pwrite == NULL)', it would instead check 'if (!p->has_callback(NBDKIT_CALLBACK_PWRITE))'. If .has_callback itself is missing, nbdkit defaults to mapping each enum value to the corresponding callback pointer in the C struct: a callback is absent only when it is NULL. But with language bindings, they provide an alternative .has_callback which can return false even when the C struct has a non-NULL pointer, to more accurately reflect the reality that even though the language binding has a wrapper function in place, the wrapper determined that the end-user script is lacking an override for that callback. Back to the idea I had for v2: diff --git i/docs/nbdkit-plugin.pod w/docs/nbdkit-plugin.pod index 494d77f8..9a22c47d 100644 --- i/docs/nbdkit-plugin.pod +++ w/docs/nbdkit-plugin.pod @@ -631,7 +631,11 @@ if C<.zero> is absent or C<.can_zero> returns false (in those cases, nbdkit fails all fast zero requests, as its fallback to C<.pwrite> is not inherently faster), otherwise false (since it cannot be determined in advance if the plugin's C<.zero> will properly honor the semantics -of C<NBDKIT_FLAG_FAST_ZERO>). +of C<NBDKIT_FLAG_FAST_ZERO>). As an aid to plugins implementing +bindings to another language, if this callback returns <-2>, then +nbdkit will perform the same fallback as if C<.can_fast_zero> were +missing, in order to provide a sane default based on the cached result +of C<.can_zero>. =head2 C<.can_extents> @@ -668,7 +672,11 @@ error message and return C<-1>. This callback is not required unless a plugin wants to specifically handle FUA requests. If omitted, nbdkit checks whether C<.flush> exists, and behaves as if this function returns C<NBDKIT_FUA_NONE> or -C<NBDKIT_FUA_EMULATE> as appropriate. +C<NBDKIT_FUA_EMULATE> as appropriate. As an aid to plugins +implementing bindings to another language, if this callback returns +<-2>, then nbdkit will perform the same fallback as if C<.can_fua> +were missing, in order to provide a sane default based on the cached +result of C<.can_flush>. =head2 C<.can_multi_conn> diff --git i/server/plugins.c w/server/plugins.c index 708e1cfa..17a8eebd 100644 --- i/server/plugins.c +++ w/server/plugins.c @@ -388,8 +388,11 @@ plugin_can_fast_zero (struct backend *b, void *handle) struct backend_plugin *p = container_of (b, struct backend_plugin, backend); int r; - if (p->plugin.can_fast_zero) - return normalize_bool (p->plugin.can_fast_zero (handle)); + if (p->plugin.can_fast_zero) { + r = p->plugin.can_fast_zero (handle); + if (r != -2) + return normalize_bool (r); + } /* Advertise support for fast zeroes if no .zero or .can_zero is * false: in those cases, we fail fast instead of using .pwrite. * This also works when v1 plugin has only ._zero_v1. @@ -423,9 +426,22 @@ plugin_can_fua (struct backend *b, void *handle) NBDKIT_FUA_NATIVE before we will pass the FUA flag on. */ if (p->plugin.can_fua) { r = p->plugin.can_fua (handle); - if (r > NBDKIT_FUA_EMULATE && p->plugin._api_version == 1) - r = NBDKIT_FUA_EMULATE; - return r; + switch (r) { + case NBDKIT_FUA_NATIVE: + if (p->plugin._api_version == 1) + return NBDKIT_FUA_EMULATE; + /* fallthrough */ + case NBDKIT_FUA_EMULATE: + case NBDKIT_FUA_NONE: + case -1: + return r; + case -2: + /* Plugin specifically asking for our default */ + break; + default: + /* Treat all remaining non-zero values like normalize_bool. */ + return NBDKIT_FUA_EMULATE; + } } /* We intend to call .flush even if .can_flush returns false. */ if (p->plugin.flush || p->plugin._flush_v1) diff --git i/plugins/python/python.c w/plugins/python/python.c index 72f37dd7..510127ed 100644 --- i/plugins/python/python.c +++ w/plugins/python/python.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2018 Red Hat Inc. + * Copyright (C) 2013-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -764,7 +764,8 @@ py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) } static int -boolean_callback (void *handle, const char *can_fn, const char *plain_fn) +boolean_callback (void *handle, const char *can_fn, const char *plain_fn, + int def) { PyObject *obj = handle; PyObject *fn; @@ -789,49 +790,49 @@ boolean_callback (void *handle, const char *can_fn, const char *plain_fn) else if (plain_fn && callback_defined (plain_fn, NULL)) return 1; else - return 0; + return def; } static int py_is_rotational (void *handle) { - return boolean_callback (handle, "is_rotational", NULL); + return boolean_callback (handle, "is_rotational", NULL, 0); } static int py_can_multi_conn (void *handle) { - return boolean_callback (handle, "can_multi_conn", NULL); + return boolean_callback (handle, "can_multi_conn", NULL, 0); } static int py_can_write (void *handle) { - return boolean_callback (handle, "can_write", "pwrite"); + return boolean_callback (handle, "can_write", "pwrite", 0); } static int py_can_flush (void *handle) { - return boolean_callback (handle, "can_flush", "flush"); + return boolean_callback (handle, "can_flush", "flush", 0); } static int py_can_trim (void *handle) { - return boolean_callback (handle, "can_trim", "trim"); + return boolean_callback (handle, "can_trim", "trim", 0); } static int py_can_zero (void *handle) { - return boolean_callback (handle, "can_zero", "zero"); + return boolean_callback (handle, "can_zero", "zero", 0); } static int py_can_fast_zero (void *handle) { - return boolean_callback (handle, "can_fast_zero", NULL); + return boolean_callback (handle, "can_fast_zero", NULL, -2); } static int @@ -853,13 +854,7 @@ py_can_fua (void *handle) Py_DECREF (r); return ret; } - /* No Python ‘can_fua’, but check if there's a Python ‘flush’ - * callback defined. (In C modules, nbdkit would do this). - */ - else if (callback_defined ("flush", NULL)) - return NBDKIT_FUA_EMULATE; - else - return NBDKIT_FUA_NONE; + return -2; } static int diff --git i/plugins/sh/methods.c w/plugins/sh/methods.c index 56e2d410..4d1c5329 100644 --- i/plugins/sh/methods.c +++ w/plugins/sh/methods.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2018-2019 Red Hat Inc. + * Copyright (C) 2018-2020 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -531,17 +531,7 @@ sh_can_fua (void *handle) return r; case MISSING: - /* Check if the plugin claims to support flush. */ - switch (sh_can_flush (handle)) { - case -1: - return -1; - case 0: - return NBDKIT_FUA_NONE; - case 1: - return NBDKIT_FUA_EMULATE; - default: - abort (); - } + return -2; /* Let nbdkit pick its default. */ case ERROR: return -1; @@ -610,16 +600,7 @@ sh_can_fast_zero (void *handle) { const char *method = "can_fast_zero"; const char *script = get_script (method); - int r = boolean_method (script, method, handle, 2); - if (r < 2) - return r; - /* We need to duplicate the logic of plugins.c: if can_fast_zero is - * missing, we advertise fast fail support when can_zero is false. - */ - r = sh_can_zero (handle); - if (r == -1) - return -1; - return !r; + return boolean_method (script, method, handle, -2); } int -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Seemingly Similar Threads
- Re: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
- [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
- [nbdkit PATCH v2 2/5] python: Expose can_zero callback
- [nbdkit PATCH 6/9] server: Cache per-connection can_FOO flags
- [PATCH nbdkit 2/3] sh: Switch nbdkit-sh-plugin to use API version 2.