[libnbd PATCH v3 0/5] python: Speed up [aio_]pread_structured
by Eric Blake
Less copying is always better. But I was quite surprised by some of
my test cases in trying to prove that I had speedups; there's a huge
difference between:
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
and
for i in range(size // m):
buf = h.pread_structured(m, m*i, f)
buf = None
The former takes around 4.5s with libnbd 1.12.3, the latter around
15s, even though I was expecting the latter to be faster. But with
more thought, I think what is happening is that Python's compilation
and garbage collection implementation looks for opportunities where it
can prove that a pool of allocated memory can be easily reused
(assigning buf to hold a new bytearray that occupies the same size as
the previous bytearray that is now down to zero references) vs. where
it allocates new memory (the intermediate state of buf going to None
throwing away the intermediate knowledge about relations between its
other values). But with either python script, this patch makes a
noticeable difference by exposing the subbuf in [aio_]pread_structured
as a slice of the original python object, rather than yet another
memory copy.
Eric Blake (5):
python: Simplify passing of mutable *error to callbacks
python: Alter lock for persistent buffer
python: Accept all buffer-like objects in aio_p{read,write}
python: Support len(nbd.Buffer(n))
python: Slice structured read callback buffer from original
generator/Python.ml | 54 ++++++++---------
python/handle.c | 19 ++++--
python/t/405-pread-structured.py | 95 +++++++++---------------------
python/t/500-aio-pread.py | 21 +++++++
python/t/505-aio-pread-callback.py | 93 ++++++++---------------------
python/t/510-aio-pwrite.py | 17 ++++++
python/t/580-aio-is-zero.py | 2 +
python/utils.c | 53 +++++++++++++++++
8 files changed, 186 insertions(+), 168 deletions(-)
--
2.36.1
2 years, 6 months
[libnbd PATCH] python: Correctly use PyGILState
by Eric Blake
The python docs are clear that in a multi-threaded app, we CANNOT call
any python functions from C code (other than a short-list of
exceptions) without first owning the GIL in the calling thread.
Although many users of nbdsh are single-threaded (where the GIL does
not matter), it is indeed possible to write a python script that
creates python threads to manage h.poll(-1) in one thread while using
other threads to trigger aio_pread and aio_pwrite requests, so
callbacks can be reached from a different thread than the
corresponding thread that initiated the nbd I/O request.
Unfortunately, our generated code was not doing it correctly (for
example, debug_wrapper() to handle h.set_debug_callback() calls
Py_BuildValue outside PyGILState_Ensure()). But it appears that we
have so far been lucky that our callback actions generally occur based
on state machine actions triggered by another Python-based action in
the same thread, coupled with the fact that we have not been
explicitly releasing the GIL around calls into libnbd C API, so in
practice all our callbacks have already owned the GIL in spite of
coding it in the wrong place. However, I cannot rule out the risk of
a mysterious crash in a multi-threaded python program using the nbd
module. Worse, holding the GIL across a potentially-blocking C
operation is bad form; no other Python thread can make progress if we
don't release the GIL.
Fix this by widening the scope of PyGILState in all callback wrappers
(all other code calling into Python is assumed to already own the
GIL), and by dropping the GIL around calls into libnbd C code.
Note that the Python docs also state that manipulation of PyGILState
is incompatible with the use of sub-interpreters with
Py_NewInterpreter(); but if someone actually wants to write a complex
program that uses sub-interpreters to interact with libnbd via Python
independently from some other use of python, rather than just directly
interacting with libnbd via C, I'd be surprised.
Fixes: 936488d4 ("python: Implement Callback properly.", v0.1)
---
generator/Python.ml | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml
index e94f37b..f196e8e 100644
--- a/generator/Python.ml
+++ b/generator/Python.ml
@@ -158,7 +158,7 @@ let
pr " const struct user_data *data = user_data;\n";
pr " int ret = -1;\n";
pr "\n";
- pr " PyGILState_STATE py_save = PyGILState_UNLOCKED;\n";
+ pr " PyGILState_STATE py_save = PyGILState_Ensure();\n";
pr " PyObject *py_args, *py_ret;\n";
List.iter (
function
@@ -227,9 +227,7 @@ let
pr ");\n";
pr " if (!py_args) { PyErr_PrintEx (0); goto out; }\n";
pr "\n";
- pr " py_save = PyGILState_Ensure ();\n";
pr " py_ret = PyObject_CallObject (data->fn, py_args);\n";
- pr " PyGILState_Release (py_save);\n";
pr "\n";
pr " Py_DECREF (py_args);\n";
pr "\n";
@@ -268,6 +266,7 @@ let
| CBUInt _ | CBUInt64 _ -> ()
| CBArrayAndLen _ | CBMutable _ -> assert false
) cbargs;
+ pr " PyGILState_Release (py_save);\n";
pr " return ret;\n";
pr "}\n";
pr "\n"
@@ -478,7 +477,8 @@ let
| BytesPersistOut (n, _) ->
pr " if (nbd_internal_py_init_aio_buffer (%s) < 0) goto out;\n" n
| _ -> ()
- ) args;
+ ) args;
+ pr " Py_BEGIN_ALLOW_THREADS\n";
pr " ret = nbd_%s (" name;
pr_wrap ',' (fun () ->
pr "h";
@@ -487,6 +487,7 @@ let
| _, _, n -> pr ", %s" n
) params);
pr ");\n";
+ pr " Py_END_ALLOW_THREADS\n";
List.iter (
function
| Closure { cbname } -> pr " %s_user_data = NULL;\n" cbname
@@ -613,8 +614,10 @@ let
pr " struct user_data *data = user_data;\n";
pr "\n";
pr " if (data) {\n";
+ pr " PyGILState_STATE py_save = PyGILState_Ensure();\n";
pr " Py_XDECREF (data->fn);\n";
pr " Py_XDECREF (data->buf);\n";
+ pr " PyGILState_Release (py_save);\n";
pr " free (data);\n";
pr " }\n";
pr "}\n";
--
2.36.1
2 years, 6 months
[PATCH nbdkit] luks: Don't advertise zero support
by Richard W.M. Jones
Eric: This seems like a bug in nbdkit, but this patch just does the
easy fix. What do you think?
Two other filters can return NBDKIT_ZERO_EMULATE:
filters/ext2/ext2.c: return NBDKIT_ZERO_EMULATE;
filters/nozero/nozero.c: return NBDKIT_ZERO_EMULATE;
nbdkit-ext2-filter doesn't crash, but it doesn't emulate either and it
seems broken. It calls down to the underlying plugin (which will
either emulate or true zero). I believe it may be zeroing a random
offset in the underlying disk.
Rich.
2 years, 6 months
luks filter breaks zeroing
by Richard W.M. Jones
(More of a note to self to investigate)
$ qemu-img create -f luks --object secret,data=LETMEPASS,id=sec0 -o key-secret=sec0 encrypted.img 100M
Formatting 'encrypted.img', fmt=luks size=104857600 key-secret=sec0
$ rm -f data.img
$ truncate -s 100M data.img
$ nbdkit file encrypted.img --filter=luks passphrase=LETMEPASS --run 'nbdcopy data.img $nbd'
nbdkit: backend.c:718: backend_zero: Assertion `c->can_zero > NBDKIT_ZERO_NONE' failed.
write at offset 0 failed: Transport endpoint is not connected
nbdkit: nbdkit command was killed by signal 6
--
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
2 years, 6 months
libnbd golang failure on RISC-V
by Richard W.M. Jones
make[2]: Entering directory '/home/rjones/d/libnbd/golang'
perl /home/rjones/d/libnbd/podwrapper.pl --section=3 --man libnbd-golang.3 \
--html ../html/libnbd-golang.3.html \
libnbd-golang.pod
/home/rjones/d/libnbd/run go build
write of Go pointer 0x3fa8028000 to non-Go memory 0x3fd2c0fb20
fatal error: Go pointer stored into non-Go memory
runtime stack:
runtime_mstart
../../../libgo/runtime/proc.c:593
goroutine 1 [running, locked to thread]:
Not sure how I should diagnose this one further? I wouldn't normally
worry about RISC-V failures, but they may indicate some kind of arch
generic problem that is just not exposed on x86.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-builder quickly builds VMs from scratch
http://libguestfs.org/virt-builder.1.html
2 years, 6 months
[PATCH libnbd] golang: aio_buffer.go: Explicit panic() on invalid usage
by Nir Soffer
Previously we depended on the behavior on common platforms to panic when
trying to use a nil pointer, but Richard reported that it segfault on
RISC-V. Avoid the undocumented assumptions and panic explicitly with a
useful panic message.
Signed-off-by: Nir Soffer <nsoffer(a)redhat.com>
---
golang/aio_buffer.go | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/golang/aio_buffer.go b/golang/aio_buffer.go
index 52ea54de..325dbc98 100644
--- a/golang/aio_buffer.go
+++ b/golang/aio_buffer.go
@@ -65,28 +65,37 @@ func (b *AioBuffer) Free() {
if b.P != nil {
C.free(b.P)
b.P = nil
}
}
// Bytes copies the underlying C array to Go allocated memory and return a
// slice. Modifying the returned slice does not modify the underlying buffer
// backing array.
func (b *AioBuffer) Bytes() []byte {
+ if b.P == nil {
+ panic("Using AioBuffer after Free()")
+ }
return C.GoBytes(b.P, C.int(b.Size))
}
// Slice creates a slice backed by the underlying C array. The slice can be
// used to access or modify the contents of the underlying array. The slice
// must not be used after caling Free().
func (b *AioBuffer) Slice() []byte {
+ if b.P == nil {
+ panic("Using AioBuffer after Free()")
+ }
// See https://github.com/golang/go/wiki/cgo#turning-c-arrays-into-go-slices
// TODO: Use unsafe.Slice() when we require Go 1.17.
return (*[1<<30]byte)(b.P)[:b.Size:b.Size]
}
// Get returns a pointer to a byte in the underlying C array. The pointer can
// be used to modify the underlying array. The pointer must not be used after
// calling Free().
func (b *AioBuffer) Get(i uint) *byte {
+ if b.P == nil {
+ panic("Using AioBuffer after Free()")
+ }
return (*byte)(unsafe.Pointer(uintptr(b.P) + uintptr(i)))
}
--
2.36.1
2 years, 6 months
