Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 0/5] Counterproposal for python v2 interfaces
As mentioned in my reviews, I wonder if we should make our python callbacks look a bit more Pythonic by having kwargs added for each new flag that we want to expose. The idea was first floated here: https://www.redhat.com/archives/libguestfs/2018-April/msg00108.html Note that with my proposal, there is no need for a python script to expose a global API_VERSION variable; new flags are added using a Python API that is backwards-compatible. In fact, with introspection, we can even get away with the user not having to write a can_fast_zero; the mere presence of support for a fast=... kw parameter is enough to let us pick the correct default. However, there are aspects of Rich's proposal that I like better: for example, by having an explicit API_VERSION variable, we can change the interface of pread to read directly into a buffer, which my code can't do. And although mine produces interfaces that feel a bit more Pythonic, I also note that mine requires more lines of code (and that's without even implementing the cache/can_cache callback present in Rich's proposal). Thus, this is an RFC to see which pieces we like best out of the various proposals, rather than something that should be applied as-is. Eric Blake (5): python: Let zero's may_trim parameter be optional python: Expose can_zero callback python: Update internals to plugin API level 2 python: Expose FUA support python: Add can_fast_zero support plugins/python/nbdkit-python-plugin.pod | 122 ++++++++-- plugins/python/python.c | 286 ++++++++++++++++++++++-- plugins/python/example.py | 6 +- tests/test.py | 6 +- 4 files changed, 375 insertions(+), 45 deletions(-) -- 2.21.0
Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 1/5] python: Let zero's may_trim parameter be optional
In preparation for adding other optional flag arguments to the python bindings, start by making the existing 'may_trim' flag to 'zero' be optional. That is, the plugin need not define the parameter if it does not make any semantic difference (ie. if the plugin ignores the hint and never trims); while if the parameter exists, we now pass it as a keyword argument rather than as a positional argument. This requires the plugin to give the parameter a specific name, and works whether or not the plugin provides a default for the parameter (although we do now recommend that plugins provide a default value, as it makes plugins built against newer nbdkit more likely to run even on older nbdkit, even though that's not the direction we typically guarantee). We can't see through a plugin that used '**kwargs', but that is less likely. If we are super-worried about back-compatibility to older plugins which hard-coded 4 required parameters, we could also tweak the introspection to assume that a 'zero' with 4 parameters and no defaults, where we do not recognize the fourth parameter name, is an old-style plugin, and call it with may_trim passed via a positional rather than a keyword argument. However, I suspect most plugins just copied from our examples, which means they used the right parameter naming to just keep working; furthermore, while we have been clear that backwards API compatibility is essential for C, we have not made the same guarantee for language plugins. The introspection framework here will also make it easy to probe for future flag additions (support for 'fua' being the obvious candidate in the short term). This patch also fixes failure to check for (unlikely) errors during creation of 'args' within py_zero(). Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/python/nbdkit-python-plugin.pod | 35 ++++++++--- plugins/python/python.c | 78 +++++++++++++++++++++++-- plugins/python/example.py | 2 +- tests/test.py | 2 +- 4 files changed, 102 insertions(+), 15 deletions(-) diff --git a/plugins/python/nbdkit-python-plugin.pod b/plugins/python/nbdkit-python-plugin.pod index 3680fd65..f504cf7a 100644 --- a/plugins/python/nbdkit-python-plugin.pod +++ b/plugins/python/nbdkit-python-plugin.pod @@ -40,11 +40,24 @@ C<__main__> module): def pread(h, count, offset): # see below -Note that the subroutines must have those literal names (like C<open>), -because the C part looks up and calls those functions directly. You -may want to include documentation and globals (eg. for storing global -state). Any other top level statements are run when the script is -loaded, just like ordinary Python. +Note that the functions must have those literal names (like C<open>), +because the C part looks up and calls those functions directly. Where +this documentation lists a parameter as mandatory, you can name the +parameter what you would like (the C part sets that parameter +positionally); however, where this documentation lists a parameter +with a default value, you must either omit that parameter or use that +exact parameter name (the C part inspects the function signature to +see whether your callback implemented that parameter, and if so, sets +the parameter via keyword). In this way, newer versions of nbdkit can +add additional flags with default values that can be useful to newer +plugins, but still run older plugins without requiring them to be +rewritten to add support for the new flags. Note that using +C<**kwargs> in your function instead of named parameters would defeat +the C code that inspects the signature. + +You may want to include documentation and globals (eg. for storing +global state). Any other top level statements are run when the script +is loaded, just like ordinary Python. =head2 Python versions @@ -260,13 +273,17 @@ exception, optionally using C<nbdkit.set_error> first. (Optional) - def zero(h, count, offset, may_trim): + def zero(h, count, offset, may_trim=False): # no return value The body of your C<zero> function should ensure that C<count> bytes -of the disk, starting at C<offset>, will read back as zero. If -C<may_trim> is true, the operation may be optimized as a trim as long -as subsequent reads see zeroes. +of the disk, starting at C<offset>, will read back as zero. + +Your function may support an optional C<may_trim> parameter; if it is +present and the caller sets it to True, then your callback may +optimize the operation by using a trim, as long as subsequent reads +see zeroes. If you omit the optional parameter, or if the caller sets +it to False, writing zeroes should not punch holes. NBD only supports whole writes, so your function should try to write the whole region (perhaps requiring a loop). If the write diff --git a/plugins/python/python.c b/plugins/python/python.c index 7052aac0..d18bcb5e 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2018 Red Hat Inc. + * Copyright (C) 2013-2019 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -109,6 +109,43 @@ callback_defined (const char *name, PyObject **obj_rtn) return 1; } +/* Does a callback support the given keyword parameter? */ +static int +callback_has_parameter (PyObject *fn, const char *name) +{ + int r = 0; + PyObject *inspect, *pname, *spec, *params; + + assert (script != NULL); + assert (module != NULL); + + pname = PyUnicode_FromString ("inspect"); + if (!pname) + return -1; + inspect = PyImport_Import (pname); + Py_DECREF (pname); + + if (!inspect) + return -1; + + spec = PyObject_CallMethod (inspect, "signature", "O", fn); + Py_DECREF (inspect); + if (!spec) + return -1; + + /* We got the signature; now check for the requested keyword */ + params = PyObject_GetAttrString (spec, "parameters"); + if (!params) { + Py_DECREF (spec); + return -1; + } + r = PyMapping_HasKeyString (params, name); + + Py_DECREF (spec); + Py_DECREF (params); + return r; +} + /* Convert bytes/str/unicode into a string. Caller must free. */ static char * python_to_string (PyObject *str) @@ -564,16 +601,49 @@ py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) { PyObject *obj = handle; PyObject *fn; + PyObject *args; + PyObject *kwargs; PyObject *r; if (callback_defined ("zero", &fn)) { + static int zero_may_trim = -1; + + if (zero_may_trim < 0) + zero_may_trim = callback_has_parameter (fn, "may_trim"); + if (zero_may_trim < 0) { + check_python_failure ("zero"); + return -1; + } + PyErr_Clear (); last_error = 0; - r = PyObject_CallFunction (fn, "OiLO", - obj, count, offset, - may_trim ? Py_True : Py_False); + args = Py_BuildValue ("OiL", obj, count, offset); + if (!args) { + check_python_failure ("zero"); + Py_DECREF (fn); + return -1; + } + kwargs = PyDict_New (); + if (!kwargs) { + check_python_failure ("zero"); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + if (zero_may_trim && + PyDict_SetItemString (kwargs, "may_trim", + may_trim ? Py_True : Py_False) == -1) { + check_python_failure ("zero"); + Py_DECREF (kwargs); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); + Py_DECREF (args); + Py_DECREF (kwargs); if (last_error == EOPNOTSUPP || last_error == ENOTSUP) { /* When user requests this particular error, we want to * gracefully fall back, and to accommodate both a normal return diff --git a/plugins/python/example.py b/plugins/python/example.py index 60f9d7f0..9205f107 100644 --- a/plugins/python/example.py +++ b/plugins/python/example.py @@ -65,7 +65,7 @@ def pwrite(h, buf, offset): disk[offset:end] = buf -def zero(h, count, offset, may_trim): +def zero(h, count, offset, may_trim=False): global disk if may_trim: disk[offset:offset+count] = bytearray(count) diff --git a/tests/test.py b/tests/test.py index 9a2e947d..a83f1181 100644 --- a/tests/test.py +++ b/tests/test.py @@ -43,7 +43,7 @@ def pwrite(h, buf, offset): disk[offset:end] = buf -def zero(h, count, offset, may_trim=False): +def zero(h, count, offset): global disk disk[offset:offset+count] = bytearray(count) -- 2.21.0
Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 2/5] python: Expose can_zero callback
Fairly straightforward, as the code in plugins.c already takes care of necessary .pwrite fallbacks. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/python/nbdkit-python-plugin.pod | 8 +++++++- plugins/python/python.c | 7 +++++++ tests/test.py | 4 ++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/plugins/python/nbdkit-python-plugin.pod b/plugins/python/nbdkit-python-plugin.pod index f504cf7a..ec126f5a 100644 --- a/plugins/python/nbdkit-python-plugin.pod +++ b/plugins/python/nbdkit-python-plugin.pod @@ -208,6 +208,13 @@ contents will be garbage collected. def can_trim(h): # return a boolean +=item C<can_zero> + +(Optional) + + def can_zero(h): + # return a boolean + =item C<pread> (Required) @@ -317,7 +324,6 @@ C<config_help>, C<magic_config_key>, C<can_fua>, C<can_cache>, -C<can_zero>, C<can_fast_zero>, C<can_extents>, C<can_multi_conn>, diff --git a/plugins/python/python.c b/plugins/python/python.c index d18bcb5e..aa09dce7 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -718,6 +718,12 @@ py_can_trim (void *handle) return boolean_callback (handle, "can_trim", "trim"); } +static int +py_can_zero (void *handle) +{ + return boolean_callback (handle, "can_zero", "zero"); +} + #define py_config_help \ "script=<FILENAME> (required) The Python plugin to run.\n" \ "[other arguments may be used by the plugin that you load]" @@ -744,6 +750,7 @@ static struct nbdkit_plugin plugin = { .can_write = py_can_write, .can_flush = py_can_flush, .can_trim = py_can_trim, + .can_zero = py_can_zero, .pread = py_pread, .pwrite = py_pwrite, diff --git a/tests/test.py b/tests/test.py index a83f1181..1b7d3e0e 100644 --- a/tests/test.py +++ b/tests/test.py @@ -32,6 +32,10 @@ def can_trim(h): return True +def can_zero(h): + return True + + def pread(h, count, offset): global disk return disk[offset:offset+count] -- 2.21.0
Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 3/5] python: Update internals to plugin API level 2
Adjust internal functions in preparation for FUA support; although at this point, the default plugins.c can_fua implementation correctly reports python as needing emulation, and we can assert that we aren't seeing a FUA flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/python/python.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/plugins/python/python.c b/plugins/python/python.c index aa09dce7..1954881c 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -46,6 +46,8 @@ #define PY_SSIZE_T_CLEAN 1 #include <Python.h> +#define NBDKIT_API_VERSION 2 + #include <nbdkit-plugin.h> #include "cleanup.h" @@ -477,7 +479,7 @@ py_get_size (void *handle) static int py_pread (void *handle, void *buf, - uint32_t count, uint64_t offset) + uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; @@ -485,6 +487,7 @@ py_pread (void *handle, void *buf, Py_buffer view = {0}; int ret = -1; + assert (!flags); if (!callback_defined ("pread", &fn)) { nbdkit_error ("%s: missing callback: %s", script, "pread"); return ret; @@ -523,12 +526,13 @@ out: static int py_pwrite (void *handle, const void *buf, - uint32_t count, uint64_t offset) + uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; PyObject *r; + assert (!flags); if (callback_defined ("pwrite", &fn)) { PyErr_Clear (); @@ -549,12 +553,13 @@ py_pwrite (void *handle, const void *buf, } static int -py_flush (void *handle) +py_flush (void *handle, uint32_t flags) { PyObject *obj = handle; PyObject *fn; PyObject *r; + assert (!flags); if (callback_defined ("flush", &fn)) { PyErr_Clear (); @@ -573,12 +578,13 @@ py_flush (void *handle) } static int -py_trim (void *handle, uint32_t count, uint64_t offset) +py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; PyObject *r; + assert (!flags); if (callback_defined ("trim", &fn)) { PyErr_Clear (); @@ -597,14 +603,16 @@ py_trim (void *handle, uint32_t count, uint64_t offset) } static int -py_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) +py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; PyObject *args; PyObject *kwargs; PyObject *r; + int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0; + assert (!(flags & ~NBDKIT_FLAG_MAY_TRIM)); if (callback_defined ("zero", &fn)) { static int zero_may_trim = -1; -- 2.21.0
Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 4/5] python: Expose FUA support
Expose support for the FUA flag to pwrite, zero, and trim, as well as a can_fua callback, for use in python plugins. There are some slight semantic differences: the C plugin had to switch to a new API with a single uint32_t flags argument (so we don't have to keep adding new ABI when new flags are added), but in so doing, there is no way to probe whether a C plugin supports FUA flags, so the C .can_fua has to return a tri-state. But for python plugins, thanks to the fact that python functions have named and optional parameters, it's a lot cleaner to provide new flags as a bool option apiece, instead of requiring the python program to parse out bits from an int, and easy to introspect if FUA support is present, so can_fua only has to return a bool on whether to advertise FUA support to the client. One other thing that makes for some interesting C glue code: if at least one of the three callbacks supports FUA, then the C can_fua function claims native support, but in so doing, the python plugin must gracefully handle the fallback to flush for the remaining callbacks that did not support FUA, since nbdkit plugins.c won't do it. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/python/nbdkit-python-plugin.pod | 46 +++++++- plugins/python/python.c | 148 +++++++++++++++++++++--- plugins/python/example.py | 6 +- 3 files changed, 179 insertions(+), 21 deletions(-) diff --git a/plugins/python/nbdkit-python-plugin.pod b/plugins/python/nbdkit-python-plugin.pod index ec126f5a..6940f4d9 100644 --- a/plugins/python/nbdkit-python-plugin.pod +++ b/plugins/python/nbdkit-python-plugin.pod @@ -215,6 +215,22 @@ contents will be garbage collected. def can_zero(h): # return a boolean +=item C<can_fua> + +(Optional) + + def can_fua(h): + # return a boolean + +Unlike the C counterpart, the Python callback does not need a +tri-state return value, because Python introspection is sufficient to +learn whether callbacks support FUA. Thus, this function only returns +a bool, which merely controls whether Forced Unit Access (FUA) support +is advertised to the client on a per-connection basis. If omitted or +this returns true, FUA support is advertised as native if any of the +C<pwrite>, C<zero>, or C<trim> callbacks support an optional parameter +C<fua>; or as emulated if there is a C<flush> callback. + =item C<pread> (Required) @@ -239,7 +255,7 @@ C<nbdkit.set_error> first. (Optional) - def pwrite(h, buf, offset): + def pwrite(h, buf, offset, fua=False): length = len (buf) # no return value @@ -247,6 +263,12 @@ The body of your C<pwrite> function should write the buffer C<buf> to the disk. You should write C<count> bytes to the disk starting at C<offset>. +Your function may support an optional C<fua> parameter; if it is +present and the caller sets it to True, then your callback must not +return success until the write has landed in persistent storage. If +it is absent but C<can_fua> returned True, then nbdkit emulates a +client request for FUA by calling C<flush>. + NBD only supports whole writes, so your function should try to write the whole region (perhaps requiring a loop). If the write fails or is partial, your function should throw an exception, @@ -262,6 +284,11 @@ fails or is partial, your function should throw an exception, The body of your C<flush> function should do a L<sync(2)> or L<fdatasync(2)> or equivalent on the backing store. +This function will not be called directly by the client if +C<can_flush> returned false; however, it may still be called by nbdkit +if C<can_fua> returned True but C<pwrite>, C<zero>, or C<trim> lacked +a C<fua> parameter. + If the flush fails, your function should throw an exception, optionally using C<nbdkit.set_error> first. @@ -269,18 +296,24 @@ using C<nbdkit.set_error> first. (Optional) - def trim(h, count, offset): + def trim(h, count, offset, fua=False): # no return value The body of your C<trim> function should "punch a hole" in the backing store. If the trim fails, your function should throw an exception, optionally using C<nbdkit.set_error> first. +Your function may support an optional C<fua> parameter; if it is +present and the caller sets it to True, then your callback must not +return success until the trim has landed in persistent storage. If it +is absent but C<can_fua> returned True, then nbdkit emulates a client +request for FUA by calling C<flush>. + =item C<zero> (Optional) - def zero(h, count, offset, may_trim=False): + def zero(h, count, offset, may_trim=False, fua=False): # no return value The body of your C<zero> function should ensure that C<count> bytes @@ -292,6 +325,12 @@ optimize the operation by using a trim, as long as subsequent reads see zeroes. If you omit the optional parameter, or if the caller sets it to False, writing zeroes should not punch holes. +Your function may support an optional C<fua> parameter; if it is +present and the caller sets it to True, then your callback must not +return success until the write has landed in persistent storage. If +it is absent but C<can_fua> returned True, then nbdkit emulates a +client request for FUA by calling C<flush>. + NBD only supports whole writes, so your function should try to write the whole region (perhaps requiring a loop). If the write fails or is partial, your function should throw an exception, @@ -322,7 +361,6 @@ C<longname>, C<description>, C<config_help>, C<magic_config_key>, -C<can_fua>, C<can_cache>, C<can_fast_zero>, C<can_extents>, diff --git a/plugins/python/python.c b/plugins/python/python.c index 1954881c..677420bd 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -64,6 +64,9 @@ static const char *script; static PyObject *module; static int last_error; +static int pwrite_has_fua; +static int zero_has_fua; +static int trim_has_fua; static PyObject * set_error (PyObject *self, PyObject *args) @@ -368,6 +371,32 @@ py_config (const char *key, const char *value) "nbdkit requires these callbacks.", script); return -1; } + + /* One-time setup to learn which callbacks have a fua parameter */ + if (callback_defined ("pwrite", &fn)) { + pwrite_has_fua = callback_has_parameter (fn, "fua"); + Py_DECREF (fn); + if (pwrite_has_fua < 0) { + check_python_failure ("config"); + return -1; + } + } + if (callback_defined ("zero", &fn)) { + zero_has_fua = callback_has_parameter (fn, "fua"); + Py_DECREF (fn); + if (zero_has_fua < 0) { + check_python_failure ("config"); + return -1; + } + } + if (callback_defined ("trim", &fn)) { + trim_has_fua = callback_has_parameter (fn, "fua"); + Py_DECREF (fn); + if (trim_has_fua < 0) { + check_python_failure ("config"); + return -1; + } + } } else if (callback_defined ("config", &fn)) { /* Other parameters are passed to the Python .config callback. */ @@ -524,22 +553,52 @@ out: return ret; } +static int py_flush (void *handle, uint32_t flags); + static int py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; + PyObject *args; + PyObject *kwargs; PyObject *r; + int fua = (flags & NBDKIT_FLAG_FUA) != 0; + int need_flush = fua && !pwrite_has_fua; - assert (!flags); + assert (!(flags & ~NBDKIT_FLAG_FUA)); if (callback_defined ("pwrite", &fn)) { PyErr_Clear (); - r = PyObject_CallFunction (fn, "ONL", obj, + args = Py_BuildValue ("ONL", obj, PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ), offset); + if (!args) { + check_python_failure ("pwrite"); + Py_DECREF (fn); + return -1; + } + kwargs = PyDict_New (); + if (!kwargs) { + check_python_failure ("pwrite"); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + if (pwrite_has_fua && + PyDict_SetItemString (kwargs, "fua", + fua ? Py_True : Py_False) == -1) { + check_python_failure ("pwrite"); + Py_DECREF (kwargs); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); + Py_DECREF (args); + Py_DECREF (kwargs); if (check_python_failure ("pwrite") == -1) return -1; Py_DECREF (r); @@ -549,7 +608,7 @@ py_pwrite (void *handle, const void *buf, return -1; } - return 0; + return need_flush ? py_flush (handle, 0) : 0; } static int @@ -582,14 +641,42 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { PyObject *obj = handle; PyObject *fn; + PyObject *args; + PyObject *kwargs; PyObject *r; + int fua = (flags & NBDKIT_FLAG_FUA) != 0; + int need_flush = fua && !trim_has_fua; - assert (!flags); + assert (!(flags & ~NBDKIT_FLAG_FUA)); if (callback_defined ("trim", &fn)) { PyErr_Clear (); - r = PyObject_CallFunction (fn, "OiL", obj, count, offset); + args = Py_BuildValue ("OiL", obj, count, offset); + if (!args) { + check_python_failure ("trim"); + Py_DECREF (fn); + return -1; + } + kwargs = PyDict_New (); + if (!kwargs) { + check_python_failure ("trim"); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + if (trim_has_fua && + PyDict_SetItemString (kwargs, "fua", + fua ? Py_True : Py_False) == -1) { + check_python_failure ("trim"); + Py_DECREF (kwargs); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } + r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); + Py_DECREF (args); + Py_DECREF (kwargs); if (check_python_failure ("trim") == -1) return -1; Py_DECREF (r); @@ -599,7 +686,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) return -1; } - return 0; + return need_flush ? py_flush (handle, 0) : 0; } static int @@ -611,8 +698,10 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) PyObject *kwargs; PyObject *r; int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0; + int fua = (flags & NBDKIT_FLAG_FUA) != 0; + int need_flush = fua && !zero_has_fua; - assert (!(flags & ~NBDKIT_FLAG_MAY_TRIM)); + assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); if (callback_defined ("zero", &fn)) { static int zero_may_trim = -1; @@ -648,6 +737,15 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) Py_DECREF (fn); return -1; } + if (zero_has_fua && + PyDict_SetItemString (kwargs, "fua", + fua ? Py_True : Py_False) == -1) { + check_python_failure ("zero"); + Py_DECREF (kwargs); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); Py_DECREF (args); @@ -665,7 +763,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) if (check_python_failure ("zero") == -1) return -1; Py_DECREF (r); - return 0; + return need_flush ? py_flush (handle, 0) : 0; } nbdkit_debug ("zero missing, falling back to pwrite"); @@ -674,7 +772,8 @@ py_zero (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 missing) { PyObject *obj = handle; PyObject *fn; @@ -699,37 +798,55 @@ 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 missing; } 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_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_fua (void *handle) +{ + /* We have to convert a python bool into the nbdkit tristate. */ + switch (boolean_callback (handle, "can_fua", NULL, 1)) { + case -1: + return -1; + case 0: + return NBDKIT_FUA_NONE; + default: + if (pwrite_has_fua || zero_has_fua || trim_has_fua) + return NBDKIT_FUA_NATIVE; + if (callback_defined ("flush", NULL)) + return NBDKIT_FUA_EMULATE; + return NBDKIT_FUA_NONE; + } } #define py_config_help \ @@ -759,6 +876,7 @@ static struct nbdkit_plugin plugin = { .can_flush = py_can_flush, .can_trim = py_can_trim, .can_zero = py_can_zero, + .can_fua = py_can_fua, .pread = py_pread, .pwrite = py_pwrite, diff --git a/plugins/python/example.py b/plugins/python/example.py index 9205f107..ca7c2661 100644 --- a/plugins/python/example.py +++ b/plugins/python/example.py @@ -59,13 +59,15 @@ def pread(h, count, offset): return disk[offset:offset+count] -def pwrite(h, buf, offset): +# Since everything is stored in memory, flush is a no-op; this example +# does not provide a flush(), but honors the fua flag by doing nothing. +def pwrite(h, buf, offset, fua=False): global disk end = offset + len(buf) disk[offset:end] = buf -def zero(h, count, offset, may_trim=False): +def zero(h, count, offset, may_trim=False, fua=False): global disk if may_trim: disk[offset:offset+count] = bytearray(count) -- 2.21.0
Eric Blake
2019-Nov-25 22:48 UTC
[Libguestfs] [nbdkit PATCH 5/5] python: Add can_fast_zero support
Add the can_fast_zero callback, and yet another bool keyword parameter to the zero callback. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/python/nbdkit-python-plugin.pod | 35 ++++++++++++++----- plugins/python/python.c | 45 ++++++++++++++++++++++++- plugins/python/example.py | 2 +- 3 files changed, 71 insertions(+), 11 deletions(-) diff --git a/plugins/python/nbdkit-python-plugin.pod b/plugins/python/nbdkit-python-plugin.pod index 6940f4d9..7b0051af 100644 --- a/plugins/python/nbdkit-python-plugin.pod +++ b/plugins/python/nbdkit-python-plugin.pod @@ -215,6 +215,18 @@ contents will be garbage collected. def can_zero(h): # return a boolean + +=item C<can_fast_zero> + +(Optional) + + def can_fast_zero(h): + # return a boolean + +Unlike the C counterpart, this can often be omitted: the Python +callback uses Python introspection to default to true if the C<zero> +callback supports an optional parameter C<fast>. + =item C<can_fua> (Optional) @@ -313,7 +325,7 @@ request for FUA by calling C<flush>. (Optional) - def zero(h, count, offset, may_trim=False, fua=False): + def zero(h, count, offset, may_trim=False, fua=False, fast=False): # no return value The body of your C<zero> function should ensure that C<count> bytes @@ -331,13 +343,19 @@ return success until the write has landed in persistent storage. If it is absent but C<can_fua> returned True, then nbdkit emulates a client request for FUA by calling C<flush>. -NBD only supports whole writes, so your function should try to -write the whole region (perhaps requiring a loop). If the write -fails or is partial, your function should throw an exception, -optionally using C<nbdkit.set_error> first. In particular, if -you would like to automatically fall back to C<pwrite> (perhaps -because there is nothing to optimize if C<may_trim> is false), -use C<nbdkit.set_error(errno.EOPNOTSUPP)>. +Your function may support an optional C<fast> parameter; if it is +present and the caller sets it to True, then your callback must return +a failure if it is not any faster than a corresponding C<pwrite>. If +it is absent but C<can_fast_zero> returned True, then nbdkit treats +all fast zero requests as failures. + +NBD only supports whole writes, so your function should try to write +the whole region (perhaps requiring a loop). If the write fails or is +partial, your function should throw an exception, optionally using +C<nbdkit.set_error> first. In particular, if you would like to +automatically fall back to C<pwrite> (perhaps because there is nothing +to optimize if C<may_trim> is false) or declare that fast zero is not +possible, use C<nbdkit.set_error(errno.EOPNOTSUPP)>. =back @@ -362,7 +380,6 @@ C<description>, C<config_help>, C<magic_config_key>, C<can_cache>, -C<can_fast_zero>, C<can_extents>, C<can_multi_conn>, C<cache>, diff --git a/plugins/python/python.c b/plugins/python/python.c index 677420bd..fee339f4 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -699,11 +699,14 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) PyObject *r; int may_trim = (flags & NBDKIT_FLAG_MAY_TRIM) != 0; int fua = (flags & NBDKIT_FLAG_FUA) != 0; + int fast = (flags & NBDKIT_FLAG_FAST_ZERO) != 0; int need_flush = fua && !zero_has_fua; - assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); + assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA | + NBDKIT_FLAG_FAST_ZERO))); if (callback_defined ("zero", &fn)) { static int zero_may_trim = -1; + static int zero_fast = -1; if (zero_may_trim < 0) zero_may_trim = callback_has_parameter (fn, "may_trim"); @@ -711,6 +714,18 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) check_python_failure ("zero"); return -1; } + if (zero_fast < 0) + zero_fast = callback_has_parameter (fn, "fast"); + if (zero_fast < 0) { + check_python_failure ("zero"); + return -1; + } + + if (fast && zero_fast != 1) { + nbdkit_debug ("zero lacks fast support, failing fast request"); + nbdkit_set_error (EOPNOTSUPP); + return -1; + } PyErr_Clear (); @@ -746,6 +761,15 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) Py_DECREF (fn); return -1; } + if (zero_fast && + PyDict_SetItemString (kwargs, "fast", + fast ? Py_True : Py_False) == -1) { + check_python_failure ("zero"); + Py_DECREF (kwargs); + Py_DECREF (args); + Py_DECREF (fn); + return -1; + } r = PyObject_Call (fn, args, kwargs); Py_DECREF (fn); Py_DECREF (args); @@ -831,6 +855,24 @@ py_can_zero (void *handle) return boolean_callback (handle, "can_zero", "zero", 0); } +static int +py_can_fast_zero (void *handle) +{ + int r = boolean_callback (handle, "can_fast_zero", NULL, 2); + PyObject *fn; + + if (r == 2) { + /* can_fast_zero was missing, but we still want to default to true + * if the zero callback is missing or supports optional fast argument. + */ + if (callback_defined ("zero", &fn)) + r = callback_has_parameter (fn, "fast"); + else + r = 1; + } + return r; +} + static int py_can_fua (void *handle) { @@ -876,6 +918,7 @@ static struct nbdkit_plugin plugin = { .can_flush = py_can_flush, .can_trim = py_can_trim, .can_zero = py_can_zero, + .can_fast_zero = py_can_fast_zero, .can_fua = py_can_fua, .pread = py_pread, diff --git a/plugins/python/example.py b/plugins/python/example.py index ca7c2661..b1a9fed1 100644 --- a/plugins/python/example.py +++ b/plugins/python/example.py @@ -67,7 +67,7 @@ def pwrite(h, buf, offset, fua=False): disk[offset:end] = buf -def zero(h, count, offset, may_trim=False, fua=False): +def zero(h, count, offset, may_trim=False, fua=False, fast=False): global disk if may_trim: disk[offset:offset+count] = bytearray(count) -- 2.21.0
Richard W.M. Jones
2019-Nov-26 19:41 UTC
Re: [Libguestfs] [nbdkit PATCH 0/5] Counterproposal for python v2 interfaces
Thanks for posting this. Some inconclusive comments in no particular order: I don't have enough experience to judge whether or not kwargs are more Pythonic. My impression is that bitmasks are used in the library and so are kwargs, and kwargs are used more often, and bitmasks seem to be used mainly in lowlevel functions where the Python code is talking to C. About API_VERSION, I think we will need that in order to implement pread into a buf, unless we implement both pread() (returning buf) and a second call preadinto(..., buf, ...). In my current patch series I only implemented pread(..., buf, ...) so API_VERSION is definitely required by mine.>From the virt-v2v point of view, performance is the most importantfactor for us. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Reasonably Related Threads
- [nbdkit PATCH v2 0/5] FUA support in Python scripts
- [PATCH nbdkit 0/8] Implement nbdkit API v2 for Python plugins.
- [PATCH nbdkit v2 00/10] Implement nbdkit API v2 for Python plugins.
- [PATCH nbdkit v2 0/7] Implement nbdkit API v2 for Python plugins.
- [PATCH nbdkit v3 0/7] Implement nbdkit API v2 for Python plugins.