"NBD_FLAG_CAN_MULTI_CONN: Indicates that the server operates entirely without cache, or that the cache it uses is shared among all connections to the given device. In particular, if this flag is present, then the effects of NBD_CMD_FLUSH and NBD_CMD_FLAG_FUA MUST be visible across all connections when the server sends its reply to that command to the client. In the absence of this flag, clients SHOULD NOT multiplex their commands over more than one connection to the export." This is implemented in nbdkit by a boolean function which plugins may implement if they believe they satisfy the requirements above. It can also be passed through or modified by filters. It defaults to false, which is the safe default. --- docs/nbdkit-filter.pod | 4 ++++ docs/nbdkit-plugin.pod | 20 ++++++++++++++++++++ docs/nbdkit-protocol.pod | 4 ++++ include/nbdkit-filter.h | 3 +++ include/nbdkit-plugin.h | 2 ++ server/internal.h | 1 + server/protocol.h | 1 + server/connections.c | 9 +++++++++ server/filters.c | 24 ++++++++++++++++++++++++ server/plugins.c | 17 +++++++++++++++++ tests/test-layers-filter.c | 10 ++++++++++ tests/test-layers-plugin.c | 8 ++++++++ tests/test-layers.c | 6 ++++++ 13 files changed, 109 insertions(+) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 1e1bf5d..4f27adb 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -354,6 +354,8 @@ calls. =head2 C<.can_fua> +=head2 C<.can_multi_conn> + int (*can_write) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); int (*can_flush) (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -367,6 +369,8 @@ calls. void *handle); int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); + int (*can_multi_conn) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle); These intercept the corresponding plugin methods, and control feature bits advertised to the client. diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index cfa14b0..c72ea0d 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -586,6 +586,26 @@ 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. +=head2 C<.can_multi_conn> + + int can_multi_conn (void *handle); + +This is called during the option negotiation phase to find out if the +plugin can handle multiple connections from a single client. + +Specifically it means that either the plugin does not cache requests +at all. Or if it does cache then the effects of a C<.flush> request +or setting C<NBDKIT_FLAG_FUA> on a request must be visible across all +connections to the plugin before the plugin replies to that request. + +If you use the Linux NBD client with the I<-C> option then this flag +is checked. + +If there is an error, C<.can_multi_conn> should call C<nbdkit_error> +with an error message and return C<-1>. + +This callback is not required. If omitted, then we return false. + =head2 C<.pread> int pread (void *handle, void *buf, uint32_t count, uint64_t offset, diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod index d5ba00f..db7fcf9 100644 --- a/docs/nbdkit-protocol.pod +++ b/docs/nbdkit-protocol.pod @@ -116,6 +116,10 @@ Supported in nbdkit E<ge> 1.9.3. This protocol enhancement allows a client to inspect details about the export without actually connecting. +=item C<NBD_FLAG_CAN_MULTI_CONN> + +Supported in nbdkit E<ge> 1.9.10. + =item Structured Replies I<Not supported>. diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 931d923..71c06c8 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -59,6 +59,7 @@ struct nbdkit_next_ops { int (*can_trim) (void *nxdata); int (*can_zero) (void *nxdata); int (*can_fua) (void *nxdata); + int (*can_multi_conn) (void *nxdata); int (*pread) (void *nxdata, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err); @@ -121,6 +122,8 @@ struct nbdkit_filter { void *handle); int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); + int (*can_multi_conn) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle); int (*pread) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, void *buf, uint32_t count, uint64_t offset, diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index 31aa6c7..d43b2f5 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -123,6 +123,8 @@ struct nbdkit_plugin { #endif const char *magic_config_key; + + int (*can_multi_conn) (void *handle); }; extern void nbdkit_set_error (int err); diff --git a/server/internal.h b/server/internal.h index e5c91a9..1f237a5 100644 --- a/server/internal.h +++ b/server/internal.h @@ -212,6 +212,7 @@ struct backend { int (*can_trim) (struct backend *, struct connection *conn); int (*can_zero) (struct backend *, struct connection *conn); int (*can_fua) (struct backend *, struct connection *conn); + int (*can_multi_conn) (struct backend *, struct connection *conn); int (*pread) (struct backend *, struct connection *conn, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err); diff --git a/server/protocol.h b/server/protocol.h index 6709ddc..003fc45 100644 --- a/server/protocol.h +++ b/server/protocol.h @@ -94,6 +94,7 @@ extern const char *name_of_nbd_flag (int); #define NBD_FLAG_ROTATIONAL (1 << 4) #define NBD_FLAG_SEND_TRIM (1 << 5) #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6) +#define NBD_FLAG_CAN_MULTI_CONN (1 << 8) /* NBD options (new style handshake only). */ extern const char *name_of_nbd_opt (int); diff --git a/server/connections.c b/server/connections.c index 0a89315..51d4912 100644 --- a/server/connections.c +++ b/server/connections.c @@ -84,6 +84,7 @@ struct connection { bool can_trim; bool can_zero; bool can_fua; + bool can_multi_conn; bool using_tls; int sockin, sockout; @@ -463,6 +464,14 @@ compute_eflags (struct connection *conn, uint16_t *flags) conn->is_rotational = true; } + fl = backend->can_multi_conn (backend, conn); + if (fl == -1) + return -1; + if (fl) { + eflags |= NBD_FLAG_CAN_MULTI_CONN; + conn->can_multi_conn = true; + } + *flags = eflags; return 0; } diff --git a/server/filters.c b/server/filters.c index d02e7fb..5b7abc4 100644 --- a/server/filters.c +++ b/server/filters.c @@ -316,6 +316,13 @@ next_can_fua (void *nxdata) return b_conn->b->can_fua (b_conn->b, b_conn->conn); } +static int +next_can_multi_conn (void *nxdata) +{ + struct b_conn *b_conn = nxdata; + return b_conn->b->can_multi_conn (b_conn->b, b_conn->conn); +} + static int next_pread (void *nxdata, void *buf, uint32_t count, uint64_t offset, uint32_t flags, int *err) @@ -365,6 +372,7 @@ static struct nbdkit_next_ops next_ops = { .can_trim = next_can_trim, .can_zero = next_can_zero, .can_fua = next_can_fua, + .can_multi_conn = next_can_multi_conn, .pread = next_pread, .pwrite = next_pwrite, .flush = next_flush, @@ -518,6 +526,21 @@ filter_can_fua (struct backend *b, struct connection *conn) return f->backend.next->can_fua (f->backend.next, conn); } +static int +filter_can_multi_conn (struct backend *b, struct connection *conn) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + void *handle = connection_get_handle (conn, f->backend.i); + struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; + + debug ("%s: can_multi_conn", f->name); + + if (f->filter.can_multi_conn) + return f->filter.can_multi_conn (&next_ops, &nxdata, handle); + else + return f->backend.next->can_multi_conn (f->backend.next, conn); +} + static int filter_pread (struct backend *b, struct connection *conn, void *buf, uint32_t count, uint64_t offset, @@ -645,6 +668,7 @@ static struct backend filter_functions = { .can_trim = filter_can_trim, .can_zero = filter_can_zero, .can_fua = filter_can_fua, + .can_multi_conn = filter_can_multi_conn, .pread = filter_pread, .pwrite = filter_pwrite, .flush = filter_flush, diff --git a/server/plugins.c b/server/plugins.c index cb0f411..bbde578 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -197,6 +197,7 @@ plugin_dump_fields (struct backend *b) HAS (flush); HAS (trim); HAS (zero); + HAS (can_multi_conn); #undef HAS /* Custom fields. */ @@ -414,6 +415,21 @@ plugin_can_fua (struct backend *b, struct connection *conn) return NBDKIT_FUA_NONE; } +static int +plugin_can_multi_conn (struct backend *b, struct connection *conn) +{ + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + + assert (connection_get_handle (conn, 0)); + + debug ("can_multi_conn"); + + if (p->plugin.can_multi_conn) + return p->plugin.can_multi_conn (connection_get_handle (conn, 0)); + else + return 0; /* assume false */ +} + /* Plugins and filters can call this to set the true errno, in cases * where !errno_is_preserved. */ @@ -656,6 +672,7 @@ static struct backend plugin_functions = { .can_trim = plugin_can_trim, .can_zero = plugin_can_zero, .can_fua = plugin_can_fua, + .can_multi_conn = plugin_can_multi_conn, .pread = plugin_pread, .pwrite = plugin_pwrite, .flush = plugin_flush, diff --git a/tests/test-layers-filter.c b/tests/test-layers-filter.c index c5a96e7..6da6ee6 100644 --- a/tests/test-layers-filter.c +++ b/tests/test-layers-filter.c @@ -167,6 +167,15 @@ test_layers_filter_can_fua (struct nbdkit_next_ops *next_ops, void *nxdata, return next_ops->can_fua (nxdata); } +static int +test_layers_filter_can_multi_conn (struct nbdkit_next_ops *next_ops, + void *nxdata, + void *handle) +{ + DEBUG_FUNCTION; + return next_ops->can_multi_conn (nxdata); +} + static int test_layers_filter_pread (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, void *buf, @@ -233,6 +242,7 @@ static struct nbdkit_filter filter = { .can_trim = test_layers_filter_can_trim, .can_zero = test_layers_filter_can_zero, .can_fua = test_layers_filter_can_fua, + .can_multi_conn = test_layers_filter_can_multi_conn, .pread = test_layers_filter_pread, .pwrite = test_layers_filter_pwrite, .flush = test_layers_filter_flush, diff --git a/tests/test-layers-plugin.c b/tests/test-layers-plugin.c index 3befcc1..042ecc3 100644 --- a/tests/test-layers-plugin.c +++ b/tests/test-layers-plugin.c @@ -137,6 +137,13 @@ test_layers_plugin_can_fua (void *handle) return NBDKIT_FUA_NATIVE; } +static int +test_layers_plugin_can_multi_conn (void *handle) +{ + DEBUG_FUNCTION; + return 1; +} + static int test_layers_plugin_pread (void *handle, void *buf, uint32_t count, uint64_t offset, @@ -196,6 +203,7 @@ static struct nbdkit_plugin plugin = { .can_trim = test_layers_plugin_can_trim, .can_zero = test_layers_plugin_can_zero, .can_fua = test_layers_plugin_can_fua, + .can_multi_conn = test_layers_plugin_can_multi_conn, .pread = test_layers_plugin_pread, .pwrite = test_layers_plugin_pwrite, .flush = test_layers_plugin_flush, diff --git a/tests/test-layers.c b/tests/test-layers.c index 261b9c6..c8d15d2 100644 --- a/tests/test-layers.c +++ b/tests/test-layers.c @@ -349,6 +349,12 @@ main (int argc, char *argv[]) "filter1: test_layers_filter_is_rotational", "test_layers_plugin_is_rotational", NULL); + log_verify_seen_in_order + ("filter3: test_layers_filter_can_multi_conn", + "filter2: test_layers_filter_can_multi_conn", + "filter1: test_layers_filter_can_multi_conn", + "test_layers_plugin_can_multi_conn", + NULL); fprintf (stderr, "%s: protocol connected\n", program_name); -- 2.19.2

On 1/5/19 8:50 AM, Richard W.M. Jones wrote: I think we could get away with returning 1, _because_ we are serializing all requests. Even though we make multiple calls into the plugin (worst-case being read-modify-write-flush on a client write-with-FUA), no other operation can compete with a client flush. But I also agree that if we were to fix the bounce buffer to be thread-safe and change the filter to be more parallel, then you are also right that unless we add a .flush callback and add locking so that all calls into the plugin are guarded within the same lock, then returning 0 is appropriate, even if the plugin itself returns 1. So I agree with adding this callback, and could go either way with you returning 0 or 1 for now. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

--- plugins/nbd/nbd.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 0443788..674f4a4 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -644,6 +644,14 @@ nbd_can_fua (void *handle) return h->flags & NBD_FLAG_SEND_FUA ? NBDKIT_FUA_NATIVE : NBDKIT_FUA_NONE; } +static int +nbd_can_multi_conn (void *handle) +{ + struct handle *h = handle; + + return h->flags & NBD_FLAG_CAN_MULTI_CONN; +} + /* Read data from the file. */ static int nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset, @@ -732,6 +740,7 @@ static struct nbdkit_plugin plugin = { .can_trim = nbd_can_trim, .can_zero = nbd_can_zero, .can_fua = nbd_can_fua, + .can_multi_conn = nbd_can_multi_conn, .pread = nbd_pread, .pwrite = nbd_pwrite, .zero = nbd_zero, -- 2.19.2

--- plugins/sh/sh.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/plugins/sh/sh.c b/plugins/sh/sh.c index 67750e5..49ade78 100644 --- a/plugins/sh/sh.c +++ b/plugins/sh/sh.c @@ -579,6 +579,12 @@ sh_can_fua (void *handle) } } +static int +sh_can_multi_conn (void *handle) +{ + return boolean_method (handle, "can_multi_conn"); +} + static int sh_flush (void *handle, uint32_t flags) { @@ -699,6 +705,7 @@ static struct nbdkit_plugin plugin = { .can_trim = sh_can_trim, .can_zero = sh_can_zero, .can_fua = sh_can_fua, + .can_multi_conn = sh_can_multi_conn, .pread = sh_pread, .pwrite = sh_pwrite, -- 2.19.2

These plugins use NBDKIT_THREAD_MODEL_SERIALIZE_ALL_REQUESTS because of their use of the global sparse array. However since they are both RAM disks, and thus (a) flush is a no-op, and (b) the locked part of each request is short, it is both safe and there may be some performance benefit to using NBD_FLAG_CAN_MULTI_CONN. --- plugins/data/data.c | 8 ++++++++ plugins/memory/memory.c | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/plugins/data/data.c b/plugins/data/data.c index f8e91be..11a7b69 100644 --- a/plugins/data/data.c +++ b/plugins/data/data.c @@ -324,6 +324,13 @@ data_get_size (void *handle) return size; } +/* Serves the same data over multiple connections. */ +static int +data_can_multi_conn (void *handle) +{ + return 1; +} + /* Read data. */ static int data_pread (void *handle, void *buf, uint32_t count, uint64_t offset) @@ -366,6 +373,7 @@ static struct nbdkit_plugin plugin = { .dump_plugin = data_dump_plugin, .open = data_open, .get_size = data_get_size, + .can_multi_conn = data_can_multi_conn, .pread = data_pread, .pwrite = data_pwrite, .zero = data_zero, diff --git a/plugins/memory/memory.c b/plugins/memory/memory.c index 4013087..0084401 100644 --- a/plugins/memory/memory.c +++ b/plugins/memory/memory.c @@ -115,6 +115,13 @@ memory_get_size (void *handle) return (int64_t) size; } +/* Serves the same data over multiple connections. */ +static int +memory_can_multi_conn (void *handle) +{ + return 1; +} + /* Read data. */ static int memory_pread (void *handle, void *buf, uint32_t count, uint64_t offset) @@ -156,6 +163,7 @@ static struct nbdkit_plugin plugin = { .config_help = memory_config_help, .open = memory_open, .get_size = memory_get_size, + .can_multi_conn = memory_can_multi_conn, .pread = memory_pread, .pwrite = memory_pwrite, .zero = memory_zero, -- 2.19.2