With the current thread model this doesn't matter since nbdkit itself essentially enforces something like the GIL. But if we want to parallelize the Python plugin we must actually start using the GIL right. This is both complex and very poorly documented (including a lot of plainly incorrect information online). However I believe what I have written here is right, and it appears to work. --- plugins/python/python.c | 45 +++++++++++++++++++++++++++++++++++++++-- 1 file changed, 43 insertions(+), 2 deletions(-) diff --git a/plugins/python/python.c b/plugins/python/python.c index 9fa89964..229a46f1 100644 --- a/plugins/python/python.c +++ b/plugins/python/python.c @@ -51,6 +51,19 @@ #include "cleanup.h" +/* All callbacks that want to call any Py* function should use this + * macro. See + * https://docs.python.org/3/c-api/init.html#non-python-created-threads + */ +#define ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE \ + __attribute__((cleanup (cleanup_release))) \ + PyGILState_STATE gstate = PyGILState_Ensure() +static inline void +cleanup_release (PyGILState_STATE *gstateptr) +{ + PyGILState_Release (*gstateptr); +} + /* XXX Apparently global state is technically wrong in Python 3, see: * * https://www.python.org/dev/peps/pep-3121/ @@ -304,23 +317,30 @@ create_nbdkit_module (void) return m; } +static PyThreadState *tstate; + static void py_load (void) { PyImport_AppendInittab ("nbdkit", create_nbdkit_module); Py_Initialize (); + tstate = PyEval_SaveThread (); } static void py_unload (void) { - Py_XDECREF (module); - Py_Finalize (); + if (tstate) { + PyEval_RestoreThread (tstate); + Py_XDECREF (module); + Py_Finalize (); + } } static void py_dump_plugin (void) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; PyObject *fn; PyObject *r; @@ -368,6 +388,7 @@ get_py_api_version (void) static int py_config (const char *key, const char *value) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; int fd; FILE *fp; PyObject *modname; @@ -456,6 +477,7 @@ py_config (const char *key, const char *value) static int py_config_complete (void) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; PyObject *fn; PyObject *r; @@ -475,6 +497,7 @@ py_config_complete (void) static int py_get_ready (void) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; PyObject *fn; PyObject *r; @@ -499,6 +522,7 @@ struct handle { static void * py_open (int readonly) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; PyObject *fn; struct handle *h; @@ -531,6 +555,7 @@ py_open (int readonly) static void py_close (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -551,6 +576,7 @@ py_close (void *handle) static int64_t py_get_size (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -580,6 +606,7 @@ static int py_pread (void *handle, void *buf, uint32_t count, uint64_t offset, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -642,6 +669,7 @@ static int py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -678,6 +706,7 @@ py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, static int py_flush (void *handle, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -710,6 +739,7 @@ py_flush (void *handle, uint32_t flags) static int py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -742,6 +772,7 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags) static int py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -788,6 +819,7 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t flags) static int py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t flags) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -847,36 +879,42 @@ boolean_callback (void *handle, const char *can_fn, const char *plain_fn) static int py_is_rotational (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; return boolean_callback (handle, "is_rotational", NULL); } static int py_can_multi_conn (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; return boolean_callback (handle, "can_multi_conn", NULL); } static int py_can_write (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; return boolean_callback (handle, "can_write", "pwrite"); } static int py_can_flush (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; return boolean_callback (handle, "can_flush", "flush"); } static int py_can_trim (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; return boolean_callback (handle, "can_trim", "trim"); } static int py_can_zero (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; if (h->can_zero >= 0) @@ -887,6 +925,7 @@ py_can_zero (void *handle) static int py_can_fast_zero (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; int r; if (callback_defined ("can_fast_zero", NULL)) @@ -904,6 +943,7 @@ py_can_fast_zero (void *handle) static int py_can_fua (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; @@ -932,6 +972,7 @@ py_can_fua (void *handle) static int py_can_cache (void *handle) { + ACQUIRE_PYTHON_GIL_FOR_CURRENT_SCOPE; struct handle *h = handle; PyObject *fn; PyObject *r; -- 2.27.0

--- tests/Makefile.am | 3 + tests/test-python-thread-model.sh | 91 +++++++++++++++++++++++++++++++ tests/python-thread-model.py | 50 +++++++++++++++++ 3 files changed, 144 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 79be5639..2aa9e14c 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -1020,15 +1020,18 @@ TESTS += \ test-python.sh \ test-python-exception.sh \ test-python-export-name.sh \ + test-python-thread-model.sh \ test-shebang-python.sh \ $(NULL) EXTRA_DIST += \ python-exception.py \ python-export-name.py \ + python-thread-model.py \ shebang.py \ test-python-exception.sh \ test-python-export-name.sh \ test-python-plugin.py \ + test-python-thread-model.sh \ test-python.sh \ test-shebang-python.sh \ test_python.py \ diff --git a/tests/test-python-thread-model.sh b/tests/test-python-thread-model.sh new file mode 100755 index 00000000..e16ff154 --- /dev/null +++ b/tests/test-python-thread-model.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# nbdkit +# 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 +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +source ./functions.sh +set -e +set -x + +SCRIPT="$SRCDIR/python-thread-model.py" +if ! test -d "$SRCDIR" || ! test -f "$SCRIPT"; then + echo "$0: could not locate python-thread-model.py" + exit 1 +fi + +# Python has proven very difficult to valgrind, therefore it is disabled. +if [ "$NBDKIT_VALGRIND" = "1" ]; then + echo "$0: skipping Python test under valgrind." + exit 77 +fi + +requires nbdsh --version + +out=test-python-thread-model.out +pid=test-python-thread-model.pid +sock=`mktemp -u` +files="$out $pid $sock" +rm -f $files +cleanup_fn rm -f $files + +# Check the plugin is loadable and the effective thread model is parallel. +nbdkit python $SCRIPT --dump-plugin >$out +grep "^thread_model=parallel" $out + +start_nbdkit -P $pid -U $sock python $SCRIPT + +export sock +nbdsh -c ' +import os +import time + +h.connect_unix (os.environ["sock"]) + +# We should be able to issue multiple requests in parallel, +# and the total time taken should not be much more than 10 seconds +# because all sleeps in the plugin should happen in parallel. +start_t = time.time() +for i in range (10): + buf = nbd.Buffer (512) + h.aio_pread (buf, 0) + +while h.aio_in_flight() > 0: + h.poll(-1) +end_t = time.time() + +t = end_t - start_t +print (t) + +# Since we launched 10 requests, if we serialized on them we +# would have waited at least 100 seconds. We would expect to +# wait around 10 seconds, but for flexibility on slow servers +# any test < 100 should be fine. +assert t <= 50 +' diff --git a/tests/python-thread-model.py b/tests/python-thread-model.py new file mode 100644 index 00000000..67879e04 --- /dev/null +++ b/tests/python-thread-model.py @@ -0,0 +1,50 @@ +# nbdkit +# 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 +# met: +# +# * Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# +# * Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# * Neither the name of Red Hat nor the names of its contributors may be +# used to endorse or promote products derived from this software without +# specific prior written permission. +# +# THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A +# PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +# SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +# LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF +# USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND +# ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, +# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT +# OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +# Python plugin which uses the parallel thread model. Note that the +# cpython implementation of time.sleep releases the GIL (see +# Modules/timemodule.c:pysleep) + +import nbdkit +import time + +def thread_model(): + return nbdkit.THREAD_MODEL_PARALLEL + +def open(readonly): + return {} + +def get_size(h): + return 512 + +def pread(h, count, offset): + time.sleep(10) + return bytearray(count) -- 2.27.0