[nbdkit PATCH 0/7] Initial implementation of FUA flag passthrough
Tested via: term1$ qemu-nbd -k $PWD/sock -t -f raw -x foo junk --trace=nbd_\* term2$ ./nbdkit -f -v -e bar nbd socket=$PWD/sock export=foo term3$ qemu-io -t none -f raw nbd://localhost:10809/bar --trace=nbd_\* and checking the traces to see that 'w 0 1' vs. 'w -f 0 1' was able to influence whether the FUA flag showed up at the server in term1. Still to go: figure out how to expose the flags through the language bindings Patch 7 is RFC; Rich and I debated on IRC whether we want it (the code in plugin_register() sounds like it tries hard to support newer plugins with older nbdkit), or whether we should just drop the patch and state that a user either manually supplies back-compat variants (tedious boilerplate for the user to supply, especially since newer nbdkit will never call the older functions) or just document that newer plugins combined with old nbdkit is not guaranteed to work. Eric Blake (7): connections: Ignore FUA flag on read/flush protocol: Split flags from cmd field in requests plugins: Move FUA fallback to plugins plugins: Add callbacks for FUA semantics plugins: Implement FUA support nbd: Wire up FUA flag passthrough RFC: plugins: Add back-compat for new plugin with old nbdkit docs/nbdkit-plugin.pod | 77 +++++++++++++++++++++++++++++++++++++-- docs/nbdkit.pod | 8 +++-- include/nbdkit-plugin.h | 34 +++++++++++++++++- plugins/nbd/nbd.c | 64 ++++++++++++++++++++------------- src/connections.c | 46 +++++++++++++----------- src/internal.h | 9 ++--- src/plugins.c | 96 +++++++++++++++++++++++++++++++++++++++---------- src/protocol.h | 10 +++--- 8 files changed, 266 insertions(+), 78 deletions(-) -- 2.14.3
The NBD spec says that we must tolerate a client sending NBD_CMD_FLAG_FUA on any command (due to historical behavior of at least qemu sending it on READ), but that it only has to have defined semantics on commands that can cause write actions. It will be easier for future patches to support plugins that can honor FUA semantics on write if we silently ignore FUA on non-writes (the NBD spec says only WRITE, WRITE_ZEROES, and TRIM have write semantics). Note that validate_request already ensured that that we are not calling a write command if conn->readonly; since only write commands can leave flush_after_command set, we no longer need to check conn->readonly in handle_request. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/connections.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/connections.c b/src/connections.c index 111a810..a16c118 100644 --- a/src/connections.c +++ b/src/connections.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2017 Red Hat Inc. + * Copyright (C) 2013-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -871,9 +871,7 @@ handle_request (struct connection *conn, bool flush_after_command; /* Flush after command performed? */ - flush_after_command = (flags & NBD_CMD_FLAG_FUA) != 0; - if (!conn->can_flush || conn->readonly) - flush_after_command = false; + flush_after_command = conn->can_flush && (flags & NBD_CMD_FLAG_FUA); /* The plugin should call nbdkit_set_error() to request a particular error, otherwise we fallback to errno or EIO. */ @@ -881,6 +879,7 @@ handle_request (struct connection *conn, switch (cmd) { case NBD_CMD_READ: + flush_after_command = false; if (plugin_pread (conn, buf, count, offset) == -1) return get_error (conn); break; @@ -891,6 +890,7 @@ handle_request (struct connection *conn, break; case NBD_CMD_FLUSH: + flush_after_command = false; if (plugin_flush (conn) == -1) return get_error (conn); break; -- 2.14.3
Since NBD is a big-endian protocol, the upstream spec was able to repurpose a 32-bit field with flags starting at (1<<16) OR'd into the command into two 16-bit fields (flags first, starting at 1<<0, then the command) for ease of documentation. Matching that split in our code base will also make it easier to implement smarter FUA flag support. This addresses one of the TODO in the nbd plugin. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/nbd/nbd.c | 39 ++++++++++++++++++++------------------- src/connections.c | 12 ++++++------ src/protocol.h | 10 +++++----- 3 files changed, 31 insertions(+), 30 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index 04147f1..c9727f7 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2017 Red Hat Inc. + * Copyright (C) 2017-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -249,13 +249,14 @@ find_trans_by_cookie (struct handle *h, uint64_t cookie) /* Send a request, return 0 on success or -1 on write failure. */ static int -nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset, - uint32_t count, uint64_t cookie, const void *buf) +nbd_request_raw (struct handle *h, uint16_t flags, uint16_t type, + uint64_t offset, uint32_t count, uint64_t cookie, + const void *buf) { struct request req = { .magic = htobe32 (NBD_REQUEST_MAGIC), - /* TODO nbdkit should have a way to pass flags, separate from cmd type */ - .type = htobe32 (type), + .flags = htobe16 (flags), + .type = htobe16 (type), .handle = cookie, /* Opaque to server, so endianness doesn't matter */ .offset = htobe64 (offset), .count = htobe32 (count), @@ -275,8 +276,9 @@ nbd_request_raw (struct handle *h, uint32_t type, uint64_t offset, /* Perform the request half of a transaction. On success, return the non-negative fd for reading the reply; on error return -1. */ static int -nbd_request_full (struct handle *h, uint32_t type, uint64_t offset, - uint32_t count, const void *req_buf, void *rep_buf) +nbd_request_full (struct handle *h, uint16_t flags, uint16_t type, + uint64_t offset, uint32_t count, const void *req_buf, + void *rep_buf) { int err; struct transaction *trans; @@ -307,7 +309,7 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset, fd = trans->u.fds[0]; cookie = trans->u.cookie; nbd_unlock (h); - if (nbd_request_raw (h, type, offset, count, cookie, req_buf) == 0) + if (nbd_request_raw (h, flags, type, offset, count, cookie, req_buf) == 0) return fd; trans = find_trans_by_cookie (h, cookie); @@ -326,9 +328,10 @@ nbd_request_full (struct handle *h, uint32_t type, uint64_t offset, /* Shorthand for nbd_request_full when no extra buffers are involved. */ static int -nbd_request (struct handle *h, uint32_t type, uint64_t offset, uint32_t count) +nbd_request (struct handle *h, uint16_t flags, uint16_t type, uint64_t offset, + uint32_t count) { - return nbd_request_full (h, type, offset, count, NULL, NULL); + return nbd_request_full (h, flags, type, offset, count, NULL, NULL); } /* Read a reply, and look up the fd corresponding to the transaction. @@ -563,7 +566,7 @@ nbd_close (void *handle) struct handle *h = handle; if (!h->dead) - nbd_request_raw (h, NBD_CMD_DISC, 0, 0, 0, NULL); + nbd_request_raw (h, 0, NBD_CMD_DISC, 0, 0, 0, NULL); close (h->fd); if ((errno = pthread_join (h->reader, NULL))) nbdkit_debug ("failed to join reader thread: %m"); @@ -622,7 +625,7 @@ nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset) /* TODO Auto-fragment this if the client has a larger max transfer limit than the server */ - c = nbd_request_full (h, NBD_CMD_READ, offset, count, NULL, buf); + c = nbd_request_full (h, 0, NBD_CMD_READ, offset, count, NULL, buf); return c < 0 ? c : nbd_reply (h, c); } @@ -635,7 +638,7 @@ nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset) /* TODO Auto-fragment this if the client has a larger max transfer limit than the server */ - c = nbd_request_full (h, NBD_CMD_WRITE, offset, count, buf, NULL); + c = nbd_request_full (h, 0, NBD_CMD_WRITE, offset, count, buf, NULL); return c < 0 ? c : nbd_reply (h, c); } @@ -644,7 +647,6 @@ static int nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) { struct handle *h = handle; - uint32_t cmd = NBD_CMD_WRITE_ZEROES; int c; if (!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES)) { @@ -653,9 +655,8 @@ nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) return -1; } - if (!may_trim) - cmd |= NBD_CMD_FLAG_NO_HOLE; - c = nbd_request (h, cmd, offset, count); + c = nbd_request (h, may_trim ? 0 : NBD_CMD_FLAG_NO_HOLE, + NBD_CMD_WRITE_ZEROES, offset, count); return c < 0 ? c : nbd_reply (h, c); } @@ -666,7 +667,7 @@ nbd_trim (void *handle, uint32_t count, uint64_t offset) struct handle *h = handle; int c; - c = nbd_request (h, NBD_CMD_TRIM, offset, count); + c = nbd_request (h, 0, NBD_CMD_TRIM, offset, count); return c < 0 ? c : nbd_reply (h, c); } @@ -677,7 +678,7 @@ nbd_flush (void *handle) struct handle *h = handle; int c; - c = nbd_request (h, NBD_CMD_FLUSH, 0, 0); + c = nbd_request (h, 0, NBD_CMD_FLUSH, 0, 0); return c < 0 ? c : nbd_reply (h, c); } diff --git a/src/connections.c b/src/connections.c index a16c118..24df4b6 100644 --- a/src/connections.c +++ b/src/connections.c @@ -760,7 +760,7 @@ valid_range (struct connection *conn, uint64_t offset, uint32_t count) static bool validate_request (struct connection *conn, - uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t count, + uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, uint32_t *error) { /* Readonly connection? */ @@ -865,7 +865,7 @@ get_error (struct connection *conn) */ static uint32_t handle_request (struct connection *conn, - uint32_t cmd, uint32_t flags, uint64_t offset, uint32_t count, + uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, void *buf) { bool flush_after_command; @@ -979,7 +979,8 @@ recv_request_send_reply (struct connection *conn) int r; struct request request; struct reply reply; - uint32_t magic, cmd, flags, count, error = 0; + uint16_t cmd, flags; + uint32_t magic, count, error = 0; uint64_t offset; CLEANUP_FREE char *buf = NULL; @@ -1005,9 +1006,8 @@ recv_request_send_reply (struct connection *conn) return set_status (conn, -1); } - cmd = be32toh (request.type); - flags = cmd & ~NBD_CMD_MASK_COMMAND; - cmd &= NBD_CMD_MASK_COMMAND; + flags = be16toh (request.flags); + cmd = be16toh (request.type); offset = be64toh (request.offset); count = be32toh (request.count); diff --git a/src/protocol.h b/src/protocol.h index 9d9dad9..aa458a0 100644 --- a/src/protocol.h +++ b/src/protocol.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013 Red Hat Inc. + * Copyright (C) 2013-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -112,7 +112,8 @@ struct new_handshake_finish { /* Request (client -> server). */ struct request { uint32_t magic; /* NBD_REQUEST_MAGIC. */ - uint32_t type; /* Request type. */ + uint16_t flags; /* Request flags. */ + uint16_t type; /* Request type. */ uint64_t handle; /* Opaque handle. */ uint64_t offset; /* Request offset. */ uint32_t count; /* Request length. */ @@ -134,9 +135,8 @@ struct reply { #define NBD_CMD_FLUSH 3 #define NBD_CMD_TRIM 4 #define NBD_CMD_WRITE_ZEROES 6 -#define NBD_CMD_MASK_COMMAND 0xffff -#define NBD_CMD_FLAG_FUA (1<<16) -#define NBD_CMD_FLAG_NO_HOLE (2<<16) +#define NBD_CMD_FLAG_FUA (1<<0) +#define NBD_CMD_FLAG_NO_HOLE (1<<1) /* Error codes (previously errno). * See http://git.qemu.org/?p=qemu.git;a=commitdiff;h=ca4414804114fd0095b317785bc0b... -- 2.14.3
As long as the plugins couldn't directly handle Forced Unit Access (FUA), it made sense to have a common fallback in handle_request(). But upcoming patches will allow plugins to handle FUA, at which point the fallback is easier to implement on a per-command basis, via an extra parameter to the plugins wrapper code. We also add an additional validation to commands sent by the client: if we did not advertise FUA flag support (because the plugin does not support flush), then the client should not be requesting FUA; this in turn means that we can assert that the flush callback is valid if the plugins layer gets a fua flag. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/connections.c | 21 +++++++++------------ src/internal.h | 8 ++++---- src/plugins.c | 42 +++++++++++++++++++++++++++++++----------- 3 files changed, 44 insertions(+), 27 deletions(-) diff --git a/src/connections.c b/src/connections.c index 24df4b6..e4a0a82 100644 --- a/src/connections.c +++ b/src/connections.c @@ -814,6 +814,11 @@ validate_request (struct connection *conn, *error = EINVAL; return false; } + if (!conn->can_flush && (flags & NBD_CMD_FLAG_FUA)) { + nbdkit_error ("invalid request: FUA flag not supported"); + *error = EINVAL; + return false; + } /* Refuse over-large read and write requests. */ if ((cmd == NBD_CMD_WRITE || cmd == NBD_CMD_READ) && @@ -868,10 +873,7 @@ handle_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, void *buf) { - bool flush_after_command; - - /* Flush after command performed? */ - flush_after_command = conn->can_flush && (flags & NBD_CMD_FLAG_FUA); + bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA); /* The plugin should call nbdkit_set_error() to request a particular error, otherwise we fallback to errno or EIO. */ @@ -879,30 +881,28 @@ handle_request (struct connection *conn, switch (cmd) { case NBD_CMD_READ: - flush_after_command = false; if (plugin_pread (conn, buf, count, offset) == -1) return get_error (conn); break; case NBD_CMD_WRITE: - if (plugin_pwrite (conn, buf, count, offset) == -1) + if (plugin_pwrite (conn, buf, count, offset, fua) == -1) return get_error (conn); break; case NBD_CMD_FLUSH: - flush_after_command = false; if (plugin_flush (conn) == -1) return get_error (conn); break; case NBD_CMD_TRIM: - if (plugin_trim (conn, count, offset) == -1) + if (plugin_trim (conn, count, offset, fua) == -1) return get_error (conn); break; case NBD_CMD_WRITE_ZEROES: if (plugin_zero (conn, count, offset, - !(flags & NBD_CMD_FLAG_NO_HOLE)) == -1) + !(flags & NBD_CMD_FLAG_NO_HOLE), fua) == -1) return get_error (conn); break; @@ -910,9 +910,6 @@ handle_request (struct connection *conn, abort (); } - if (flush_after_command && plugin_flush (conn) == -1) - return get_error (conn); - return 0; } diff --git a/src/internal.h b/src/internal.h index 73bc09e..3ab08d3 100644 --- a/src/internal.h +++ b/src/internal.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2017 Red Hat Inc. + * Copyright (C) 2013-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -164,10 +164,10 @@ extern int plugin_can_flush (struct connection *conn); extern int plugin_is_rotational (struct connection *conn); extern int plugin_can_trim (struct connection *conn); extern int plugin_pread (struct connection *conn, void *buf, uint32_t count, uint64_t offset); -extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count, uint64_t offset); +extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count, uint64_t offset, int fua); extern int plugin_flush (struct connection *conn); -extern int plugin_trim (struct connection *conn, uint32_t count, uint64_t offset); -extern int plugin_zero (struct connection *conn, uint32_t count, uint64_t offset, int may_trim); +extern int plugin_trim (struct connection *conn, uint32_t count, uint64_t offset, int fua); +extern int plugin_zero (struct connection *conn, uint32_t count, uint64_t offset, int may_trim, int fua); /* sockets.c */ extern int *bind_unix_socket (size_t *); diff --git a/src/plugins.c b/src/plugins.c index 9b5d2d5..61f0cc8 100644 --- a/src/plugins.c +++ b/src/plugins.c @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013 Red Hat Inc. + * Copyright (C) 2013-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -497,19 +497,26 @@ plugin_pread (struct connection *conn, int plugin_pwrite (struct connection *conn, - void *buf, uint32_t count, uint64_t offset) + void *buf, uint32_t count, uint64_t offset, int fua) { + int r; assert (dl); assert (connection_get_handle (conn)); - debug ("pwrite count=%" PRIu32 " offset=%" PRIu64, count, offset); + debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, + fua); if (plugin.pwrite != NULL) - return plugin.pwrite (connection_get_handle (conn), buf, count, offset); + r = plugin.pwrite (connection_get_handle (conn), buf, count, offset); else { errno = EROFS; return -1; } + if (r == 0 && fua) { + assert (plugin.flush); + r = plugin_flush (conn); + } + return r; } int @@ -529,24 +536,31 @@ plugin_flush (struct connection *conn) } int -plugin_trim (struct connection *conn, uint32_t count, uint64_t offset) +plugin_trim (struct connection *conn, uint32_t count, uint64_t offset, int fua) { + int r; assert (dl); assert (connection_get_handle (conn)); - debug ("trim count=%" PRIu32 " offset=%" PRIu64, count, offset); + debug ("trim count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, + fua); if (plugin.trim != NULL) - return plugin.trim (connection_get_handle (conn), count, offset); + r = plugin.trim (connection_get_handle (conn), count, offset); else { errno = EINVAL; return -1; } + if (r == 0 && fua) { + assert (plugin.flush); + r = plugin_flush (conn); + } + return r; } int plugin_zero (struct connection *conn, - uint32_t count, uint64_t offset, int may_trim) + uint32_t count, uint64_t offset, int may_trim, int fua) { assert (dl); assert (connection_get_handle (conn)); @@ -555,8 +569,8 @@ plugin_zero (struct connection *conn, int result; int err = 0; - debug ("zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d", - count, offset, may_trim); + debug ("zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d fua=%d", + count, offset, may_trim, fua); if (!count) return 0; @@ -569,7 +583,7 @@ plugin_zero (struct connection *conn, err = errno; } if (result == 0 || err != EOPNOTSUPP) - return result; + goto done; } assert (plugin.pwrite); @@ -593,5 +607,11 @@ plugin_zero (struct connection *conn, err = errno; free (buf); errno = err; + + done: + if (!result && fua) { + assert (plugin.flush); + result = plugin_flush (conn); + } return result; } -- 2.14.3
The NBD protocol supports Forced Unit Access (FUA) as a more efficient way to wait for just one write to land in persistent storage, rather than all outstanding writes at the time of a flush; modeled after the kernel's block I/O flag of the same name. While we can emulate the proper semantics with a full-blown flush, there are some plugins that can properly pass the FUA flag on to the end storage and thereby avoid some overhead. This patch introduces new callbacks and documentations for those callbacks, although the actual implementation to take advantage of the new callbacks will be in later patches. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-plugin.pod | 77 +++++++++++++++++++++++++++++++++++++++++++++++-- docs/nbdkit.pod | 9 ++++-- include/nbdkit-plugin.h | 8 ++++- 3 files changed, 88 insertions(+), 6 deletions(-) diff --git a/docs/nbdkit-plugin.pod b/docs/nbdkit-plugin.pod index 9abf75f..cb936f7 100644 --- a/docs/nbdkit-plugin.pod +++ b/docs/nbdkit-plugin.pod @@ -359,7 +359,7 @@ If there is an error, C<.can_write> should call C<nbdkit_error> with an error message and return C<-1>. This callback is not required. If omitted, then we return true iff a -C<.pwrite> callback has been defined. +C<.pwrite> or C<.pwrite_fua> callback has been defined. =head2 C<.can_flush> @@ -400,7 +400,28 @@ If there is an error, C<.can_trim> should call C<nbdkit_error> with an error message and return C<-1>. This callback is not required. If omitted, then we return true iff a -C<.trim> callback has been defined. +C<.trim> or C<.trim_fua> callback has been defined. + +=head2 C<.can_fua> + + int can_fua (void *handle); + +This is called during the option negotiation phase to find out if the +plugin supports the Forced Unit Access (FUA) flag on write and trim +requests. + +If there is an error, C<.can_fua> should call C<nbdkit_error> with an +error message and return C<-1>. + +This callback is not required. If omitted, then we return true iff +either the C<.pwrite_fua> callback has been defined, or if C<.can_flush> +returns true (in the latter case, FUA semantics are emulated by nbdkit +calling C<.flush> before completing any write or trim operation with +the FUA flag set). + +Note that if this defaults to true and C<.can_trim> also returns true, +the plugin must provide either C<.flush> or C<.trim_fua> for correct +FUA semantics. =head2 C<.pread> @@ -442,6 +463,21 @@ recovered from), C<.pwrite> should call C<nbdkit_error> with an error message, and C<nbdkit_set_error> to record an appropriate error (unless C<errno> is sufficient), then return C<-1>. +If the plugin can provide efficient Forced Unit Access (FUA) semantics, +it should define C<.pwrite_fua> instead. + +=head2 C<.pwrite_fua> + + int pwrite_fua (void *handle, const void *buf, uint32_t count, uint64_t offset, int fua); + +This callback has the same requirements as C<.pwrite>, with the +additional parameter C<fua> set to a non-zero value if the client +wants FUA semantics (where the command must not return until the +actions of the write have landed in persistent storage). If the +plugin cannot provide efficient FUA, but C<.can_flush> returns true +and C<.can_fua> does not return false, then client requests for FUA +semantics are emulated by nbdkit calling C<.flush>. + =head2 C<.flush> int flush (void *handle); @@ -455,6 +491,11 @@ If there is an error, C<.flush> should call C<nbdkit_error> with an error message, and C<nbdkit_set_error> to record an appropriate error (unless C<errno> is sufficient), then return C<-1>. +Note that C<.flush> can be called both by the client doing an explicit +flush request, and by nbdkit when emulating Forced Unit Access (FUA) +semantics after a write or trim where the plugin did not provide FUA +callbacks (C<.pwrite_fua>, C<.zero_fua>, and C<.trim_fua>). + =head2 C<.trim> int trim (void *handle, uint32_t count, uint64_t offset); @@ -467,6 +508,21 @@ If there is an error, C<.trim> should call C<nbdkit_error> with an error message, and C<nbdkit_set_error> to record an appropriate error (unless C<errno> is sufficient), then return C<-1>. +If the plugin can provide efficient Forced Unit Access (FUA) semantics, +it should define C<.trim_fua> instead. + +=head2 C<.trim_fua> + + int trim_fua (void *handle, uint32_t count, uint64_t offset, int fua); + +This callback has the same requirements as C<.trim>, with the +additional parameter C<fua> set to a non-zero value if the client +wants FUA semantics (where the command must not return until the +actions of the trim have landed in persistent storage). If the plugin +cannot provide efficient FUA, but C<.can_flush> returns true and +C<.can_fua> does not return false, then client requests for FUA +semantics are emulated by nbdkit calling C<.flush>. + =head2 C<.zero> int zero (void *handle, uint32_t count, uint64_t offset, int may_trim); @@ -488,6 +544,21 @@ If there is an error, C<.zero> should call C<nbdkit_error> with an error message, and C<nbdkit_set_error> to record an appropriate error (unless C<errno> is sufficient), then return C<-1>. +If the plugin can provide efficient Forced Unit Access (FUA) semantics, +it should define C<.zero_fua> instead. + +=head2 C<.zero_fua> + + int zero_fua (void *handle, uint32_t count, uint64_t offset, int may_trim, int fua); + +This callback has the same requirements as C<.zero>, with the +additional parameter C<fua> set to a non-zero value if the client +wants FUA semantics (where the command must not return until the +actions of the write have landed in persistent storage). If the +plugin cannot provide efficient FUA, but C<.can_flush> returns true +and C<.can_fua> does not return false, then client requests for FUA +semantics are emulated by nbdkit calling C<.flush>. + =head1 THREADS Each nbdkit plugin must declare its thread safety model by defining @@ -711,7 +782,7 @@ Pino Toscano =head1 COPYRIGHT -Copyright (C) 2013-2017 Red Hat Inc. +Copyright (C) 2013-2018 Red Hat Inc. =head1 LICENSE diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod index 1687ac9..2b868d9 100644 --- a/docs/nbdkit.pod +++ b/docs/nbdkit.pod @@ -785,7 +785,12 @@ information about that plugin, eg: [etc] Plugins which ship with nbdkit usually have the same version as the -corresponding nbdkit binary. +corresponding nbdkit binary. The nbdkit binary will always be able +to utilize plugins compiled against an older version of the header; +however, there are cases where a newer plugin may not be fully +supported by an older nbdkit binary (for example, a plugin that +supplies C<.pwrite_fua> but not C<.pwrite> may not support writes +when loaded by the older nbdkit). =head2 Detect if a plugin is installed @@ -895,7 +900,7 @@ Pino Toscano =head1 COPYRIGHT -Copyright (C) 2013-2017 Red Hat Inc. +Copyright (C) 2013-2018 Red Hat Inc. =head1 LICENSE diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index 2ec3b15..d3b0050 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2013-2017 Red Hat Inc. + * Copyright (C) 2013-2018 Red Hat Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without @@ -96,6 +96,12 @@ struct nbdkit_plugin { void (*dump_plugin) (void); + int (*can_fua) (void *handle); + int (*pwrite_fua) (void *handle, const void *buf, uint32_t count, + uint64_t offset, int fua); + int (*zero_fua) (void *handle, uint32_t count, uint64_t offset, int may_trim, + int fua); + int (*trim_fua) (void *handle, uint32_t count, uint64_t offset, int fua); /* int (*set_exportname) (void *handle, const char *exportname); */ }; -- 2.14.3
Implement the logic to actually use FUA callbacks defined by a plugin. This includes separating the tracking of advertising flush vs. fua to clients, as it is feasible that a plugin might support fua but not flush. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/connections.c | 15 ++++++++++++--- src/internal.h | 1 + src/plugins.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++-------- 3 files changed, 59 insertions(+), 11 deletions(-) diff --git a/src/connections.c b/src/connections.c index e4a0a82..2884f0e 100644 --- a/src/connections.c +++ b/src/connections.c @@ -79,6 +79,7 @@ struct connection { int is_rotational; int can_trim; int using_tls; + int can_fua; int sockin, sockout; connection_recv_function recv; @@ -367,10 +368,18 @@ compute_eflags (struct connection *conn, uint16_t *flags) if (fl == -1) return -1; if (fl) { - eflags |= NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA; + eflags |= NBD_FLAG_SEND_FLUSH; conn->can_flush = 1; } + fl = plugin_can_fua (conn); + if (fl == -1) + return -1; + if (fl) { + eflags |= NBD_FLAG_SEND_FUA; + conn->can_fua = 1; + } + fl = plugin_is_rotational (conn); if (fl == -1) return -1; @@ -814,7 +823,7 @@ validate_request (struct connection *conn, *error = EINVAL; return false; } - if (!conn->can_flush && (flags & NBD_CMD_FLAG_FUA)) { + if (!conn->can_fua && (flags & NBD_CMD_FLAG_FUA)) { nbdkit_error ("invalid request: FUA flag not supported"); *error = EINVAL; return false; @@ -873,7 +882,7 @@ handle_request (struct connection *conn, uint16_t cmd, uint16_t flags, uint64_t offset, uint32_t count, void *buf) { - bool fua = conn->can_flush && (flags & NBD_CMD_FLAG_FUA); + bool fua = conn->can_fua && (flags & NBD_CMD_FLAG_FUA); /* The plugin should call nbdkit_set_error() to request a particular error, otherwise we fallback to errno or EIO. */ diff --git a/src/internal.h b/src/internal.h index 3ab08d3..88a2eb8 100644 --- a/src/internal.h +++ b/src/internal.h @@ -163,6 +163,7 @@ extern int plugin_can_write (struct connection *conn); extern int plugin_can_flush (struct connection *conn); extern int plugin_is_rotational (struct connection *conn); extern int plugin_can_trim (struct connection *conn); +extern int plugin_can_fua (struct connection *conn); extern int plugin_pread (struct connection *conn, void *buf, uint32_t count, uint64_t offset); extern int plugin_pwrite (struct connection *conn, void *buf, uint32_t count, uint64_t offset, int fua); extern int plugin_flush (struct connection *conn); diff --git a/src/plugins.c b/src/plugins.c index 61f0cc8..93cc535 100644 --- a/src/plugins.c +++ b/src/plugins.c @@ -271,6 +271,10 @@ plugin_dump_fields (void) HAS (flush); HAS (trim); HAS (zero); + HAS (can_fua); + HAS (pwrite_fua); + HAS (zero_fua); + HAS (trim_fua); #undef HAS /* Custom fields. */ @@ -437,7 +441,7 @@ plugin_can_write (struct connection *conn) if (plugin.can_write) return plugin.can_write (connection_get_handle (conn)); else - return plugin.pwrite != NULL; + return plugin.pwrite || plugin.pwrite_fua; } int @@ -454,6 +458,21 @@ plugin_can_flush (struct connection *conn) return plugin.flush != NULL; } +int +plugin_can_fua (struct connection *conn) +{ + assert (dl); + assert (connection_get_handle (conn)); + + debug ("can_fua"); + + if (plugin.can_fua) + return plugin.can_fua (connection_get_handle (conn)); + if (plugin.pwrite_fua) + return 1; + return plugin_can_flush (conn); +} + int plugin_is_rotational (struct connection *conn) { @@ -506,7 +525,12 @@ plugin_pwrite (struct connection *conn, debug ("pwrite count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, fua); - if (plugin.pwrite != NULL) + if (plugin.pwrite_fua) { + r = plugin.pwrite_fua (connection_get_handle (conn), buf, count, offset, + fua); + fua = 0; + } + else if (plugin.pwrite != NULL) r = plugin.pwrite (connection_get_handle (conn), buf, count, offset); else { errno = EROFS; @@ -545,7 +569,11 @@ plugin_trim (struct connection *conn, uint32_t count, uint64_t offset, int fua) debug ("trim count=%" PRIu32 " offset=%" PRIu64 " fua=%d", count, offset, fua); - if (plugin.trim != NULL) + if (plugin.trim_fua) { + r = plugin.trim_fua (connection_get_handle (conn), count, offset, fua); + fua = 0; + } + else if (plugin.trim) r = plugin.trim (connection_get_handle (conn), count, offset); else { errno = EINVAL; @@ -574,19 +602,25 @@ plugin_zero (struct connection *conn, if (!count) return 0; - if (plugin.zero) { + if (plugin.zero || plugin.zero_fua) { errno = 0; - result = plugin.zero (connection_get_handle (conn), count, offset, may_trim); + result = plugin.zero_fua + ? plugin.zero_fua (connection_get_handle (conn), count, offset, may_trim, + fua) + : plugin.zero (connection_get_handle (conn), count, offset, may_trim); if (result == -1) { err = threadlocal_get_error (); if (!err && plugin_errno_is_preserved ()) err = errno; } - if (result == 0 || err != EOPNOTSUPP) + if (result == 0 || err != EOPNOTSUPP) { + if (plugin.zero_fua) + fua = 0; goto done; + } } - assert (plugin.pwrite); + assert (plugin.pwrite || plugin.pwrite_fua); threadlocal_set_error (0); limit = count < MAX_REQUEST_SIZE ? count : MAX_REQUEST_SIZE; buf = calloc (limit, 1); @@ -595,14 +629,18 @@ plugin_zero (struct connection *conn, return -1; } + /* If we have to emulate FUA, we can flush once at the end rather + than on each write iteration */ while (count) { - result = plugin.pwrite (connection_get_handle (conn), buf, limit, offset); + result = plugin_pwrite (conn, buf, limit, offset, fua && plugin.pwrite_fua); if (result < 0) break; count -= limit; if (count < limit) limit = count; } + if (plugin.pwrite_fua) + fua = 0; err = errno; free (buf); -- 2.14.3
If the target server supports FUA, then we should pass the client flag through rather than emulating things with a less-efficient flush. Signed-off-by: Eric Blake <eblake@redhat.com> --- plugins/nbd/nbd.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/plugins/nbd/nbd.c b/plugins/nbd/nbd.c index c9727f7..09cefe5 100644 --- a/plugins/nbd/nbd.c +++ b/plugins/nbd/nbd.c @@ -616,6 +616,14 @@ nbd_can_trim (void *handle) return h->flags & NBD_FLAG_SEND_TRIM; } +static int +nbd_can_fua (void *handle) +{ + struct handle *h = handle; + + return h->flags & NBD_FLAG_SEND_FUA; +} + /* Read data from the file. */ static int nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset) @@ -631,23 +639,26 @@ nbd_pread (void *handle, void *buf, uint32_t count, uint64_t offset) /* Write data to the file. */ static int -nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset) +nbd_pwrite (void *handle, const void *buf, uint32_t count, uint64_t offset, + int fua) { struct handle *h = handle; int c; /* TODO Auto-fragment this if the client has a larger max transfer limit than the server */ - c = nbd_request_full (h, 0, NBD_CMD_WRITE, offset, count, buf, NULL); + c = nbd_request_full (h, fua ? NBD_CMD_FLAG_FUA : 0, + NBD_CMD_WRITE, offset, count, buf, NULL); return c < 0 ? c : nbd_reply (h, c); } /* Write zeroes to the file. */ static int -nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) +nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim, int fua) { struct handle *h = handle; int c; + int flags = 0; if (!(h->flags & NBD_FLAG_SEND_WRITE_ZEROES)) { /* Trigger a fall back to regular writing */ @@ -655,19 +666,22 @@ nbd_zero (void *handle, uint32_t count, uint64_t offset, int may_trim) return -1; } - c = nbd_request (h, may_trim ? 0 : NBD_CMD_FLAG_NO_HOLE, - NBD_CMD_WRITE_ZEROES, offset, count); + if (!may_trim) + flags |= NBD_CMD_FLAG_NO_HOLE; + if (fua) + flags |= NBD_CMD_FLAG_FUA; + c = nbd_request (h, flags, NBD_CMD_WRITE_ZEROES, offset, count); return c < 0 ? c : nbd_reply (h, c); } /* Trim a portion of the file. */ static int -nbd_trim (void *handle, uint32_t count, uint64_t offset) +nbd_trim (void *handle, uint32_t count, uint64_t offset, int fua) { struct handle *h = handle; int c; - c = nbd_request (h, 0, NBD_CMD_TRIM, offset, count); + c = nbd_request (h, fua ? NBD_CMD_FLAG_FUA : 0, NBD_CMD_TRIM, offset, count); return c < 0 ? c : nbd_reply (h, c); } @@ -697,11 +711,12 @@ static struct nbdkit_plugin plugin = { .can_flush = nbd_can_flush, .is_rotational = nbd_is_rotational, .can_trim = nbd_can_trim, + .can_fua = nbd_can_fua, .pread = nbd_pread, - .pwrite = nbd_pwrite, - .zero = nbd_zero, + .pwrite_fua = nbd_pwrite, + .zero_fua = nbd_zero, .flush = nbd_flush, - .trim = nbd_trim, + .trim_fua = nbd_trim, .errno_is_preserved = 1, }; -- 2.14.3
New nbdkit already knows how to gracefully handle a plugin that defines .pwrite_fua but not .pwrite, making it possible to write a plugin that does not have to provide duplicate boilerplate code. But for maximum cross-version compatibility, we must make sure that older nbdkit, which doesn't know about the existance of the .pwrite_fua callback, can still perform writes when loading such a newer plugin. The trick is to make our registration macro provide sane fallback definitions, marked such that gcc won't warn if the fallbacks are not needed. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit.pod | 11 +++++------ include/nbdkit-plugin.h | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/docs/nbdkit.pod b/docs/nbdkit.pod index 2b868d9..a72c9b1 100644 --- a/docs/nbdkit.pod +++ b/docs/nbdkit.pod @@ -785,12 +785,11 @@ information about that plugin, eg: [etc] Plugins which ship with nbdkit usually have the same version as the -corresponding nbdkit binary. The nbdkit binary will always be able -to utilize plugins compiled against an older version of the header; -however, there are cases where a newer plugin may not be fully -supported by an older nbdkit binary (for example, a plugin that -supplies C<.pwrite_fua> but not C<.pwrite> may not support writes -when loaded by the older nbdkit). +corresponding nbdkit binary. The nbdkit binary will always be able to +utilize plugins compiled against an older version of the header; the +converse direction of an older nbdkit driving a newer plugin is not +guaranteed to work, although the design tries hard to preserve +back-compatibility. =head2 Detect if a plugin is installed diff --git a/include/nbdkit-plugin.h b/include/nbdkit-plugin.h index d3b0050..0c6e67e 100644 --- a/include/nbdkit-plugin.h +++ b/include/nbdkit-plugin.h @@ -117,6 +117,12 @@ extern char *nbdkit_absolute_path (const char *path); extern int64_t nbdkit_parse_size (const char *str); extern int nbdkit_read_password (const char *value, char **password); +#ifdef __GNUC__ +#define NBDKIT_UNUSED __attribute__ (( __unused__ )) +#else +#define NBDKIT_UNUSED /* empty */ +#endif + #ifdef __cplusplus #define NBDKIT_CXX_LANG_C extern "C" #else @@ -124,6 +130,20 @@ extern int nbdkit_read_password (const char *value, char **password); #endif #define NBDKIT_REGISTER_PLUGIN(plugin) \ + static int NBDKIT_UNUSED \ + nbdkit_pwrite_no_fua (void *handle, const void *buf, uint32_t count, \ + uint64_t offset) { \ + return (plugin).pwrite_fua (handle, buf, count, offset, 0); \ + } \ + static int NBDKIT_UNUSED \ + nbdkit_zero_no_fua (void *handle, uint32_t count, uint64_t offset, \ + int may_trim) { \ + return (plugin).zero_fua (handle, count, offset, may_trim, 0); \ + } \ + static int NBDKIT_UNUSED \ + nbdkit_trim_no_fua (void *handle, uint32_t count, uint64_t offset) { \ + return (plugin).trim_fua (handle, count, offset, 0); \ + } \ NBDKIT_CXX_LANG_C \ struct nbdkit_plugin * \ plugin_init (void) \ @@ -131,6 +151,12 @@ extern int nbdkit_read_password (const char *value, char **password); (plugin)._struct_size = sizeof (plugin); \ (plugin)._api_version = NBDKIT_API_VERSION; \ (plugin)._thread_model = THREAD_MODEL; \ + if ((plugin).pwrite_fua && !(plugin).pwrite) \ + (plugin).pwrite = nbdkit_pwrite_no_fua; \ + if ((plugin).zero_fua && !(plugin).zero) \ + (plugin).zero = nbdkit_zero_no_fua; \ + if ((plugin).trim_fua && !(plugin).trim) \ + (plugin).trim = nbdkit_trim_no_fua; \ return &(plugin); \ } -- 2.14.3
I had an idea about how we might do this and still keep a single .pwrite method for plugins. Maybe it's too complicated but here goes: (1) Rename the pwrite method as pwrite_old1, and add new pwrite method with either the FUA flag or general flags: int (*pread) (void *handle, void *buf, uint32_t count, uint64_t offset); - int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t offset); + int (*pwrite_old1) (void *handle, const void *buf, uint32_t count, uint64_t offset); int (*flush) (void *handle); int (*trim) (void *handle, uint32_t count, uint64_t offset); int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim); int errno_is_preserved; void (*dump_plugin) (void); + int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t offset, unsigned flags); This is binary-compatible with old plugins, but not source compatible, so: (2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including <nbdkit-plugin.h>. Code wishing to use the flags variant of pwrite can do: #define NBDKIT_PLUGIN_LEVEL 2 #include <nbdkit-plugin.h> which would modify the definition of the struct (again), something like: #ifndef NBDKIT_PLUGIN_LEVEL #define NBDKIT_PLUGIN_LEVEL 1 #endif ... int (*pread) (void *handle, void *buf, uint32_t count, uint64_t offset); #if NBDKIT_PLUGIN_LEVEL <= 1 int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t offset); #else int (*pwrite_old1) (void *handle, const void *buf, uint32_t count, uint64_t offset); #endif int (*flush) (void *handle); int (*trim) (void *handle, uint32_t count, uint64_t offset); int (*zero) (void *handle, uint32_t count, uint64_t offset, int may_trim); int errno_is_preserved; void (*dump_plugin) (void); #if NBDKIT_PLUGIN_LEVEL <= 1 int (*never_used1) (void *handle, const void *buf, uint32_t count, uint64_t offset, unsigned flags); #else int (*pwrite) (void *handle, const void *buf, uint32_t count, uint64_t offset, unsigned flags); #endif (3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that it always sees the new function, but it may need to call the old function: int plugin_pwrite (...) { if (plugin.pwrite != NULL) plugin.pwrite (handle, buf, count, offset, flags); else if (plugin.pwrite_old1 != NULL) plugin.pwrite_old1 (handle, buf, count, offset); } (4) Internal plugins which don't need the FUA flag can continue to use the level 1 API, which means that we keep checking that the old API still works. What do you think? 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
On 01/16/2018 02:25 AM, Richard W.M. Jones wrote:
I had an idea about how we might do this and still keep a single .pwrite method for plugins. Maybe it's too complicated but here goes:
This is binary-compatible with old plugins, but not source compatible, so:
(2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including <nbdkit-plugin.h>. Code wishing to use the flags variant of pwrite can do:
#define NBDKIT_PLUGIN_LEVEL 2 #include <nbdkit-plugin.h>
which would modify the definition of the struct (again), something like:
#ifndef NBDKIT_PLUGIN_LEVEL #define NBDKIT_PLUGIN_LEVEL 1 #endif
So the difference between your proposal and my patch 4 is that in your proposal, a new plugin has to explicitly opt-in to the new ABI by defining NBDKIT_PLUGIN_LEVEL 2, while in mine, a new plugin opts-in by assigning to .pwrite_fua instead of .pwrite. Yours is slightly cleaner in making it obvious that there is only one (and not two) pwrite callback per plugin, and either way, the plugin chooses which variant it wants.
(3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that it always sees the new function, but it may need to call the old function:
int plugin_pwrite (...) { if (plugin.pwrite != NULL) plugin.pwrite (handle, buf, count, offset, flags); else if (plugin.pwrite_old1 != NULL) plugin.pwrite_old1 (handle, buf, count, offset); }
This part happens whether by your proposal or by mine (see patch 5) - a new nbdkit is always able to call the correct one out of two callbacks for both old and new plugins. Both proposals are still able to install a shim for the old name when the plugin uses only the new semantics; if I use your proposal, my patch 7 would be rewritten to only create the shims when a plugin requests NBDKIT_PLUGIN_LEVEL 2 (whereas by my proposal, I had to define the shims always). Documentation may be a bit tricky under your proposal, but I'm not seeing it as much different than mine. Then there's the decision of whether to go with a single flags parameter (closer to the wire protocol, but the plugin has to decode bits; and hopefully we do proper feature advertisement so that a client never passes a flag that the plugin won't understand) or with multiple bool parameters (what my proposal did for .pwrite_fua).
(4) Internal plugins which don't need the FUA flag can continue to use the level 1 API, which means that we keep checking that the old API still works.
My proposal did that as well - most plugins still used the older spelling without the fua flag.
What do you think?
I don't see any fundamental differences in end behavior, so much as which style we find nicer to document and expose to plugin writers. I'm happy to go with whichever of the two styles you prefer. The other thing to consider is how we translate all of this to other language bindings. For languages that allow function overloading, I suppose we want the declaration of 'pwrite' to take either function signature, then the glue code that converts back to C knows based on the function signature whether the plugin is written for old- or new-style API; for languages without function overloading, they will have to mirror some way of declaring which of the two APIs they are interested in. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
On Tue, Jan 16, 2018 at 07:59:01AM -0600, Eric Blake wrote:
On 01/16/2018 02:25 AM, Richard W.M. Jones wrote:
I had an idea about how we might do this and still keep a single .pwrite method for plugins. Maybe it's too complicated but here goes:
This is binary-compatible with old plugins, but not source compatible, so:
(2) Plugsin may optionally define NBDKIT_PLUGIN_LEVEL before including <nbdkit-plugin.h>. Code wishing to use the flags variant of pwrite can do:
#define NBDKIT_PLUGIN_LEVEL 2 #include <nbdkit-plugin.h>
which would modify the definition of the struct (again), something like:
#ifndef NBDKIT_PLUGIN_LEVEL #define NBDKIT_PLUGIN_LEVEL 1 #endif
So the difference between your proposal and my patch 4 is that in your proposal, a new plugin has to explicitly opt-in to the new ABI by defining NBDKIT_PLUGIN_LEVEL 2, while in mine, a new plugin opts-in by assigning to .pwrite_fua instead of .pwrite. Yours is slightly cleaner in making it obvious that there is only one (and not two) pwrite callback per plugin, and either way, the plugin chooses which variant it wants.
Yes that's a fair summary. I think they have to "opt in" in some way because current plugins as written shouldn't be broken, so they should always see the current pwrite function. (Note that the API/ABI guarantees only apply to C code, not to the other languages)
(3) Core nbdkit code always defines NBDKIT_PLUGIN_LEVEL == 2 so that it always sees the new function, but it may need to call the old function:
int plugin_pwrite (...) { if (plugin.pwrite != NULL) plugin.pwrite (handle, buf, count, offset, flags); else if (plugin.pwrite_old1 != NULL) plugin.pwrite_old1 (handle, buf, count, offset); }
This part happens whether by your proposal or by mine (see patch 5) - a new nbdkit is always able to call the correct one out of two callbacks for both old and new plugins.
Right.
Both proposals are still able to install a shim for the old name when the plugin uses only the new semantics; if I use your proposal, my patch 7 would be rewritten to only create the shims when a plugin requests NBDKIT_PLUGIN_LEVEL 2 (whereas by my proposal, I had to define the shims always).
Right.
Documentation may be a bit tricky under your proposal, but I'm not seeing it as much different than mine. Then there's the decision of whether to go with a single flags parameter (closer to the wire protocol, but the plugin has to decode bits; and hopefully we do proper feature advertisement so that a client never passes a flag that the plugin won't understand) or with multiple bool parameters (what my proposal did for .pwrite_fua).
TBH I would only bother to document NBDKIT_PLUGIN_LEVEL 2, ie. the documentation would be changed to: + #define NBDKIT_PLUGIN_LEVEL 2 #include <nbdkit-plugin.h> and would only document the new pwrite.
(4) Internal plugins which don't need the FUA flag can continue to use the level 1 API, which means that we keep checking that the old API still works.
My proposal did that as well - most plugins still used the older spelling without the fua flag.
Yup.
What do you think?
I don't see any fundamental differences in end behavior, so much as which style we find nicer to document and expose to plugin writers. I'm happy to go with whichever of the two styles you prefer.
I kind of like only have one pwrite method (my version IOW), as the result is much cleaner, but I could be biased :-)
The other thing to consider is how we translate all of this to other language bindings. For languages that allow function overloading, I suppose we want the declaration of 'pwrite' to take either function signature, then the glue code that converts back to C knows based on the function signature whether the plugin is written for old- or new-style API; for languages without function overloading, they will have to mirror some way of declaring which of the two APIs they are interested in.
We can break the language bindings, IOW making them just use the newer form. However as you say for languages like Perl it's easy enough to add an optional FUA parameter so it wouldn't even be a break. I have a large (almost pure) refactoring change which I'll send to you in a moment as it would affect anything you write, although it should only require mechanical changes. 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
participants (2)
-
Eric Blake -
Richard W.M. Jones