[libnbd PATCH 0/2] Optimize h.pread_structured
Add some more safety against a non-compliant server with structured reads, and kill off another useless copy operation for a noticeable speedup when using h.pread_structured. However, I'm still trying to play with code to see if PyObject_GetItem(memoryview, PySlice_New(start, end, NULL)) can get me the same zero-copy python object but in a way that can survive as long as the original object lives, rather than forcing .release() at the end of the callback. Maybe I'll have a patch 3/2 later today. Eric Blake (2): api: Tighter checking of structured read replies python: Optimize away copy during pread_structured lib/internal.h | 2 +- generator/Python.ml | 20 ++++++++++++++------ generator/states-reply-simple.c | 4 ++-- generator/states-reply-structured.c | 6 ++++-- lib/aio.c | 7 +++++-- 5 files changed, 26 insertions(+), 13 deletions(-) -- 2.36.1
Now that we allow clients to bypass buffer pre-initialization, it becomes more important to detect when a buggy server using structured replies does not send us enough bytes to cover the requested read size. Our check is not perfect (a server that duplicates reply chunks for byte 0 and omits byte 1 gets past our check), but this is a tighter sanity check so that we are less likely to report a successful read containing uninitialized memory on a buggy server. Because we have a maximum read buffer size of 64M, and first check that the server's chunk fits bounds, we don't have to worry about overflowing a uint32_t, even if a server sends enough duplicate responses that an actual sum would overflow. --- lib/internal.h | 2 +- generator/states-reply-simple.c | 4 ++-- generator/states-reply-structured.c | 6 ++++-- lib/aio.c | 7 +++++-- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 885cee1..4121a5c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -352,8 +352,8 @@ struct command { void *data; /* Buffer for read/write */ struct command_cb cb; enum state state; /* State to resume with on next POLLIN */ - bool data_seen; /* For read, true if at least one data chunk seen */ bool initialized; /* For read, true if getting a hole may skip memset */ + uint32_t data_seen; /* For read, cumulative size of data chunks seen */ uint32_t error; /* Local errno value */ }; diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7dc26fd..2a7b9a9 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -40,7 +40,7 @@ STATE_MACHINE { if (cmd->error == 0 && cmd->type == NBD_CMD_READ) { h->rbuf = cmd->data; h->rlen = cmd->count; - cmd->data_seen = true; + cmd->data_seen = cmd->count; SET_NEXT_STATE (%RECV_READ_PAYLOAD); } else { diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 12c24f5..cabd543 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -354,7 +354,6 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */ assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true; /* Length of the data following. */ length -= 8; @@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length; /* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; @@ -422,13 +423,14 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */ assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true; /* Is the data within bounds? */ if (! structured_reply_in_bounds (offset, length, cmd)) { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length; /* Now this is the byte offset in the read buffer. */ offset -= cmd->offset; diff --git a/lib/aio.c b/lib/aio.c index 9744840..dc01f90 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -91,8 +91,11 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, assert (cmd->type != NBD_CMD_DISC); /* The spec states that a 0-length read request is unspecified; but * it is easy enough to treat it as successful as an extension. + * Conversely, make sure a server sending structured replies sent + * enough data chunks to cover the overall count (although we do not + * detect if it duplicated some bytes while omitting others). */ - if (type == NBD_CMD_READ && !cmd->data_seen && cmd->count && !error) + if (type == NBD_CMD_READ && cmd->data_seen != cmd->count && !error) error = EIO; /* Retire it from the list and free it. */ -- 2.36.1
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake@redhat.com> wrote:
Now that we allow clients to bypass buffer pre-initialization, it becomes more important to detect when a buggy server using structured replies does not send us enough bytes to cover the requested read size. Our check is not perfect (a server that duplicates reply chunks for byte 0 and omits byte 1 gets past our check), but this is a tighter sanity check so that we are less likely to report a successful read containing uninitialized memory on a buggy server.
Nice!
Because we have a maximum read buffer size of 64M, and first check that the server's chunk fits bounds, we don't have to worry about overflowing a uint32_t, even if a server sends enough duplicate responses that an actual sum would overflow. --- lib/internal.h | 2 +- generator/states-reply-simple.c | 4 ++-- generator/states-reply-structured.c | 6 ++++-- lib/aio.c | 7 +++++-- 4 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h index 885cee1..4121a5c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -352,8 +352,8 @@ struct command { void *data; /* Buffer for read/write */ struct command_cb cb; enum state state; /* State to resume with on next POLLIN */ - bool data_seen; /* For read, true if at least one data chunk seen */ bool initialized; /* For read, true if getting a hole may skip memset */ + uint32_t data_seen; /* For read, cumulative size of data chunks seen */ uint32_t error; /* Local errno value */ };
diff --git a/generator/states-reply-simple.c b/generator/states-reply-simple.c index 7dc26fd..2a7b9a9 100644 --- a/generator/states-reply-simple.c +++ b/generator/states-reply-simple.c @@ -1,5 +1,5 @@ /* nbd client library in userspace: state machine - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -40,7 +40,7 @@ STATE_MACHINE { if (cmd->error == 0 && cmd->type == NBD_CMD_READ) { h->rbuf = cmd->data; h->rlen = cmd->count; - cmd->data_seen = true; + cmd->data_seen = cmd->count; SET_NEXT_STATE (%RECV_READ_PAYLOAD); } else { diff --git a/generator/states-reply-structured.c b/generator/states-reply-structured.c index 12c24f5..cabd543 100644 --- a/generator/states-reply-structured.c +++ b/generator/states-reply-structured.c @@ -354,7 +354,6 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */
assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true;
/* Length of the data following. */ length -= 8; @@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
This does not feel right. if you received more data, it should be counted, and if this causes data_seen to be bigger than cmd->count, isn't this a fatal error?
/* Now this is the byte offset in the read buffer. */ offset -= cmd->offset;
@@ -422,13 +423,14 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */
assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true;
/* Is the data within bounds? */ if (! structured_reply_in_bounds (offset, length, cmd)) { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
Same here
/* Now this is the byte offset in the read buffer. */ offset -= cmd->offset;
diff --git a/lib/aio.c b/lib/aio.c index 9744840..dc01f90 100644 --- a/lib/aio.c +++ b/lib/aio.c @@ -1,5 +1,5 @@ /* NBD client library in userspace - * Copyright (C) 2013-2019 Red Hat Inc. + * Copyright (C) 2013-2022 Red Hat Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -91,8 +91,11 @@ nbd_unlocked_aio_command_completed (struct nbd_handle *h, assert (cmd->type != NBD_CMD_DISC); /* The spec states that a 0-length read request is unspecified; but * it is easy enough to treat it as successful as an extension. + * Conversely, make sure a server sending structured replies sent + * enough data chunks to cover the overall count (although we do not + * detect if it duplicated some bytes while omitting others). */ - if (type == NBD_CMD_READ && !cmd->data_seen && cmd->count && !error) + if (type == NBD_CMD_READ && cmd->data_seen != cmd->count && !error) error = EIO;
/* Retire it from the list and free it. */ -- 2.36.1
_______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
Nir
On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake@redhat.com> wrote:
Now that we allow clients to bypass buffer pre-initialization, it becomes more important to detect when a buggy server using structured replies does not send us enough bytes to cover the requested read size. Our check is not perfect (a server that duplicates reply chunks for byte 0 and omits byte 1 gets past our check), but this is a tighter sanity check so that we are less likely to report a successful read containing uninitialized memory on a buggy server.
Nice!
Because we have a maximum read buffer size of 64M, and first check that the server's chunk fits bounds, we don't have to worry about overflowing a uint32_t, even if a server sends enough duplicate responses that an actual sum would overflow. ---
+++ b/generator/states-reply-structured.c @@ -354,7 +354,6 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */
assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true;
/* Length of the data following. */ length -= 8; @@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
This does not feel right. if you received more data, it should be counted, and if this causes data_seen to be bigger than cmd->count, isn't this a fatal error?
cmd->count is at most 64M; it represents how much we asked the server to provide. length was just validated (in the elided statement between these two hunks) to be <= cmd->count (otherwise, the server is known-buggy for replying with more than we asked, and we've already moved to %.DEAD state). cmd->data_seen is a cumulative count of all bytes seen in prior chunks, plus the current chunk. If we have not yet passed cmd->count, then this chunk counts towards the cumulative limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we have already passed cmd->count (in a previous chunk), then we don't further increase cmd->count, but we already know that we will fail the overall read later. In other words, we can stop incrementing cmd->data_seen as soon as we know it exceeds cmd->count, and by stopping early, we still detect server bugs without overflowing uint32_t. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, May 31, 2022 at 7:13 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 06:59:25PM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake@redhat.com> wrote:
Now that we allow clients to bypass buffer pre-initialization, it becomes more important to detect when a buggy server using structured replies does not send us enough bytes to cover the requested read size. Our check is not perfect (a server that duplicates reply chunks for byte 0 and omits byte 1 gets past our check), but this is a tighter sanity check so that we are less likely to report a successful read containing uninitialized memory on a buggy server.
Nice!
Because we have a maximum read buffer size of 64M, and first check that the server's chunk fits bounds, we don't have to worry about overflowing a uint32_t, even if a server sends enough duplicate responses that an actual sum would overflow. ---
+++ b/generator/states-reply-structured.c @@ -354,7 +354,6 @@ STATE_MACHINE { assert (cmd); /* guaranteed by CHECK */
assert (cmd->data && cmd->type == NBD_CMD_READ); - cmd->data_seen = true;
/* Length of the data following. */ length -= 8; @@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
This does not feel right. if you received more data, it should be counted, and if this causes data_seen to be bigger than cmd->count, isn't this a fatal error?
cmd->count is at most 64M; it represents how much we asked the server to provide. length was just validated (in the elided statement between these two hunks) to be <= cmd->count (otherwise, the server is known-buggy for replying with more than we asked, and we've already moved to %.DEAD state). cmd->data_seen is a cumulative count of all bytes seen in prior chunks, plus the current chunk. If we have not yet passed cmd->count, then this chunk counts towards the cumulative limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we have already passed cmd->count (in a previous chunk), then we don't further increase cmd->count, but we already know that we will fail the overall read later. In other words, we can stop incrementing cmd->data_seen as soon as we know it exceeds cmd->count, and by stopping early, we still detect server bugs without overflowing uint32_t.
But we can have this case: 1. ask for 32m 2. server sends 16m (data_seen increase to 16m) 3. server sends 16m (data_seen increase to 32m) 4. server sends 1m (data_seen does not increase) 5. entire request succeeds Shouldn't we fail if server sends unexpected data? If we detected that all data was received, and we get unexpected data, why not fail immediately? cmd->data_seen += length if (cmd->data_seen > cmd->count) switch to dead state? Nir
On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote:
@@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
This does not feel right. if you received more data, it should be counted, and if this causes data_seen to be bigger than cmd->count, isn't this a fatal error?
cmd->count is at most 64M; it represents how much we asked the server to provide. length was just validated (in the elided statement between these two hunks) to be <= cmd->count (otherwise, the server is known-buggy for replying with more than we asked, and we've already moved to %.DEAD state). cmd->data_seen is a cumulative count of all bytes seen in prior chunks, plus the current chunk. If we have not yet passed cmd->count, then this chunk counts towards the cumulative limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we have already passed cmd->count (in a previous chunk), then we don't further increase cmd->count, but we already know that we will fail the overall read later. In other words, we can stop incrementing cmd->data_seen as soon as we know it exceeds cmd->count, and by stopping early, we still detect server bugs without overflowing uint32_t.
But we can have this case:
1. ask for 32m 2. server sends 16m (data_seen increase to 16m) 3. server sends 16m (data_seen increase to 32m) 4. server sends 1m (data_seen does not increase)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m. Then, later on when retiring the command, we note that 33m != 32m and fail the read with EIO (if it has not already failed for other reasons).
5. entire request succeeds
Shouldn't we fail if server sends unexpected data?
If we detected that all data was received, and we get unexpected data, why not fail immediately?
cmd->data_seen += length if (cmd->data_seen > cmd->count) switch to dead state?
Switching immediately to a dead state is also possible, but it's nice to try and keep the connection alive as long as we can with a nice diagnosis of a failed CMD_READ but still allow further commands, rather than an abrupt disconnect that takes out all other use of the server. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, May 31, 2022 at 10:23 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 08:40:57PM +0300, Nir Soffer wrote:
@@ -364,6 +363,8 @@ STATE_MACHINE { SET_NEXT_STATE (%.DEAD); return 0; } + if (cmd->data_seen <= cmd->count) + cmd->data_seen += length;
This does not feel right. if you received more data, it should be counted, and if this causes data_seen to be bigger than cmd->count, isn't this a fatal error?
cmd->count is at most 64M; it represents how much we asked the server to provide. length was just validated (in the elided statement between these two hunks) to be <= cmd->count (otherwise, the server is known-buggy for replying with more than we asked, and we've already moved to %.DEAD state). cmd->data_seen is a cumulative count of all bytes seen in prior chunks, plus the current chunk. If we have not yet passed cmd->count, then this chunk counts towards the cumulative limit (and there is no overflow, since 64M*2 < UINT32_MAX). If we have already passed cmd->count (in a previous chunk), then we don't further increase cmd->count, but we already know that we will fail the overall read later. In other words, we can stop incrementing cmd->data_seen as soon as we know it exceeds cmd->count, and by stopping early, we still detect server bugs without overflowing uint32_t.
But we can have this case:
1. ask for 32m 2. server sends 16m (data_seen increase to 16m) 3. server sends 16m (data_seen increase to 32m) 4. server sends 1m (data_seen does not increase)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.
Right! I missed this.
Then, later on when retiring the command, we note that 33m != 32m and fail the read with EIO (if it has not already failed for other reasons).
5. entire request succeeds
Shouldn't we fail if server sends unexpected data?
If we detected that all data was received, and we get unexpected data, why not fail immediately?
cmd->data_seen += length if (cmd->data_seen > cmd->count) switch to dead state?
Switching immediately to a dead state is also possible, but it's nice to try and keep the connection alive as long as we can with a nice diagnosis of a failed CMD_READ but still allow further commands, rather than an abrupt disconnect that takes out all other use of the server.
I agree, this is better. Reviewed-by: Nir Soffer <nsoffer@redhat.com>
On Tue, May 31, 2022 at 10:32:23PM +0300, Nir Soffer wrote:
But we can have this case:
1. ask for 32m 2. server sends 16m (data_seen increase to 16m) 3. server sends 16m (data_seen increase to 32m) 4. server sends 1m (data_seen does not increase)
Yes it does. 32m <= cmd->count is true, so we bump data_seen to 33m.
Right! I missed this.
Then, later on when retiring the command, we note that 33m != 32m and fail the read with EIO (if it has not already failed for other reasons).
5. entire request succeeds
Shouldn't we fail if server sends unexpected data?
If we detected that all data was received, and we get unexpected data, why not fail immediately?
cmd->data_seen += length if (cmd->data_seen > cmd->count) switch to dead state?
Switching immediately to a dead state is also possible, but it's nice to try and keep the connection alive as long as we can with a nice diagnosis of a failed CMD_READ but still allow further commands, rather than an abrupt disconnect that takes out all other use of the server.
I agree, this is better.
Reviewed-by: Nir Soffer <nsoffer@redhat.com>
I've pushed this one as edd8f5c; the rest of the series will be tweaked and posted as v2. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
The Py_BuildValue "y#" format copies data; this is because Python wants to allow our C memory to go out of scope without worrying about whether the user's python callback has stashed off a longer-living reference to its incoming parameter. But it is inefficient; we can do better by utilizing Python's memoryview for a zero-copy exposure to the callback's C buffer, as well as a .release method that we can utilize just before our C memory goes out of scope. Now, if the user stashes away a reference, they will get a clean Python error if they try to access the memory after the fact. This IS an API change (code previously expecting a stashed copy to be long-lived will break), but we never promised Python API stability, and anyone writing a callback that saves off data was already risky (neither libnbd nor nbdkit's testsuite had such a case). For a demonstration of the new error, where the old code succeeded: $ ./run nbdsh nbd> h.connect_command(["nbdkit", "-s", "memory", "10"]) nbd> copy=None nbd> def f(b,o,s,e): ... global copy ... copy = b ... print(b[0]) ... nbd> print(copy) None nbd> h.pread_structured(10,0,f) 0 bytearray(b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') nbd> copy <released memory at 0x7ff97410de40> nbd> copy[0] Traceback (most recent call last): File "/usr/lib64/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> ValueError: operation forbidden on released memoryview object To demonstrate the speedup, I tested: $ export script=' def f(b,o,s,e): pass m=1024*1024 size=h.get_size() for i in range(size // m): buf = h.pread_structured(m, m*i, f) ' $ time ./run nbdkit -U - memory 10G --run 'nbdsh -u "$uri" -c "$script"' On my machine, this took 9.1s pre-patch, and 3.0s post-patch, for an approximate 65% speedup. The corresponding diff to the generated code is: | --- python/methods.c.bak 2022-05-31 07:57:25.256293091 -0500 | +++ python/methods.c 2022-05-31 08:14:09.570567858 -0500 | @@ -73,8 +73,12 @@ chunk_wrapper (void *user_data, const vo | | PyGILState_STATE py_save = PyGILState_UNLOCKED; | PyObject *py_args, *py_ret; | + PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | + /* Cast away const */ | + py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (!py_subbuf) { PyErr_PrintEx (0); goto out; } | PyObject *py_error_modname = PyUnicode_FromString ("ctypes"); | if (!py_error_modname) { PyErr_PrintEx (0); goto out; } | PyObject *py_error_mod = PyImport_Import (py_error_modname); | @@ -84,7 +88,7 @@ | Py_DECREF (py_error_mod); | if (!py_error) { PyErr_PrintEx (0); goto out; } | | - py_args = Py_BuildValue ("(" "y#" "K" "I" "O" ")", subbuf, (int) count, offset, status, py_error); | + py_args = Py_BuildValue ("(" "O" "K" "I" "O" ")", py_subbuf, offset, status, py_error); | if (!py_args) { PyErr_PrintEx (0); goto out; } | | py_save = PyGILState_Ensure (); | @@ -111,6 +115,11 @@ chunk_wrapper (void *user_data, const vo | }; | | out: | + if (py_subbuf) { | + PyObject *tmp = PyObject_CallMethod(py_subbuf, "release", NULL); | + Py_XDECREF (tmp); | + Py_DECREF (py_subbuf); | + } | if (py_error) { | PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value"); | *error = PyLong_AsLong (py_error_ret); --- generator/Python.ml | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 1c4446e..0191f79 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -169,8 +169,7 @@ let pr " PyObject *py_args, *py_ret;\n"; List.iter ( function - | CBArrayAndLen (UInt32 n, _) -> - pr " PyObject *py_%s = NULL;\n" n + | CBArrayAndLen (UInt32 n, _) | CBBytesIn (n, _) | CBMutable (Int n) -> pr " PyObject *py_%s = NULL;\n" n | _ -> () @@ -187,7 +186,10 @@ let pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" - | CBBytesIn _ + | CBBytesIn (n, len) -> + pr " /* Cast away const */\n"; + pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n | CBInt _ | CBInt64 _ -> () | CBMutable (Int n) -> @@ -210,7 +212,7 @@ let List.iter ( function | CBArrayAndLen (UInt32 n, len) -> pr " \"O\"" - | CBBytesIn (n, len) -> pr " \"y#\"" + | CBBytesIn (n, _) -> pr " \"O\"" | CBInt n -> pr " \"i\"" | CBInt64 n -> pr " \"L\"" | CBMutable (Int n) -> pr " \"O\"" @@ -223,7 +225,7 @@ let List.iter ( function | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n - | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len + | CBBytesIn (n, _) -> pr ", py_%s" n | CBMutable (Int n) -> pr ", py_%s" n | CBInt n | CBInt64 n | CBString n @@ -268,7 +270,13 @@ let pr " Py_DECREF (py_%s_ret);\n" n; pr " Py_DECREF (py_%s);\n" n; pr " }\n" - | CBBytesIn _ + | CBBytesIn (n, _) -> + (* https://stackoverflow.com/questions/58870010/how-to-call-release-on-a-memory... *) + pr " if (py_%s) {\n" n; + pr " PyObject *tmp = PyObject_CallMethod(py_%s, \"release\", NULL);\n" n; + pr " Py_XDECREF (tmp);\n"; + pr " Py_DECREF (py_%s);\n" n; + pr " }\n" | CBInt _ | CBInt64 _ | CBString _ | CBUInt _ | CBUInt64 _ -> () -- 2.36.1
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake@redhat.com> wrote:
The Py_BuildValue "y#" format copies data; this is because Python wants to allow our C memory to go out of scope without worrying about whether the user's python callback has stashed off a longer-living reference to its incoming parameter. But it is inefficient; we can do better by utilizing Python's memoryview for a zero-copy exposure to the callback's C buffer, as well as a .release method that we can utilize just before our C memory goes out of scope. Now, if the user stashes away a reference, they will get a clean Python error if they try to access the memory after the fact. This IS an API change (code previously expecting a stashed copy to be long-lived will break), but we never promised Python API stability, and anyone writing a callback that saves off data was already risky (neither libnbd nor nbdkit's testsuite had such a case). For a demonstration of the new error, where the old code succeeded:
$ ./run nbdsh nbd> h.connect_command(["nbdkit", "-s", "memory", "10"]) nbd> copy=None nbd> def f(b,o,s,e): ... global copy ... copy = b ... print(b[0]) ... nbd> print(copy) None nbd> h.pread_structured(10,0,f) 0 bytearray(b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') nbd> copy <released memory at 0x7ff97410de40> nbd> copy[0] Traceback (most recent call last): File "/usr/lib64/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> ValueError: operation forbidden on released memoryview object
To demonstrate the speedup, I tested:
$ export script=' def f(b,o,s,e): pass m=1024*1024 size=h.get_size() for i in range(size // m): buf = h.pread_structured(m, m*i, f) ' $ time ./run nbdkit -U - memory 10G --run 'nbdsh -u "$uri" -c "$script"'
On my machine, this took 9.1s pre-patch, and 3.0s post-patch, for an approximate 65% speedup.
Looks like ~300% speedup to me. I guess 3 times faster is the most clear way to describe the change.
The corresponding diff to the generated code is:
| --- python/methods.c.bak 2022-05-31 07:57:25.256293091 -0500 | +++ python/methods.c 2022-05-31 08:14:09.570567858 -0500 | @@ -73,8 +73,12 @@ chunk_wrapper (void *user_data, const vo | | PyGILState_STATE py_save = PyGILState_UNLOCKED; | PyObject *py_args, *py_ret; | + PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | + /* Cast away const */
I think it will be more clear and helpful as: /* Casting subbuf to char* is safe since we use PyBUF_READ. */
| + py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ);
Maybe py_view? And also change the doscring to mention "view" instead of "subbuf".
| + if (!py_subbuf) { PyErr_PrintEx (0); goto out; } | PyObject *py_error_modname = PyUnicode_FromString ("ctypes"); | if (!py_error_modname) { PyErr_PrintEx (0); goto out; } | PyObject *py_error_mod = PyImport_Import (py_error_modname); | @@ -84,7 +88,7 @@ | Py_DECREF (py_error_mod); | if (!py_error) { PyErr_PrintEx (0); goto out; } | | - py_args = Py_BuildValue ("(" "y#" "K" "I" "O" ")", subbuf, (int) count, offset, status, py_error); | + py_args = Py_BuildValue ("(" "O" "K" "I" "O" ")", py_subbuf, offset, status, py_error); | if (!py_args) { PyErr_PrintEx (0); goto out; } | | py_save = PyGILState_Ensure (); | @@ -111,6 +115,11 @@ chunk_wrapper (void *user_data, const vo | }; | | out: | + if (py_subbuf) { | + PyObject *tmp = PyObject_CallMethod(py_subbuf, "release", NULL);
What it the user does: def f(b,o,s,e): use b... b.release() Or: def f(b,o,s,e): with b: use b... We would release a released memoryview here. I don't think this is likely to happen though.
| + Py_XDECREF (tmp); | + Py_DECREF (py_subbuf); | + } | if (py_error) { | PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value"); | *error = PyLong_AsLong (py_error_ret); --- generator/Python.ml | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)
diff --git a/generator/Python.ml b/generator/Python.ml index 1c4446e..0191f79 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -169,8 +169,7 @@ let pr " PyObject *py_args, *py_ret;\n"; List.iter ( function - | CBArrayAndLen (UInt32 n, _) -> - pr " PyObject *py_%s = NULL;\n" n + | CBArrayAndLen (UInt32 n, _) | CBBytesIn (n, _) | CBMutable (Int n) -> pr " PyObject *py_%s = NULL;\n" n | _ -> () @@ -187,7 +186,10 @@ let pr " if (!py_e_%s) { PyErr_PrintEx (0); goto out; }\n" n; pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" - | CBBytesIn _ + | CBBytesIn (n, len) -> + pr " /* Cast away const */\n"; + pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n | CBInt _ | CBInt64 _ -> () | CBMutable (Int n) -> @@ -210,7 +212,7 @@ let List.iter ( function | CBArrayAndLen (UInt32 n, len) -> pr " \"O\"" - | CBBytesIn (n, len) -> pr " \"y#\"" + | CBBytesIn (n, _) -> pr " \"O\"" | CBInt n -> pr " \"i\"" | CBInt64 n -> pr " \"L\"" | CBMutable (Int n) -> pr " \"O\"" @@ -223,7 +225,7 @@ let List.iter ( function | CBArrayAndLen (UInt32 n, _) -> pr ", py_%s" n - | CBBytesIn (n, len) -> pr ", %s, (int) %s" n len + | CBBytesIn (n, _) -> pr ", py_%s" n | CBMutable (Int n) -> pr ", py_%s" n | CBInt n | CBInt64 n | CBString n @@ -268,7 +270,13 @@ let pr " Py_DECREF (py_%s_ret);\n" n; pr " Py_DECREF (py_%s);\n" n; pr " }\n" - | CBBytesIn _ + | CBBytesIn (n, _) -> + (* https://stackoverflow.com/questions/58870010/how-to-call-release-on-a-memory... *) + pr " if (py_%s) {\n" n; + pr " PyObject *tmp = PyObject_CallMethod(py_%s, \"release\", NULL);\n" n; + pr " Py_XDECREF (tmp);\n"; + pr " Py_DECREF (py_%s);\n" n; + pr " }\n" | CBInt _ | CBInt64 _ | CBString _ | CBUInt _ | CBUInt64 _ -> () -- 2.36.1
I'm not sure about this change - the performance improvement is great, but the API change is surprising. Since pread_structured() returns a buffer with the read data, can we arrange that the memoryview argument to the chunk callback is pointing into the returned buffer, so the view does not have to be released by the C extension? Nir
On Tue, May 31, 2022 at 11:12:50PM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 5:15 PM Eric Blake <eblake@redhat.com> wrote:
The Py_BuildValue "y#" format copies data; this is because Python wants to allow our C memory to go out of scope without worrying about whether the user's python callback has stashed off a longer-living reference to its incoming parameter. But it is inefficient; we can do better by utilizing Python's memoryview for a zero-copy exposure to the callback's C buffer, as well as a .release method that we can utilize just before our C memory goes out of scope. Now, if the user stashes away a reference, they will get a clean Python error if they try to access the memory after the fact. This IS an API change (code previously expecting a stashed copy to be long-lived will break), but we never promised Python API stability, and anyone writing a callback that saves off data was already risky (neither libnbd nor nbdkit's testsuite had such a case). For a demonstration of the new error, where the old code succeeded:
$ ./run nbdsh nbd> h.connect_command(["nbdkit", "-s", "memory", "10"]) nbd> copy=None nbd> def f(b,o,s,e): ... global copy ... copy = b ... print(b[0]) ... nbd> print(copy) None nbd> h.pread_structured(10,0,f) 0 bytearray(b'\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00') nbd> copy <released memory at 0x7ff97410de40> nbd> copy[0] Traceback (most recent call last): File "/usr/lib64/python3.10/code.py", line 90, in runcode exec(code, self.locals) File "<console>", line 1, in <module> ValueError: operation forbidden on released memoryview object
To demonstrate the speedup, I tested:
$ export script=' def f(b,o,s,e): pass m=1024*1024 size=h.get_size() for i in range(size // m): buf = h.pread_structured(m, m*i, f) ' $ time ./run nbdkit -U - memory 10G --run 'nbdsh -u "$uri" -c "$script"'
On my machine, this took 9.1s pre-patch, and 3.0s post-patch, for an approximate 65% speedup.
Looks like ~300% speedup to me. I guess 3 times faster is the most clear way to describe the change.
I guess it depends whether you compare the delta in execution speed to the old or the new speed. But either way, it is a nice speedup! (I'll probably re-write to use the ~3x wording, as that sounds more impressive)
The corresponding diff to the generated code is:
| --- python/methods.c.bak 2022-05-31 07:57:25.256293091 -0500 | +++ python/methods.c 2022-05-31 08:14:09.570567858 -0500 | @@ -73,8 +73,12 @@ chunk_wrapper (void *user_data, const vo | | PyGILState_STATE py_save = PyGILState_UNLOCKED; | PyObject *py_args, *py_ret; | + PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | + /* Cast away const */
I think it will be more clear and helpful as:
/* Casting subbuf to char* is safe since we use PyBUF_READ. */
| + py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ);
Yes, good idea.
Maybe py_view?
And also change the doscring to mention "view" instead of "subbuf".
That's determined by the public API parameter name. The name is not an API change, per-se, but changing it (in API.ml) would have knock-on effects to other language bindings. And 'view' is potentially misleading, since while it has always been a view in C, and this patch is getting us closer to it being a view in Python (you need patch 3/2 to make it an actual view of the final buffer for h.pread_structured, and an as-yet unfinished 4/2 before it is an actual view of the buffer passed to h.aio_pread_structured), we do not guarantee that all other language bindings are able to avoid the memcpy.
| out: | + if (py_subbuf) { | + PyObject *tmp = PyObject_CallMethod(py_subbuf, "release", NULL);
What it the user does:
def f(b,o,s,e): use b... b.release()
Or:
def f(b,o,s,e): with b: use b...
We would release a released memoryview here.
I don't think this is likely to happen though.
Even if it does happen, it's harmless. https://docs.python.org/3/library/stdtypes.html#memoryview.release says "After this method has been called, any further operation on the view raises a ValueError (except release() itself which can be called multiple times):".
I'm not sure about this change - the performance improvement is great, but the API change is surprising.
Since pread_structured() returns a buffer with the read data, can we arrange that the memoryview argument to the chunk callback is pointing into the returned buffer, so the view does not have to be released by the C extension?
Yes, in patch 3/2 for h.pread_structured; and I'm still working on it for h.aio_pread_structured. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid. Note that this does not tackle the case of aio_pread_structured; for that, we either need to make nbd.Buffer honor the Python buffer protocol, or else change our handling of BytesPersistOut to utilize the buffer protocol directly instead of requiring the user to round-trip through nbd.Buffer. The runtime of this patch is still about the same 3.0s as in the test of the previous patch, but now you can do: $ nbdsh nbd> h.connect_command(["nbdkit","-s","memory","10"]) nbd> copy=None nbd> def f(b,o,s,e): ... global copy ... copy = b ... print(b[0]) ... nbd> print(copy) None nbd> buf = h.pread_structured(10,0,f) 0 nbd> copy <memory at 0x7fecef4e07c0> nbd> copy[0]=0x31 nbd> print(buf) bytearray(b'1\x00\x00\x00\x00\x00\x00\x00\x00\x00') That is, because we passed in a memoryview slice of the original buffer instead of a temporary slice of C memory, we can now stash that slice into a global, modify it, and see those modifications reflected back to the overall bytearray object returned by pread_structured. The generated diff is a bit more verbose, including some no-op lines added into pread, aio_pread, and aio_pread_structured. In practice, only pread_structured is actually impacted. | --- python/methods.c.bak 2022-05-31 09:51:30.406757934 -0500 | +++ python/methods.c 2022-05-31 10:26:01.797636894 -0500 | @@ -27,6 +27,7 @@ | #include <stdlib.h> | #include <stdint.h> | #include <stdbool.h> | +#include <stddef.h> | | #include <libnbd.h> | | @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) { | + PyObject *start, *end, *slice; | + assert (PyMemoryView_Check (data->buf)); | + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf; | + start = PyLong_FromLong (offs); | + if (!start) { PyErr_PrintEx (0); goto out; } | + end = PyLong_FromLong (offs + count); | + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; } | + slice = PySlice_New (start, end, NULL); | + Py_DECREF (start); | + Py_DECREF (end); | + if (!slice) { PyErr_PrintEx (0); goto out; } | + py_subbuf = PyObject_GetItem (data->buf, slice); | + } | + else { | + /* Cast away const */ | + py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + } | if (!py_subbuf) { PyErr_PrintEx (0); goto out; } | PyObject *py_error_modname = PyUnicode_FromString ("ctypes"); | if (!py_error_modname) { PyErr_PrintEx (0); goto out; } | @@ -115,11 +132,11 @@ chunk_wrapper (void *user_data, const vo | }; | | out: | - if (py_subbuf) { | + if (py_subbuf && !data->buf) { | PyObject *tmp = PyObject_CallMethod(py_subbuf, "release", NULL); | Py_XDECREF (tmp); | - Py_DECREF (py_subbuf); | } | + Py_XDECREF (py_subbuf); | if (py_error) { | PyObject *py_error_ret = PyObject_GetAttrString (py_error, "value"); | *error = PyLong_AsLong (py_error_ret); | @@ -2304,7 +2321,7 @@ nbd_internal_py_pread (PyObject *self, P | struct nbd_handle *h; | int ret; | PyObject *py_ret = NULL; | - PyObject *buf = NULL; | + PyObject *py_buf = NULL; | Py_ssize_t count; | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | @@ -2318,20 +2335,20 @@ nbd_internal_py_pread (PyObject *self, P | h = get_handle (py_h); | if (!h) goto out; | flags_u32 = flags; | - buf = PyByteArray_FromStringAndSize (NULL, count); | - if (buf == NULL) goto out; | + py_buf = PyByteArray_FromStringAndSize (NULL, count); | + if (py_buf == NULL) goto out; | offset_u64 = offset; | | - ret = nbd_pread (h, PyByteArray_AS_STRING (buf), count, offset_u64, flags_u32); | + ret = nbd_pread (h, PyByteArray_AS_STRING (py_buf), count, offset_u64, flags_u32); | if (ret == -1) { | raise_exception (); | goto out; | } | - py_ret = buf; | - buf = NULL; | + py_ret = py_buf; | + py_buf = NULL; | | out: | - Py_XDECREF (buf); | + Py_XDECREF (py_buf); | return py_ret; | } | | @@ -2342,7 +2359,7 @@ nbd_internal_py_pread_structured (PyObje | struct nbd_handle *h; | int ret; | PyObject *py_ret = NULL; | - PyObject *buf = NULL; | + PyObject *py_buf = NULL; | Py_ssize_t count; | uint64_t offset_u64; | unsigned long long offset; /* really uint64_t */ | @@ -2360,8 +2377,8 @@ nbd_internal_py_pread_structured (PyObje | h = get_handle (py_h); | if (!h) goto out; | flags_u32 = flags; | - buf = PyByteArray_FromStringAndSize (NULL, count); | - if (buf == NULL) goto out; | + py_buf = PyByteArray_FromStringAndSize (NULL, count); | + if (py_buf == NULL) goto out; | offset_u64 = offset; | chunk.user_data = chunk_user_data = alloc_user_data (); | if (chunk_user_data == NULL) goto out; | @@ -2373,18 +2390,20 @@ nbd_internal_py_pread_structured (PyObje | /* Increment refcount since pointer may be saved by libnbd. */ | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | + if (py_buf) | + chunk_user_data->buf = PyMemoryView_FromObject (py_buf); | | - ret = nbd_pread_structured (h, PyByteArray_AS_STRING (buf), count, offset_u64, chunk, flags_u32); | + ret = nbd_pread_structured (h, PyByteArray_AS_STRING (py_buf), count, offset_u64, chunk, flags_u32); | chunk_user_data = NULL; | if (ret == -1) { | raise_exception (); | goto out; | } | - py_ret = buf; | - buf = NULL; | + py_ret = py_buf; | + py_buf = NULL; | | out: | - Py_XDECREF (buf); | + Py_XDECREF (py_buf); | free_user_data (chunk_user_data); | return py_ret; | } | @@ -3171,6 +3190,7 @@ nbd_internal_py_aio_pread (PyObject *sel | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | + PyObject *py_buf = NULL; | PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | struct py_aio_buffer *buf_buf; | uint64_t offset_u64; | @@ -3230,6 +3250,7 @@ nbd_internal_py_aio_pread_structured (Py | struct nbd_handle *h; | int64_t ret; | PyObject *py_ret = NULL; | + PyObject *py_buf = NULL; | PyObject *buf; /* PyCapsule pointing to struct py_aio_buffer */ | struct py_aio_buffer *buf_buf; | uint64_t offset_u64; | @@ -3282,6 +3303,8 @@ nbd_internal_py_aio_pread_structured (Py | /* Increment refcount since pointer may be saved by libnbd. */ | Py_INCREF (py_chunk_fn); | chunk_user_data->fn = py_chunk_fn; | + if (py_buf) | + chunk_user_data->buf = PyMemoryView_FromObject (py_buf); | | ret = nbd_aio_pread_structured (h, buf_buf->data, buf_buf->len, offset_u64, chunk, completion, flags_u32); | chunk_user_data = NULL; --- generator/Python.ml | 60 +++++++++++++++++++++++++++++++++------------ 1 file changed, 44 insertions(+), 16 deletions(-) diff --git a/generator/Python.ml b/generator/Python.ml index 0191f79..4160759 100644 --- a/generator/Python.ml +++ b/generator/Python.ml @@ -187,8 +187,24 @@ let pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" | CBBytesIn (n, len) -> - pr " /* Cast away const */\n"; - pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (data->buf) {\n"; + pr " PyObject *start, *end, *slice;\n"; + pr " assert (PyMemoryView_Check (data->buf));\n"; + pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER (data->buf)->buf;\n" n; + pr " start = PyLong_FromLong (offs);\n"; + pr " if (!start) { PyErr_PrintEx (0); goto out; }\n"; + pr " end = PyLong_FromLong (offs + %s);\n" len; + pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }\n"; + pr " slice = PySlice_New (start, end, NULL);\n"; + pr " Py_DECREF (start);\n"; + pr " Py_DECREF (end);\n"; + pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n"; + pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n; + pr " }\n"; + pr " else {\n"; + pr " /* Cast away const */\n"; + pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " }\n"; pr " if (!py_%s) { PyErr_PrintEx (0); goto out; }\n" n | CBInt _ | CBInt64 _ -> () @@ -272,11 +288,11 @@ let pr " }\n" | CBBytesIn (n, _) -> (* https://stackoverflow.com/questions/58870010/how-to-call-release-on-a-memory... *) - pr " if (py_%s) {\n" n; + pr " if (py_%s && !data->buf) {\n" n; pr " PyObject *tmp = PyObject_CallMethod(py_%s, \"release\", NULL);\n" n; pr " Py_XDECREF (tmp);\n"; - pr " Py_DECREF (py_%s);\n" n; - pr " }\n" + pr " }\n"; + pr " Py_XDECREF (py_%s);\n" n | CBInt _ | CBInt64 _ | CBString _ | CBUInt _ | CBUInt64 _ -> () @@ -288,6 +304,7 @@ let (* Generate the Python binding. *) let print_python_binding name { args; optargs; ret; may_set_error } = + let store_mem = ref false in pr "PyObject *\n"; pr "nbd_internal_py_%s (PyObject *self, PyObject *args)\n" name; pr "{\n"; @@ -301,13 +318,19 @@ let | BytesIn (n, _) -> pr " Py_buffer %s = { .obj = NULL };\n" n | BytesOut (n, count) -> - pr " PyObject *%s = NULL;\n" n; - pr " Py_ssize_t %s;\n" count - | BytesPersistIn (n, _) - | BytesPersistOut (n, _) -> + pr " PyObject *py_%s = NULL;\n" n; + pr " Py_ssize_t %s;\n" count; + store_mem := true + | BytesPersistIn (n, _) -> pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n" n; pr " struct py_aio_buffer *%s_buf;\n" n + | BytesPersistOut (n, _) -> + pr " PyObject *py_%s = NULL;\n" n; + pr " PyObject *%s; /* PyCapsule pointing to struct py_aio_buffer */\n" + n; + pr " struct py_aio_buffer *%s_buf;\n" n; + store_mem := true | Closure { cbname } -> pr " struct user_data *%s_user_data = NULL;\n" cbname; pr " PyObject *py_%s_fn;\n" cbname; @@ -440,8 +463,8 @@ let | Bool _ -> () | BytesIn _ -> () | BytesOut (n, count) -> - pr " %s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count; - pr " if (%s == NULL) goto out;\n" n + pr " py_%s = PyByteArray_FromStringAndSize (NULL, %s);\n" n count; + pr " if (py_%s == NULL) goto out;\n" n | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> pr " %s_buf = nbd_internal_py_get_aio_buffer (%s);\n" n n; pr " if (!%s_buf) goto out;\n" n; @@ -458,7 +481,11 @@ let pr " }\n"; pr " /* Increment refcount since pointer may be saved by libnbd. */\n"; pr " Py_INCREF (py_%s_fn);\n" cbname; - pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname + pr " %s_user_data->fn = py_%s_fn;\n" cbname cbname; + if !store_mem then ( + pr " if (py_buf)\n"; + pr " %s_user_data->buf = PyMemoryView_FromObject (py_buf);\n" cbname + ) | Enum _ -> () | Flags (n, _) -> pr " %s_u32 = %s;\n" n n | Fd _ | Int _ -> () @@ -487,7 +514,7 @@ let function | Bool n -> pr ", %s" n | BytesIn (n, _) -> pr ", %s.buf, %s.len" n n - | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (%s), %s" n count + | BytesOut (n, count) -> pr ", PyByteArray_AS_STRING (py_%s), %s" n count | BytesPersistIn (n, _) | BytesPersistOut (n, _) -> pr ", %s_buf->data, %s_buf->len" n n | Closure { cbname } -> pr ", %s" cbname @@ -533,8 +560,8 @@ let List.iter ( function | BytesOut (n, _) -> - pr " py_ret = %s;\n" n; - pr " %s = NULL;\n" n; + pr " py_ret = py_%s;\n" n; + pr " py_%s = NULL;\n" n; use_ret := false | Bool _ | BytesIn _ @@ -581,7 +608,7 @@ let | BytesIn (n, _) -> pr " if (%s.obj)\n" n; pr " PyBuffer_Release (&%s);\n" n - | BytesOut (n, _) -> pr " Py_XDECREF (%s);\n" n + | BytesOut (n, _) -> pr " Py_XDECREF (py_%s);\n" n | BytesPersistIn _ | BytesPersistOut _ -> () | Closure { cbname } -> pr " free_user_data (%s_user_data);\n" cbname @@ -620,6 +647,7 @@ let pr "#include <stdlib.h>\n"; pr "#include <stdint.h>\n"; pr "#include <stdbool.h>\n"; + pr "#include <stddef.h>\n"; pr "\n"; pr "#include <libnbd.h>\n"; pr "\n"; -- 2.36.1
On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid.
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) { | + PyObject *start, *end, *slice; | + assert (PyMemoryView_Check (data->buf)); | + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf; | + start = PyLong_FromLong (offs); | + if (!start) { PyErr_PrintEx (0); goto out; } | + end = PyLong_FromLong (offs + count); | + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; } | + slice = PySlice_New (start, end, NULL); | + Py_DECREF (start); | + Py_DECREF (end); | + if (!slice) { PyErr_PrintEx (0); goto out; } | + py_subbuf = PyObject_GetItem (data->buf, slice);
Missing a Py_DECREF (slice) here. Easy enough to add...
+++ b/generator/Python.ml @@ -187,8 +187,24 @@ let pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" | CBBytesIn (n, len) -> - pr " /* Cast away const */\n"; - pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (data->buf) {\n"; + pr " PyObject *start, *end, *slice;\n"; + pr " assert (PyMemoryView_Check (data->buf));\n"; + pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER (data->buf)->buf;\n" n; + pr " start = PyLong_FromLong (offs);\n"; + pr " if (!start) { PyErr_PrintEx (0); goto out; }\n"; + pr " end = PyLong_FromLong (offs + %s);\n" len; + pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }\n"; + pr " slice = PySlice_New (start, end, NULL);\n"; + pr " Py_DECREF (start);\n"; + pr " Py_DECREF (end);\n"; + pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n"; + pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n;
...here And I really wish python didn't make it so hard to grab a slice of another object using C code. Having to create 3 temporary PyObjects instead of having a utility C function that takes normal integers was annoying. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, May 31, 2022 at 6:52 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid.
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) {
In which case we don't have data->buf?
| + PyObject *start, *end, *slice; | + assert (PyMemoryView_Check (data->buf));
Why do we need data->buf to be a memoryview? We can create a new memoryview form data->buf - it only needs to be an object supporting the buffer protocol. Basically we need the C version of: memoryview(buf)[start:stop] buf can be bytes, bytearray, mmap, or another memoryview.
| + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf;
Because PyMemoryView_Check is inside the assert, build with NDEBUG will remove the check, and this call may crash if data->buf is not a memoryview. It would be nicer if we could get the offset without looking into the internal buffer of the memoryview.
| + start = PyLong_FromLong (offs); | + if (!start) { PyErr_PrintEx (0); goto out; } | + end = PyLong_FromLong (offs + count); | + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; } | + slice = PySlice_New (start, end, NULL); | + Py_DECREF (start); | + Py_DECREF (end); | + if (!slice) { PyErr_PrintEx (0); goto out; } | + py_subbuf = PyObject_GetItem (data->buf, slice);
Missing a Py_DECREF (slice) here. Easy enough to add...
+++ b/generator/Python.ml @@ -187,8 +187,24 @@ let pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" | CBBytesIn (n, len) -> - pr " /* Cast away const */\n"; - pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (data->buf) {\n"; + pr " PyObject *start, *end, *slice;\n"; + pr " assert (PyMemoryView_Check (data->buf));\n"; + pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER (data->buf)->buf;\n" n; + pr " start = PyLong_FromLong (offs);\n"; + pr " if (!start) { PyErr_PrintEx (0); goto out; }\n"; + pr " end = PyLong_FromLong (offs + %s);\n" len; + pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }\n"; + pr " slice = PySlice_New (start, end, NULL);\n"; + pr " Py_DECREF (start);\n"; + pr " Py_DECREF (end);\n"; + pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n"; + pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n;
...here
And I really wish python didn't make it so hard to grab a slice of another object using C code. Having to create 3 temporary PyObjects instead of having a utility C function that takes normal integers was annoying.
Does this work? PySlice_New(NULL, NULL, NULL); PySlice_AdjustIndices(length, start, stop, step); Nir
On Wed, Jun 01, 2022 at 02:20:48AM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 6:52 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid.
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) {
In which case we don't have data->buf?
Right now, in nbd_internal_py_aio_read_structured. Fixing that will eventually become patch 4/2 for this series (the idea is that instead of requiring the user to pass in an nbd.Buffer object, we should take any buffer-like object, populate data->buf with zero-copy semantics, and we're good to go. But to avoid breaking back-compat, we either have to also special-case existing code using nbd.Buffer, or enhance the nbd.Buffer class to implement the buffer-like interface).
| + PyObject *start, *end, *slice; | + assert (PyMemoryView_Check (data->buf));
Why do we need data->buf to be a memoryview?
Maybe it doesn't. It looks like (at least from python, rather than in the C coding side of things) that you can apply the slice operation to bytes and bytearray. But there may be other buffer-like objects that don't directly support slice while memoryview always does; and one of the reasons memoryview is part of the standard python library is to make it easy to add operations on top of any buffer-like object. memoryview also takes care of doing a temporary copy to consolidate out a contiguous view if the original buffer is not contiguous; you don't need that with bytes or bytearray, but definitely need it with array.array and friends.
We can create a new memoryview form data->buf - it only needs to be an object supporting the buffer protocol. Basically we need the C version of:
memoryview(buf)[start:stop]
buf can be bytes, bytearray, mmap, or another memoryview.
Yes - and that's what this C code is. memoryview(buf) was created when populating data->buf (whether the original was bytes, bytearray, mmap, or other buffer-like object), then this code follows up with creating the slice [start:stop] then pulling it altogether with view.__getitem__(slice).
| + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf;
Because PyMemoryView_Check is inside the assert, build with NDEBUG will remove the check, and this call may crash if data->buf is not a memoryview.
It's more a proof that the earlier code in nbd_internal_py_pread_structured correctly set data->buf. I'm not worried about an NDEBUG build failing; this is one case where an assert() really is more for documentation.
It would be nicer if we could get the offset without looking into the internal buffer of the memoryview.
We could pass the void* alongside the PyObject* in struct user_data. But ultimately, we HAVE to look at the internal pointer of the memoryview, in order to call nbd_pread_structured in the first place. Whether we look once (and populate two fields in data) or every time the callback is reached is less important. It would also be possible to store the original offset to nbd_pread_structured in user_data, then compute the slice start as the callback's offset - original offset, without having to do pointer math. But we still have to store the memoryview PyObject that we will be slicing. So it just becomes a question of whether struct user_data needs to grow.
| + start = PyLong_FromLong (offs); | + if (!start) { PyErr_PrintEx (0); goto out; } | + end = PyLong_FromLong (offs + count); | + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; } | + slice = PySlice_New (start, end, NULL); | + Py_DECREF (start); | + Py_DECREF (end); | + if (!slice) { PyErr_PrintEx (0); goto out; } | + py_subbuf = PyObject_GetItem (data->buf, slice);
Missing a Py_DECREF (slice) here. Easy enough to add...
+++ b/generator/Python.ml @@ -187,8 +187,24 @@ let pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" | CBBytesIn (n, len) -> - pr " /* Cast away const */\n"; - pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (data->buf) {\n"; + pr " PyObject *start, *end, *slice;\n"; + pr " assert (PyMemoryView_Check (data->buf));\n"; + pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER (data->buf)->buf;\n" n; + pr " start = PyLong_FromLong (offs);\n"; + pr " if (!start) { PyErr_PrintEx (0); goto out; }\n"; + pr " end = PyLong_FromLong (offs + %s);\n" len; + pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }\n"; + pr " slice = PySlice_New (start, end, NULL);\n"; + pr " Py_DECREF (start);\n"; + pr " Py_DECREF (end);\n"; + pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n"; + pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n;
...here
And I really wish python didn't make it so hard to grab a slice of another object using C code. Having to create 3 temporary PyObjects instead of having a utility C function that takes normal integers was annoying.
Does this work?
PySlice_New(NULL, NULL, NULL); PySlice_AdjustIndices(length, start, stop, step);
New in python 3.6.1. README says we still target python 3.3. Worse, look at the signature - it does NOT take a PyObject *, and therefore it cannot modify an existing slice object (rather, it is for easing the computation of how to turn [-1:-2:2] into a slice with positive bounds) - you still have to go through a step that converts integers into PyObject* before creating a PySlice object. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, May 31, 2022 at 07:24:03PM -0500, Eric Blake wrote:
On Wed, Jun 01, 2022 at 02:20:48AM +0300, Nir Soffer wrote:
Does this work?
PySlice_New(NULL, NULL, NULL); PySlice_AdjustIndices(length, start, stop, step);
New in python 3.6.1. README says we still target python 3.3.
There are two things we could do here. Either move the baseline up. Python 3.6.1 was released in March 2017 (5 years ago), and more importantly is available in RHEL 7 which is the oldest Linux distro we care about. Or (harder, not necessarily better) detect the required function in configure.ac, and add a utility function to hide the difference.
Worse, look at the signature - it does NOT take a PyObject *, and therefore it cannot modify an existing slice object (rather, it is for easing the computation of how to turn [-1:-2:2] into a slice with positive bounds) - you still have to go through a step that converts integers into PyObject* before creating a PySlice object.
Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
On Wed, Jun 1, 2022 at 3:24 AM Eric Blake <eblake@redhat.com> wrote:
On Wed, Jun 01, 2022 at 02:20:48AM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 6:52 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid.
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) {
In which case we don't have data->buf?
Right now, in nbd_internal_py_aio_read_structured. Fixing that will eventually become patch 4/2 for this series (the idea is that instead of requiring the user to pass in an nbd.Buffer object, we should take any buffer-like object, populate data->buf with zero-copy semantics, and we're good to go.
Avoiding the nbd.Buffer class and using the buffer protocol sounds like the way to go.
But to avoid breaking back-compat, we either have to also special-case existing code using nbd.Buffer, or enhance the nbd.Buffer class to implement the buffer-like interface).
Backward compatibility is very important, but I'm not sure if we have enough users of the python bindings to care about it now.
| + PyObject *start, *end, *slice; | + assert (PyMemoryView_Check (data->buf));
Why do we need data->buf to be a memoryview?
Maybe it doesn't. It looks like (at least from python, rather than in the C coding side of things) that you can apply the slice operation to bytes and bytearray. But there may be other buffer-like objects that don't directly support slice while memoryview always does; and one of the reasons memoryview is part of the standard python library is to make it easy to add operations on top of any buffer-like object. memoryview also takes care of doing a temporary copy to consolidate out a contiguous view if the original buffer is not contiguous; you don't need that with bytes or bytearray, but definitely need it with array.array and friends.
In the python side, the reason you need memoryview is to avoid the copy when creating a slice. I guess the C API works in the same way but I did not check.
We can create a new memoryview form data->buf - it only needs to be an object supporting the buffer protocol. Basically we need the C version of:
memoryview(buf)[start:stop]
buf can be bytes, bytearray, mmap, or another memoryview.
Yes - and that's what this C code is. memoryview(buf) was created when populating data->buf (whether the original was bytes, bytearray, mmap, or other buffer-like object), then this code follows up with creating the slice [start:stop] then pulling it altogether with view.__getitem__(slice).
Sounds good! I think Richard's suggestion to extract a helper for slicing a memoryview will make this code much easier to read and review.
| + ptrdiff_t offs = subbuf - PyMemoryView_GET_BUFFER (data->buf)->buf;
Because PyMemoryView_Check is inside the assert, build with NDEBUG will remove the check, and this call may crash if data->buf is not a memoryview.
It's more a proof that the earlier code in nbd_internal_py_pread_structured correctly set data->buf. I'm not worried about an NDEBUG build failing; this is one case where an assert() really is more for documentation.
This looks like a coding error as is. We can add a comment or change the code to look more correct. If we really don't have a memory view, the slice will copy the data (unless C level slices are unsafe, unlikely), and we want to get a view without coping. So it would be better to fail loudly.
It would be nicer if we could get the offset without looking into the internal buffer of the memoryview.
We could pass the void* alongside the PyObject* in struct user_data. But ultimately, we HAVE to look at the internal pointer of the memoryview, in order to call nbd_pread_structured in the first place. Whether we look once (and populate two fields in data) or every time the callback is reached is less important.
It would also be possible to store the original offset to nbd_pread_structured in user_data, then compute the slice start as the callback's offset - original offset, without having to do pointer math. But we still have to store the memoryview PyObject that we will be slicing. So it just becomes a question of whether struct user_data needs to grow.
Maybe add a macro or inline function to return the offset?
| + start = PyLong_FromLong (offs); | + if (!start) { PyErr_PrintEx (0); goto out; } | + end = PyLong_FromLong (offs + count); | + if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; } | + slice = PySlice_New (start, end, NULL); | + Py_DECREF (start); | + Py_DECREF (end); | + if (!slice) { PyErr_PrintEx (0); goto out; } | + py_subbuf = PyObject_GetItem (data->buf, slice);
Missing a Py_DECREF (slice) here. Easy enough to add...
+++ b/generator/Python.ml @@ -187,8 +187,24 @@ let pr " PyList_SET_ITEM (py_%s, i_%s, py_e_%s);\n" n n n; pr " }\n" | CBBytesIn (n, len) -> - pr " /* Cast away const */\n"; - pr " py_%s = PyMemoryView_FromMemory ((char *) %s, %s, PyBUF_READ);\n" n n len; + pr " if (data->buf) {\n"; + pr " PyObject *start, *end, *slice;\n"; + pr " assert (PyMemoryView_Check (data->buf));\n"; + pr " ptrdiff_t offs = %s - PyMemoryView_GET_BUFFER (data->buf)->buf;\n" n; + pr " start = PyLong_FromLong (offs);\n"; + pr " if (!start) { PyErr_PrintEx (0); goto out; }\n"; + pr " end = PyLong_FromLong (offs + %s);\n" len; + pr " if (!end) { Py_DECREF (start); PyErr_PrintEx (0); goto out; }\n"; + pr " slice = PySlice_New (start, end, NULL);\n"; + pr " Py_DECREF (start);\n"; + pr " Py_DECREF (end);\n"; + pr " if (!slice) { PyErr_PrintEx (0); goto out; }\n"; + pr " py_%s = PyObject_GetItem (data->buf, slice);\n" n;
...here
And I really wish python didn't make it so hard to grab a slice of another object using C code. Having to create 3 temporary PyObjects instead of having a utility C function that takes normal integers was annoying.
Does this work?
PySlice_New(NULL, NULL, NULL); PySlice_AdjustIndices(length, start, stop, step);
New in python 3.6.1. README says we still target python 3.3. Worse, look at the signature - it does NOT take a PyObject *, and therefore it cannot modify an existing slice object (rather, it is for easing the computation of how to turn [-1:-2:2] into a slice with positive bounds) - you still have to go through a step that converts integers into PyObject* before creating a PySlice object.
Correct, I was confused by the name. Nir
On Tue, May 31, 2022 at 07:24:03PM -0500, Eric Blake wrote:
On Wed, Jun 01, 2022 at 02:20:48AM +0300, Nir Soffer wrote:
On Tue, May 31, 2022 at 6:52 PM Eric Blake <eblake@redhat.com> wrote:
On Tue, May 31, 2022 at 10:49:03AM -0500, Eric Blake wrote:
This patch fixes the corner-case regression introduced by the previous patch where the pread_structured callback buffer lifetime ends as soon as the callback (that is, where accessing a stashed callback parameter can result in ValueError instead of modifying a harmless copy). With careful effort, we can create a memoryview of the Python object that we read into, then slice that memoryview as part of the callback; now the slice will be valid as long as the original object is also valid.
| @@ -76,8 +77,24 @@ chunk_wrapper (void *user_data, const vo | PyObject *py_subbuf = NULL; | PyObject *py_error = NULL; | | - /* Cast away const */ | - py_subbuf = PyMemoryView_FromMemory ((char *) subbuf, count, PyBUF_READ); | + if (data->buf) {
In which case we don't have data->buf?
Right now, in nbd_internal_py_aio_read_structured. Fixing that will eventually become patch 4/2 for this series (the idea is that instead of requiring the user to pass in an nbd.Buffer object, we should take any buffer-like object, populate data->buf with zero-copy semantics, and we're good to go. But to avoid breaking back-compat, we either have to also special-case existing code using nbd.Buffer, or enhance the nbd.Buffer class to implement the buffer-like interface).
Following up on this, after first reworking aio_pread to be more efficient in other series that landed first, I was able to respin this series to no longer care about pread_structured vs. aio_pread_structured, and with no temporary regression in being unable to stash off information that survives past the callback: https://listman.redhat.com/archives/libguestfs/2022-June/029148.html I'm still working on patches to make nbd.Buffer not do as much copying; right now, I'm leaning towards this design: Add nbd.Buffer.copy_default, set to False. Enhance existing functions along the lines of: @classmethod def nbd.Buffer.from_bytearray(cls, buffer, copy=None): if copy is None: copy = nbd.Buffer.copy_default if copy: buffer = bytearray(buffer) self = cls(0) self._o = buffer self._init = True return self def nbd.Buffer.to_bytearray(self, copy=None): if copy is None: copy = self.copy_default # falls back to nbd.Buffer.copy_default as needed if not hasattr(self, '_init'): self._o = bytearray(len(self)) self._init = True if copy: return bytearray(self._o) return self._o With this design, newer libnbd clients default to zero-copy, and can request a copy when needed; while code that wants to be portable to older and newer libnbd at the same time can start out by doing: nbd.Buffer.copy_default = True at initialization time (since they can't pass a copy= parameter to .{to,from}_bytearray() in older libnbd). -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, May 31, 2022 at 10:52:35AM -0500, Eric Blake wrote:
And I really wish python didn't make it so hard to grab a slice of another object using C code. Having to create 3 temporary PyObjects instead of having a utility C function that takes normal integers was annoying.
You can add hand-written utility functions to python/utils.c if that makes things easier. Note you will need to add a prototype for the new function(s) to the generated code in python/methods.h. See nbd_internal_py_get_string_list for an example of this. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org
I agree with Nir's feedback and R-b's and don't have anything else to add. 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
participants (3)
-
Eric Blake -
Nir Soffer -
Richard W.M. Jones