8:21 p.m.
When I added .can_FOO caching in 1.16, I missed the case that the sh
plugin itself was calling .can_flush twice in some situations (in
order to default .can_fua). Then right after, I regressed it to call
.can_zero twice (in order to default .can_fast_zero). I also missed
that .thread_model could use better caching, because at the time, I
did not add testsuite coverage. Fix that now.
Eric Blake (2):
sh, eval: Cache .can_zero and .can_flush
server: Better caching of .thread_model
server/internal.h | 1 +
server/connections.c | 8 ++-
server/filters.c | 8 +--
server/locks.c | 7 ++-
server/plugins.c | 12 ++---
server/protocol-handshake.c | 5 +-
server/sockets.c | 3 +-
plugins/sh/methods.c | 102 +++++++++++++++++++++++-------------
tests/test-eflags.sh | 42 +++++++++++++--
9 files changed, 123 insertions(+), 65 deletions(-)
--
2.25.1
8:21 p.m.
New subject: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
In commit c306fa93ab and neighbors (v1.15.1), a concerted effort went
into caching the results of .can_FOO callbacks, with commit messages
demonstrating that a plugin with a slow callback should not have that
delay magnified multiple times. But nothing was added to the
testsuite at the time, and with the sh and eval plugins, we still have
two scenarios where we did not take advantage of the nbdkit cache
because we directly called things ourselves (one pre-existing since
v1.13.9, another introduced in v1.15.1 right after the cleanups). Fix
those spots by reworking the handle we pass to nbdkit, then enhance
test-eflags.sh to ensure we don't regress again.
Note that nbdkit does not yet cache .thread_model; that will be
addressed in the next patch. Furthermore, we end up duplicating the
caching that nbdkit itself does, because it would be a layering
violation for us to have enough information to call into
backend_can_flush().
Fixes: 05c5eed6f2
Fixes: 8490694d08
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
plugins/sh/methods.c | 102 +++++++++++++++++++++++++++----------------
tests/test-eflags.sh | 44 +++++++++++++++++--
2 files changed, 105 insertions(+), 41 deletions(-)
diff --git a/plugins/sh/methods.c b/plugins/sh/methods.c
index 56e2d410..cce143de 100644
--- a/plugins/sh/methods.c
+++ b/plugins/sh/methods.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -193,29 +193,42 @@ sh_preconnect (int readonly)
}
}
+struct sh_handle {
+ int can_flush;
+ int can_zero;
+ char *h;
+};
+
void *
sh_open (int readonly)
{
const char *method = "open";
const char *script = get_script (method);
- char *h = NULL;
size_t hlen;
const char *args[] =
{ script, method,
readonly ? "true" : "false",
nbdkit_export_name () ? : "",
NULL };
+ struct sh_handle *h = malloc (sizeof *h);
+
+ if (!h) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+ h->can_flush = -1;
+ h->can_zero = -1;
/* We store the string returned by open in the handle. */
- switch (call_read (&h, &hlen, args)) {
+ switch (call_read (&h->h, &hlen, args)) {
case OK:
/* Remove final newline if present. */
- if (hlen > 0 && h[hlen-1] == '\n') {
- h[hlen-1] = '\0';
+ if (hlen > 0 && h->h[hlen-1] == '\n') {
+ h->h[hlen-1] = '\0';
hlen--;
}
if (hlen > 0)
- nbdkit_debug ("sh: handle: %s", h);
+ nbdkit_debug ("sh: handle: %s", h->h);
return h;
case MISSING:
@@ -223,17 +236,19 @@ sh_open (int readonly)
* missing then we return "" as the handle. Allocate a new string
* for it because we don't know what call_read returned here.
*/
- free (h);
- h = strdup ("");
- if (h == NULL)
+ free (h->h);
+ h->h = strdup ("");
+ if (h->h == NULL)
nbdkit_error ("strdup: %m");
return h;
case ERROR:
+ free (h->h);
free (h);
return NULL;
case RET_FALSE:
+ free (h->h);
free (h);
nbdkit_error ("%s: %s method returned unexpected code (3/false)",
script, method);
@@ -249,14 +264,15 @@ sh_close (void *handle)
{
const char *method = "close";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK:
case MISSING:
case ERROR:
case RET_FALSE:
+ free (h->h);
free (h);
return;
default: abort ();
@@ -268,8 +284,8 @@ sh_get_size (void *handle)
{
const char *method = "get_size";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int64_t r;
@@ -307,9 +323,9 @@ sh_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
{
const char *method = "pread";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, NULL };
CLEANUP_FREE char *data = NULL;
size_t len;
@@ -395,9 +411,9 @@ sh_pwrite (void *handle, const void *buf, uint32_t count, uint64_t
offset,
{
const char *method = "pwrite";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -429,8 +445,8 @@ static int
boolean_method (const char *script, const char *method,
void *handle, int def)
{
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK: /* true */
@@ -457,8 +473,14 @@ int
sh_can_flush (void *handle)
{
const char *method = "can_flush";
- const char *script = get_script (method);
- return boolean_method (script, method, handle, 0);
+ const char *script;
+ struct sh_handle *h = handle;
+
+ if (h->can_flush >= 0)
+ return h->can_flush;
+
+ script = get_script (method);
+ return h->can_flush = boolean_method (script, method, handle, 0);
}
int
@@ -481,8 +503,14 @@ int
sh_can_zero (void *handle)
{
const char *method = "can_zero";
- const char *script = get_script (method);
- return boolean_method (script, method, handle, 0);
+ const char *script;
+ struct sh_handle *h = handle;
+
+ if (h->can_zero >= 0)
+ return h->can_zero;
+
+ script = get_script (method);
+ return h->can_zero = boolean_method (script, method, handle, 0);
}
int
@@ -507,8 +535,8 @@ sh_can_fua (void *handle)
{
const char *method = "can_fua";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -562,8 +590,8 @@ sh_can_cache (void *handle)
{
const char *method = "can_cache";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -627,8 +655,8 @@ sh_flush (void *handle, uint32_t flags)
{
const char *method = "flush";
const char *script = get_script (method);
- char *h = handle;
- const char *args[] = { script, method, h, NULL };
+ struct sh_handle *h = handle;
+ const char *args[] = { script, method, h->h, NULL };
switch (call (args)) {
case OK:
@@ -656,9 +684,9 @@ sh_trim (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "trim";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -690,9 +718,9 @@ sh_zero (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "zero";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
@@ -795,9 +823,9 @@ sh_extents (void *handle, uint32_t count, uint64_t offset, uint32_t
flags,
{
const char *method = "extents";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32], fbuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, fbuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, fbuf, NULL };
CLEANUP_FREE char *s = NULL;
size_t slen;
int r;
@@ -840,9 +868,9 @@ sh_cache (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
{
const char *method = "cache";
const char *script = get_script (method);
- char *h = handle;
+ struct sh_handle *h = handle;
char cbuf[32], obuf[32];
- const char *args[] = { script, method, h, cbuf, obuf, NULL };
+ const char *args[] = { script, method, h->h, cbuf, obuf, NULL };
snprintf (cbuf, sizeof cbuf, "%" PRIu32, count);
snprintf (obuf, sizeof obuf, "%" PRIu64, offset);
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 9b3a6a3a..3228bbf6 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -1,6 +1,6 @@
#!/usr/bin/env bash
# nbdkit
-# Copyright (C) 2018-2019 Red Hat Inc.
+# Copyright (C) 2018-2020 Red Hat Inc.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are
@@ -50,7 +50,7 @@ if ! qemu-nbd --help | grep -sq -- --list; then
exit 77
fi
-files="eflags.out"
+files="eflags.out eflags.err"
late_args=
rm -f $files
cleanup_fn rm -f $files
@@ -72,12 +72,29 @@ SEND_FAST_ZERO=$(( 1 << 11 ))
do_nbdkit ()
{
- nbdkit -v -U - "$@" sh - $late_args --run 'qemu-nbd --list -k
$unixsocket' |
- grep -E "flags: 0x" | grep -Eoi '0x[a-f0-9]+' > eflags.out
+ # Prepend a check for internal caching to the script on stdin.
+ { printf %s '
+ if test $1 = thread_model; then
+ :
+ elif test -f $tmpdir/seen_$1; then
+ echo "repeat call to $1"
>>'"$PWD/eflags.err"'
+ else
+ touch $tmpdir/seen_$1
+ fi
+ '; cat; } | nbdkit -v -U - "$@" sh - $late_args \
+ --run 'qemu-nbd --list -k $unixsocket' |
+ grep -E "flags: 0x" | grep -Eoi '0x[a-f0-9]+' >eflags.out
2>eflags.err
printf eflags=; cat eflags.out
# Convert hex flags to decimal and assign it to $eflags.
eflags=$(printf "%d" $(cat eflags.out))
+
+ # See if nbdkit failed to cache a callback.
+ if test -s eflags.err; then
+ echo "error: nbdkit did not cache callbacks properly"
+ cat eflags.err
+ exit 1
+ fi
}
fail ()
@@ -327,6 +344,25 @@ EOF
[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_DF )) ] ||
fail "$LINENO: expected HAS_FLAGS|READ_ONLY|SEND_DF"
+#----------------------------------------------------------------------
+# -r
+# can_write=true
+# can_flush=true
+#
+# Setting read-only does not ignore can_flush.
+
+do_nbdkit -r <<'EOF'
+case "$1" in
+ get_size) echo 1M ;;
+ can_write) exit 0 ;;
+ can_flush) exit 0 ;;
+ *) exit 2 ;;
+esac
+EOF
+
+[ $eflags -eq $(( HAS_FLAGS|READ_ONLY|SEND_FLUSH|SEND_DF )) ] ||
+ fail "$LINENO: expected HAS_FLAGS|READ_ONLY|SEND_FLUSH|SEND_DF"
+
#----------------------------------------------------------------------
# can_write=true
# can_flush=true
--
2.25.1
7:09 a.m.
New subject: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On 3/18/20 8:21 PM, Eric Blake wrote:
In commit c306fa93ab and neighbors (v1.15.1), a concerted effort
went
into caching the results of .can_FOO callbacks, with commit messages
demonstrating that a plugin with a slow callback should not have that
delay magnified multiple times. But nothing was added to the
testsuite at the time, and with the sh and eval plugins, we still have
two scenarios where we did not take advantage of the nbdkit cache
because we directly called things ourselves (one pre-existing since
v1.13.9, another introduced in v1.15.1 right after the cleanups). Fix
those spots by reworking the handle we pass to nbdkit, then enhance
test-eflags.sh to ensure we don't regress again.
Note that nbdkit does not yet cache .thread_model; that will be
addressed in the next patch. Furthermore, we end up duplicating the
caching that nbdkit itself does, because it would be a layering
violation for us to have enough information to call into
backend_can_flush().
Fixes: 05c5eed6f2
Fixes: 8490694d08
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
@@ -457,8 +473,14 @@ int
sh_can_flush (void *handle)
{
const char *method = "can_flush";
- const char *script = get_script (method);
- return boolean_method (script, method, handle, 0);
+ const char *script;
+ struct sh_handle *h = handle;
+
+ if (h->can_flush >= 0)
+ return h->can_flush;
+
+ script = get_script (method);
+ return h->can_flush = boolean_method (script, method, handle, 0);
}
Thinking about this more, maybe the real problem is that all language
bindings that have to wrap scripts (OCaml and Rust are exceptions as
they directly call into the C code; but lua, perl, python, ruby, tcl,
and sh all fit into the category I'm describing) MUST define .can_FOO at
the C level because they don't know a priori whether the script they
will be loading will also provide a .can_FOO callback, so we have a lot
of duplicated code in each language binding.
Life might be easier if we change the C contract for the .can_FOO
callbacks to support the special return value of -2 to behave the same
as if .can_FOO were missing. In a way, it would make it easier for the
remaining languages to behave more like sh's special MISSING return
value (sh is an oddball because we can't even probe whether .FOO is
missing so .can_FOO is mandatory in the script; but in eval, python,
etc, it's easy to implement a C binding for .can_FOO that introspects
the script). I'll spin up a v2 patch along those lines, so we can
compare the difference.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
2:08 p.m.
New subject: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On Thu, Mar 19, 2020 at 07:09:32AM -0500, Eric Blake wrote:
Thinking about this more, maybe the real problem is that all
language bindings that have to wrap scripts (OCaml and Rust are
exceptions as they directly call into the C code; but lua, perl,
python, ruby, tcl, and sh all fit into the category I'm describing)
MUST define .can_FOO at the C level because they don't know a priori
whether the script they will be loading will also provide a .can_FOO
callback, so we have a lot of duplicated code in each language
binding.
Life might be easier if we change the C contract for the .can_FOO
callbacks to support the special return value of -2 to behave the
same as if .can_FOO were missing. In a way, it would make it easier
for the remaining languages to behave more like sh's special MISSING
return value (sh is an oddball because we can't even probe whether
.FOO is missing so .can_FOO is mandatory in the script; but in eval,
python, etc, it's easy to implement a C binding for .can_FOO that
introspects the script). I'll spin up a v2 patch along those lines,
so we can compare the difference.
What problem are we actually trying to solve here? If it calls the
can_* functions in the plugin more than once it's not a big deal.
Rich.
--
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-top is 'top' for virtual machines. Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top
2:54 p.m.
New subject: [nbdkit PATCH 1/2] sh, eval: Cache .can_zero and .can_flush
On 3/19/20 2:08 PM, Richard W.M. Jones wrote:
On Thu, Mar 19, 2020 at 07:09:32AM -0500, Eric Blake wrote:
> Thinking about this more, maybe the real problem is that all
> language bindings that have to wrap scripts (OCaml and Rust are
> exceptions as they directly call into the C code; but lua, perl,
> python, ruby, tcl, and sh all fit into the category I'm describing)
> MUST define .can_FOO at the C level because they don't know a priori
> whether the script they will be loading will also provide a .can_FOO
> callback, so we have a lot of duplicated code in each language
> binding.
>
> Life might be easier if we change the C contract for the .can_FOO
> callbacks to support the special return value of -2 to behave the
> same as if .can_FOO were missing. In a way, it would make it easier
> for the remaining languages to behave more like sh's special MISSING
> return value (sh is an oddball because we can't even probe whether
> .FOO is missing so .can_FOO is mandatory in the script; but in eval,
> python, etc, it's easy to implement a C binding for .can_FOO that
> introspects the script). I'll spin up a v2 patch along those lines,
> so we can compare the difference.
What problem are we actually trying to solve here? If it calls the
can_* functions in the plugin more than once it's not a big deal.
Below is what I ended up with as a potential v2 after a bit of playing.
It's smaller than the patch series we already reviewed, but it fails the
test-eflags.sh additions (basically, it caused the sh plugin to always
advertise FUA support when .can_fua was not implemented in the script,
which ends up being different than the behavior for C plugins where FUA
is advertised only when .flush exists). And anything else I can think
of to fix that change in behavior is just going to add more code,
compared to the ease of the fix of having the sh plugin do a bit of
caching itself. So at this point, I'm ditching any further ideas to
change plugins.c, and going ahead and committing the v1 patches as
reviewed (with the change that I've now split the sh and python patches
into two parts: handle refactoring with no semantic change vs. adding a
cache variable into the handle).
For the record, this is an additional idea I thought of, but considered
too complex for the benefit: add a new callback .has_callback, with
signature:
bool has_callback(enum)
Then, anywhere that nbdkit currently does 'if (p->pwrite == NULL)', it
would instead check 'if (!p->has_callback(NBDKIT_CALLBACK_PWRITE))'. If
.has_callback itself is missing, nbdkit defaults to mapping each enum
value to the corresponding callback pointer in the C struct: a callback
is absent only when it is NULL. But with language bindings, they
provide an alternative .has_callback which can return false even when
the C struct has a non-NULL pointer, to more accurately reflect the
reality that even though the language binding has a wrapper function in
place, the wrapper determined that the end-user script is lacking an
override for that callback.
Back to the idea I had for v2:
diff --git i/docs/nbdkit-plugin.pod w/docs/nbdkit-plugin.pod
index 494d77f8..9a22c47d 100644
--- i/docs/nbdkit-plugin.pod
+++ w/docs/nbdkit-plugin.pod
@@ -631,7 +631,11 @@ if C<.zero> is absent or C<.can_zero> returns false
(in those cases,
nbdkit fails all fast zero requests, as its fallback to C<.pwrite> is
not inherently faster), otherwise false (since it cannot be determined
in advance if the plugin's C<.zero> will properly honor the semantics
-of C<NBDKIT_FLAG_FAST_ZERO>).
+of C<NBDKIT_FLAG_FAST_ZERO>). As an aid to plugins implementing
+bindings to another language, if this callback returns <-2>, then
+nbdkit will perform the same fallback as if C<.can_fast_zero> were
+missing, in order to provide a sane default based on the cached result
+of C<.can_zero>.
=head2 C<.can_extents>
@@ -668,7 +672,11 @@ error message and return C<-1>.
This callback is not required unless a plugin wants to specifically
handle FUA requests. If omitted, nbdkit checks whether C<.flush>
exists, and behaves as if this function returns C<NBDKIT_FUA_NONE> or
-C<NBDKIT_FUA_EMULATE> as appropriate.
+C<NBDKIT_FUA_EMULATE> as appropriate. As an aid to plugins
+implementing bindings to another language, if this callback returns
+<-2>, then nbdkit will perform the same fallback as if C<.can_fua>
+were missing, in order to provide a sane default based on the cached
+result of C<.can_flush>.
=head2 C<.can_multi_conn>
diff --git i/server/plugins.c w/server/plugins.c
index 708e1cfa..17a8eebd 100644
--- i/server/plugins.c
+++ w/server/plugins.c
@@ -388,8 +388,11 @@ plugin_can_fast_zero (struct backend *b, void *handle)
struct backend_plugin *p = container_of (b, struct backend_plugin,
backend);
int r;
- if (p->plugin.can_fast_zero)
- return normalize_bool (p->plugin.can_fast_zero (handle));
+ if (p->plugin.can_fast_zero) {
+ r = p->plugin.can_fast_zero (handle);
+ if (r != -2)
+ return normalize_bool (r);
+ }
/* Advertise support for fast zeroes if no .zero or .can_zero is
* false: in those cases, we fail fast instead of using .pwrite.
* This also works when v1 plugin has only ._zero_v1.
@@ -423,9 +426,22 @@ plugin_can_fua (struct backend *b, void *handle)
NBDKIT_FUA_NATIVE before we will pass the FUA flag on. */
if (p->plugin.can_fua) {
r = p->plugin.can_fua (handle);
- if (r > NBDKIT_FUA_EMULATE && p->plugin._api_version == 1)
- r = NBDKIT_FUA_EMULATE;
- return r;
+ switch (r) {
+ case NBDKIT_FUA_NATIVE:
+ if (p->plugin._api_version == 1)
+ return NBDKIT_FUA_EMULATE;
+ /* fallthrough */
+ case NBDKIT_FUA_EMULATE:
+ case NBDKIT_FUA_NONE:
+ case -1:
+ return r;
+ case -2:
+ /* Plugin specifically asking for our default */
+ break;
+ default:
+ /* Treat all remaining non-zero values like normalize_bool. */
+ return NBDKIT_FUA_EMULATE;
+ }
}
/* We intend to call .flush even if .can_flush returns false. */
if (p->plugin.flush || p->plugin._flush_v1)
diff --git i/plugins/python/python.c w/plugins/python/python.c
index 72f37dd7..510127ed 100644
--- i/plugins/python/python.c
+++ w/plugins/python/python.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -764,7 +764,8 @@ py_cache (void *handle, uint32_t count, uint64_t
offset, uint32_t flags)
}
static int
-boolean_callback (void *handle, const char *can_fn, const char *plain_fn)
+boolean_callback (void *handle, const char *can_fn, const char *plain_fn,
+ int def)
{
PyObject *obj = handle;
PyObject *fn;
@@ -789,49 +790,49 @@ boolean_callback (void *handle, const char
*can_fn, const char *plain_fn)
else if (plain_fn && callback_defined (plain_fn, NULL))
return 1;
else
- return 0;
+ return def;
}
static int
py_is_rotational (void *handle)
{
- return boolean_callback (handle, "is_rotational", NULL);
+ return boolean_callback (handle, "is_rotational", NULL, 0);
}
static int
py_can_multi_conn (void *handle)
{
- return boolean_callback (handle, "can_multi_conn", NULL);
+ return boolean_callback (handle, "can_multi_conn", NULL, 0);
}
static int
py_can_write (void *handle)
{
- return boolean_callback (handle, "can_write", "pwrite");
+ return boolean_callback (handle, "can_write", "pwrite", 0);
}
static int
py_can_flush (void *handle)
{
- return boolean_callback (handle, "can_flush", "flush");
+ return boolean_callback (handle, "can_flush", "flush", 0);
}
static int
py_can_trim (void *handle)
{
- return boolean_callback (handle, "can_trim", "trim");
+ return boolean_callback (handle, "can_trim", "trim", 0);
}
static int
py_can_zero (void *handle)
{
- return boolean_callback (handle, "can_zero", "zero");
+ return boolean_callback (handle, "can_zero", "zero", 0);
}
static int
py_can_fast_zero (void *handle)
{
- return boolean_callback (handle, "can_fast_zero", NULL);
+ return boolean_callback (handle, "can_fast_zero", NULL, -2);
}
static int
@@ -853,13 +854,7 @@ py_can_fua (void *handle)
Py_DECREF (r);
return ret;
}
- /* No Python ‘can_fua’, but check if there's a Python ‘flush’
- * callback defined. (In C modules, nbdkit would do this).
- */
- else if (callback_defined ("flush", NULL))
- return NBDKIT_FUA_EMULATE;
- else
- return NBDKIT_FUA_NONE;
+ return -2;
}
static int
diff --git i/plugins/sh/methods.c w/plugins/sh/methods.c
index 56e2d410..4d1c5329 100644
--- i/plugins/sh/methods.c
+++ w/plugins/sh/methods.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2018-2019 Red Hat Inc.
+ * Copyright (C) 2018-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -531,17 +531,7 @@ sh_can_fua (void *handle)
return r;
case MISSING:
- /* Check if the plugin claims to support flush. */
- switch (sh_can_flush (handle)) {
- case -1:
- return -1;
- case 0:
- return NBDKIT_FUA_NONE;
- case 1:
- return NBDKIT_FUA_EMULATE;
- default:
- abort ();
- }
+ return -2; /* Let nbdkit pick its default. */
case ERROR:
return -1;
@@ -610,16 +600,7 @@ sh_can_fast_zero (void *handle)
{
const char *method = "can_fast_zero";
const char *script = get_script (method);
- int r = boolean_method (script, method, handle, 2);
- if (r < 2)
- return r;
- /* We need to duplicate the logic of plugins.c: if can_fast_zero is
- * missing, we advertise fast fail support when can_zero is false.
- */
- r = sh_can_zero (handle);
- if (r == -1)
- return -1;
- return !r;
+ return boolean_method (script, method, handle, -2);
}
int
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
8:21 p.m.
New subject: [nbdkit PATCH 2/2] server: Better caching of .thread_model
Repeatedly calling out to the plugin to determine the thread model is
not only potentially expensive (for a plugin that takes its time
answering, as is easy to do with 'nbdkit eval thread_model="sleep 1"
...'), but risks confusion if the plugin changes its answer over time.
Expose the cache variable in locks.c to the rest of the server code so
that we can learn the thread model exactly once, with testsuite
coverage to ensure we don't regress.
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
server/internal.h | 1 +
server/connections.c | 8 +++-----
server/filters.c | 8 ++++----
server/locks.c | 7 +++----
server/plugins.c | 12 ++++++------
server/protocol-handshake.c | 5 ++---
server/sockets.c | 3 +--
tests/test-eflags.sh | 4 +---
8 files changed, 21 insertions(+), 27 deletions(-)
diff --git a/server/internal.h b/server/internal.h
index c1177b5d..e39db8a8 100644
--- a/server/internal.h
+++ b/server/internal.h
@@ -468,6 +468,7 @@ extern struct backend *filter_register (struct backend *next, size_t
index,
__attribute__((__nonnull__ (1, 3, 4, 5)));
/* locks.c */
+extern unsigned thread_model;
extern void lock_init_thread_model (void);
extern const char *name_of_thread_model (int model);
extern void lock_connection (void);
diff --git a/server/connections.c b/server/connections.c
index 5be3266a..c54c71c1 100644
--- a/server/connections.c
+++ b/server/connections.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -147,8 +147,7 @@ handle_single_connection (int sockin, int sockout)
return;
}
- if (top->thread_model (top) < NBDKIT_THREAD_MODEL_PARALLEL ||
- nworkers == 1)
+ if (thread_model < NBDKIT_THREAD_MODEL_PARALLEL || nworkers == 1)
nworkers = 0;
conn = new_connection (sockin, sockout, nworkers);
if (!conn)
@@ -277,8 +276,7 @@ new_connection (int sockin, int sockout, int nworkers)
* we aren't accepting until the plugin is not running, making
* non-atomicity okay.
*/
- assert (top->thread_model (top) <=
- NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
+ assert (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
lock_request (NULL);
if (pipe (conn->status_pipe)) {
perror ("pipe");
diff --git a/server/filters.c b/server/filters.c
index f0371066..b4f05236 100644
--- a/server/filters.c
+++ b/server/filters.c
@@ -66,7 +66,7 @@ filter_thread_model (struct backend *b)
{
struct backend_filter *f = container_of (b, struct backend_filter, backend);
int filter_thread_model = NBDKIT_THREAD_MODEL_PARALLEL;
- int thread_model = b->next->thread_model (b->next);
+ int model = b->next->thread_model (b->next);
if (f->filter.thread_model) {
filter_thread_model = f->filter.thread_model ();
@@ -74,10 +74,10 @@ filter_thread_model (struct backend *b)
exit (EXIT_FAILURE);
}
- if (filter_thread_model < thread_model) /* more serialized */
- thread_model = filter_thread_model;
+ if (filter_thread_model < model) /* more serialized */
+ model = filter_thread_model;
- return thread_model;
+ return model;
}
/* This is actually passing the request through to the final plugin,
diff --git a/server/locks.c b/server/locks.c
index 6211648d..5d54d311 100644
--- a/server/locks.c
+++ b/server/locks.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -40,7 +40,7 @@
/* Note that the plugin's thread model cannot change after being
* loaded, so caching it here is safe.
*/
-static int thread_model = -1;
+unsigned thread_model = -1;
static pthread_mutex_t connection_lock = PTHREAD_MUTEX_INITIALIZER;
static pthread_mutex_t all_requests_lock = PTHREAD_MUTEX_INITIALIZER;
@@ -71,12 +71,12 @@ lock_init_thread_model (void)
{
thread_model = top->thread_model (top);
debug ("using thread model: %s", name_of_thread_model (thread_model));
+ assert (thread_model <= NBDKIT_THREAD_MODEL_PARALLEL);
}
void
lock_connection (void)
{
- assert (thread_model >= 0);
if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS &&
pthread_mutex_lock (&connection_lock))
abort ();
@@ -95,7 +95,6 @@ lock_request (void)
{
GET_CONN;
- assert (thread_model >= 0);
if (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS &&
pthread_mutex_lock (&all_requests_lock))
abort ();
diff --git a/server/plugins.c b/server/plugins.c
index 708e1cfa..fa572a6a 100644
--- a/server/plugins.c
+++ b/server/plugins.c
@@ -65,14 +65,14 @@ static int
plugin_thread_model (struct backend *b)
{
struct backend_plugin *p = container_of (b, struct backend_plugin, backend);
- int thread_model = p->plugin._thread_model;
+ int model = p->plugin._thread_model;
int r;
#if !(defined SOCK_CLOEXEC && defined HAVE_MKOSTEMP && defined HAVE_PIPE2
&& \
defined HAVE_ACCEPT4)
- if (thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
+ if (model > NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS) {
debug ("system lacks atomic CLOEXEC, serializing to avoid fd leaks");
- thread_model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
+ model = NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS;
}
#endif
@@ -80,11 +80,11 @@ plugin_thread_model (struct backend *b)
r = p->plugin.thread_model ();
if (r == -1)
exit (EXIT_FAILURE);
- if (r < thread_model)
- thread_model = r;
+ if (r < model)
+ model = r;
}
- return thread_model;
+ return model;
}
static const char *
diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c
index 70ea4933..5e4b5e6e 100644
--- a/server/protocol-handshake.c
+++ b/server/protocol-handshake.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2019 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -144,8 +144,7 @@ protocol_common_open (uint64_t *exportsize, uint16_t *flags)
fl = backend_can_multi_conn (top);
if (fl == -1)
return -1;
- if (fl && (top->thread_model (top) >
- NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS))
+ if (fl && (thread_model > NBDKIT_THREAD_MODEL_SERIALIZE_CONNECTIONS))
eflags |= NBD_FLAG_CAN_MULTI_CONN;
fl = backend_can_cache (top);
diff --git a/server/sockets.c b/server/sockets.c
index 15aacf52..79ee704e 100644
--- a/server/sockets.c
+++ b/server/sockets.c
@@ -393,8 +393,7 @@ accept_connection (int listen_sock)
* at which point, we can use a mutex to ensure we aren't accepting
* until the plugin is not running, making non-atomicity okay.
*/
- assert (top->thread_model (top) <=
- NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
+ assert (thread_model <= NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS);
lock_request ();
thread_data->sock = set_cloexec (accept (listen_sock, NULL, NULL));
unlock_request ();
diff --git a/tests/test-eflags.sh b/tests/test-eflags.sh
index 3228bbf6..aebcaf68 100755
--- a/tests/test-eflags.sh
+++ b/tests/test-eflags.sh
@@ -74,9 +74,7 @@ do_nbdkit ()
{
# Prepend a check for internal caching to the script on stdin.
{ printf %s '
- if test $1 = thread_model; then
- :
- elif test -f $tmpdir/seen_$1; then
+ if test -f $tmpdir/seen_$1; then
echo "repeat call to $1"
>>'"$PWD/eflags.err"'
else
touch $tmpdir/seen_$1
--
2.25.1
9:04 p.m.
New subject: [nbdkit PATCH 3/2] python: Smarter default for .can_fast_zero
Similarly to the sh plugin, we want to let nbdkit advertise fast zero
support when it will not be calling into our .zero. And just like the
sh plugin, this requires that we cache a bit of information ourselves
(since it would be a layering violation to get what nbdkit has itself
cached). Thankfully, our defaults for .can_fua are easier implement
correctly than what the sh plugin has to do.
To demonstrate this, try adding:
def can_zero(h):
print("can_zero")
return False
to example.py, before running
./nbdkit python plugins/python/example.py --run 'qemu-nbd --list'
Signed-off-by: Eric Blake <eblake(a)redhat.com>
---
An audit of the other language plugins did not find any more missing
caching at this point.
plugins/python/python.c | 103 ++++++++++++++++++++++++++--------------
1 file changed, 67 insertions(+), 36 deletions(-)
diff --git a/plugins/python/python.c b/plugins/python/python.c
index 72f37dd7..a1a0438b 100644
--- a/plugins/python/python.c
+++ b/plugins/python/python.c
@@ -1,5 +1,5 @@
/* nbdkit
- * Copyright (C) 2013-2018 Red Hat Inc.
+ * Copyright (C) 2013-2020 Red Hat Inc.
*
* Redistribution and use in source and binary forms, with or without
* modification, are permitted provided that the following conditions are
@@ -455,51 +455,67 @@ py_get_ready (void)
return 0;
}
+struct handle {
+ int can_zero;
+ PyObject *py_h;
+};
+
static void *
py_open (int readonly)
{
PyObject *fn;
- PyObject *handle;
+ struct handle *h;
if (!callback_defined ("open", &fn)) {
nbdkit_error ("%s: missing callback: %s", script, "open");
return NULL;
}
+ h = malloc (sizeof *h);
+ if (h == NULL) {
+ nbdkit_error ("malloc: %m");
+ return NULL;
+ }
+ h->can_zero = -1;
+
PyErr_Clear ();
- handle = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True : Py_False,
- NULL);
+ h->py_h = PyObject_CallFunctionObjArgs (fn, readonly ? Py_True : Py_False,
+ NULL);
Py_DECREF (fn);
- if (check_python_failure ("open") == -1)
+ if (check_python_failure ("open") == -1) {
+ free (h);
return NULL;
+ }
- return handle;
+ assert (h->py_h);
+ return h;
}
static void
py_close (void *handle)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
if (callback_defined ("close", &fn)) {
PyErr_Clear ();
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
Py_DECREF (fn);
check_python_failure ("close");
Py_XDECREF (r);
}
- Py_DECREF (obj);
+ Py_DECREF (h->py_h);
+ free (h);
}
static int64_t
py_get_size (void *handle)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
int64_t ret;
@@ -511,7 +527,7 @@ py_get_size (void *handle)
PyErr_Clear ();
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
Py_DECREF (fn);
if (check_python_failure ("get_size") == -1)
return -1;
@@ -528,7 +544,7 @@ static int
py_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
uint32_t flags)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
Py_buffer view = {0};
@@ -543,10 +559,10 @@ py_pread (void *handle, void *buf, uint32_t count, uint64_t offset,
switch (py_api_version) {
case 1:
- r = PyObject_CallFunction (fn, "OiL", obj, count, offset);
+ r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset);
break;
case 2:
- r = PyObject_CallFunction (fn, "ONLI", obj,
+ r = PyObject_CallFunction (fn, "ONLI", h->py_h,
PyMemoryView_FromMemory ((char *)buf, count, PyBUF_WRITE),
offset, flags);
break;
@@ -590,7 +606,7 @@ static int
py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset,
uint32_t flags)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
@@ -599,12 +615,12 @@ py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t
offset,
switch (py_api_version) {
case 1:
- r = PyObject_CallFunction (fn, "ONL", obj,
+ r = PyObject_CallFunction (fn, "ONL", h->py_h,
PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
offset);
break;
case 2:
- r = PyObject_CallFunction (fn, "ONLI", obj,
+ r = PyObject_CallFunction (fn, "ONLI", h->py_h,
PyMemoryView_FromMemory ((char *)buf, count, PyBUF_READ),
offset, flags);
break;
@@ -626,7 +642,7 @@ py_pwrite (void *handle, const void *buf, uint32_t count, uint64_t
offset,
static int
py_flush (void *handle, uint32_t flags)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
@@ -635,10 +651,10 @@ py_flush (void *handle, uint32_t flags)
switch (py_api_version) {
case 1:
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
break;
case 2:
- r = PyObject_CallFunction (fn, "OI", obj, flags);
+ r = PyObject_CallFunction (fn, "OI", h->py_h, flags);
break;
default: abort ();
}
@@ -658,7 +674,7 @@ py_flush (void *handle, uint32_t flags)
static int
py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t flags)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
@@ -667,10 +683,10 @@ py_trim (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
switch (py_api_version) {
case 1:
- r = PyObject_CallFunction (fn, "OiL", obj, count, offset);
+ r = PyObject_CallFunction (fn, "OiL", h->py_h, count, offset);
break;
case 2:
- r = PyObject_CallFunction (fn, "OiLI", obj, count, offset, flags);
+ r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset,
flags);
break;
default: abort ();
}
@@ -690,7 +706,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)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
@@ -702,12 +718,12 @@ py_zero (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
case 1: {
int may_trim = flags & NBDKIT_FLAG_MAY_TRIM;
r = PyObject_CallFunction (fn, "OiLO",
- obj, count, offset,
+ h->py_h, count, offset,
may_trim ? Py_True : Py_False);
break;
}
case 2:
- r = PyObject_CallFunction (fn, "OiLI", obj, count, offset, flags);
+ r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset,
flags);
break;
default: abort ();
}
@@ -736,7 +752,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)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
@@ -746,7 +762,7 @@ py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
switch (py_api_version) {
case 1:
case 2:
- r = PyObject_CallFunction (fn, "OiLI", obj, count, offset, flags, NULL);
+ r = PyObject_CallFunction (fn, "OiLI", h->py_h, count, offset, flags,
NULL);
break;
default: abort ();
}
@@ -766,7 +782,7 @@ py_cache (void *handle, uint32_t count, uint64_t offset, uint32_t
flags)
static int
boolean_callback (void *handle, const char *can_fn, const char *plain_fn)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
int ret;
@@ -774,7 +790,7 @@ boolean_callback (void *handle, const char *can_fn, const char
*plain_fn)
if (callback_defined (can_fn, &fn)) {
PyErr_Clear ();
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
Py_DECREF (fn);
if (check_python_failure (can_fn) == -1)
return -1;
@@ -825,19 +841,34 @@ py_can_trim (void *handle)
static int
py_can_zero (void *handle)
{
- return boolean_callback (handle, "can_zero", "zero");
+ struct handle *h = handle;
+
+ if (h->can_zero >= 0)
+ return h->can_zero;
+ return h->can_zero = boolean_callback (handle, "can_zero",
"zero");
}
static int
py_can_fast_zero (void *handle)
{
- return boolean_callback (handle, "can_fast_zero", NULL);
+ int r;
+
+ if (callback_defined ("can_fast_zero", NULL))
+ return boolean_callback (handle, "can_fast_zero", NULL);
+
+ /* No Python ‘can_fast_zero’, but we advertise fast fail support when
+ * 'can_zero' is false. (In C modules, nbdkit would do this).
+ */
+ r = py_can_zero (handle);
+ if (r == -1)
+ return -1;
+ return !r;
}
static int
py_can_fua (void *handle)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
int ret;
@@ -845,7 +876,7 @@ py_can_fua (void *handle)
if (callback_defined ("can_fua", &fn)) {
PyErr_Clear ();
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
Py_DECREF (fn);
if (check_python_failure ("can_fua") == -1)
return -1;
@@ -865,7 +896,7 @@ py_can_fua (void *handle)
static int
py_can_cache (void *handle)
{
- PyObject *obj = handle;
+ struct handle *h = handle;
PyObject *fn;
PyObject *r;
int ret;
@@ -873,7 +904,7 @@ py_can_cache (void *handle)
if (callback_defined ("can_cache", &fn)) {
PyErr_Clear ();
- r = PyObject_CallFunctionObjArgs (fn, obj, NULL);
+ r = PyObject_CallFunctionObjArgs (fn, h->py_h, NULL);
Py_DECREF (fn);
if (check_python_failure ("can_cache") == -1)
return -1;
--
2.25.1
3:36 a.m.
It took me a while to work out the "why" of patch 1. The answer is
because methods like sh_can_fua call sh_can_flush, and since
sh_can_flush is also called by core nbdkit you end up calling this
function twice.
I think patches 1 and 3 would have been easier to review if the
mechanical change to the handle struct was in a separate patch from
the change in caching logic, but I've reviewed them now.
ACK
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
1633
days inactive
1633
days old
7 comments
2 participants
participants (2)
-
Eric Blake -
Richard W.M. Jones