While it may be counterintuitive at first, the introduction of NBD_CMD_WRITE_ZEROES and NBD_CMD_BLOCK_STATUS has caused a performance regression in qemu [1], when copying a sparse file. When the destination file must contain the same contents as the source, but it is not known in advance whether the destination started life with all zero content, then there are cases where it is faster to request a bulk zero of the entire device followed by writing only the portions of the device that are to contain data, as that results in fewer I/O transactions overall. In fact, there are even situations where trimming the entire device prior to writing zeroes may be faster than bare write zero request [2]. However, if a bulk zero request ever falls back to the same speed as a normal write, a bulk pre-zeroing algorithm is actually a pessimization, as it ends up writing portions of the disk twice. [1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06389.html [2] https://github.com/libguestfs/nbdkit/commit/407f8dde Hence, it is desirable to have a way for clients to specify that a particular write zero request is being attempted for a fast wipe, and get an immediate failure if the zero request would otherwise take the same time as a write. Conversely, if the client is not performing a pre-initialization pass, it is still more efficient in terms of networking traffic to send NBD_CMD_WRITE_ZERO requests where the server implements the fallback to the slower write, than it is for the client to have to perform the fallback to send NBD_CMD_WRITE with a zeroed buffer. Add a protocol flag and corresponding transmission advertisement flag to make it easier for clients to inform the server of their intent. If the server advertises NBD_FLAG_SEND_FAST_ZERO, then it promises two things: to perform a fallback to write when the client does not request NBD_CMD_FLAG_FAST_ZERO (so that the client benefits from the lower network overhead); and to fail quickly with ENOTSUP, preferably without modifying the export, if the client requested the flag but the server cannot write zeroes more efficiently than a normal write (so that the client is not penalized with the time of writing data areas of the disk twice). Note that the semantics are chosen so that servers should advertise the new flag whether or not they have fast zeroing (that is, this is NOT the server advertising that it has fast zeroes, but rather advertising that the client can get fast feedback as needed on whether zeroing is fast). It is also intentional that the new advertisement includes a new errno value, ENOTSUP, with rules that this error should not be returned for any pre-existing behaviors, must not happen when the client does not request a fast zero, and must be returned quickly if the client requested fast zero but anything other than the error would not be fast; while leaving it possible for clients to distinguish other errors like EINVAL if alignment constraints are not met. Clients should not send the flag unless the server advertised support, but well-behaved servers should already be reporting EINVAL to unrecognized flags. If the server does not advertise the new feature, clients can safely fall back to assuming that writing zeroes is no faster than normal writes (whether or not the assumption actually holds). Note that the Linux fallocate(2) interface may or may not be powerful enough to easily determine if zeroing will be efficient - in particular, FALLOC_FL_ZERO_RANGE in isolation does NOT give that insight; likewise, for block devices, it is known that ioctl(BLKZEROOUT) does NOT have a way for userspace to probe if it is efficient or slow. But with enough demand, the kernel may add another FALLOC_FL_ flag to use with FALLOC_FL_ZERO_RANGE, and/or appropriate ioctls with guaranteed ENOTSUP failures if a fast path cannot be taken. If a server cannot easily determine if write zeroes will be efficient, the server should either fail all NBD_CMD_FLAG_FAST_ZERO with ENOTSUP, or else choose to not advertise NBD_FLAG_SEND_FAST_ZERO. Signed-off-by: Eric Blake <eblake@redhat.com> --- doc/proto.md | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/doc/proto.md b/doc/proto.md index 52d3e7b..702688b 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -1070,6 +1070,18 @@ The field has the following format: which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact. +- bit 11, `NBD_FLAG_SEND_FAST_ZERO`: allow clients to detect whether + `NBD_CMD_WRITE_ZEROES` is faster than a corresponding write. The + server MUST set this transmission flag to 1 if the + `NBD_CMD_WRITE_ZEROES` request supports the `NBD_CMD_FLAG_FAST_ZERO` + flag, and MUST set this transmission flag to 0 if + `NBD_FLAG_SEND_WRITE_ZEROES` is not set. Servers MAY set this this + transmission flag even if it will always use `NBD_ENOTSUP` failures for + requests with `NBD_CMD_FLAG_FAST_ZERO` set (such as if the server + cannot quickly determine whether a particular write zeroes request + will be faster than a regular write). Clients MUST NOT set the + `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag + is set. Clients SHOULD ignore unknown flags. @@ -1647,6 +1659,12 @@ valid may depend on negotiation during the handshake phase. MUST NOT send metadata on more than one extent in the reply. Client implementors should note that using this flag on multiple contiguous requests is likely to be inefficient. +- bit 4, `NBD_CMD_FLAG_FAST_ZERO`; valid during + `NBD_CMD_WRITE_ZEROES`. If set, but the server cannot perform the + write zeroes any faster than it would for an equivalent + `NBD_CMD_WRITE`, then the server MUST fail quickly with an error of + `NBD_ENOTSUP`. The client MUST NOT set this unless the server advertised + `NBD_FLAG_SEND_FAST_ZERO`. ##### Structured reply flags @@ -2015,7 +2033,10 @@ The following request types exist: reached permanent storage, unless `NBD_CMD_FLAG_FUA` is in use. A client MUST NOT send a write zeroes request unless - `NBD_FLAG_SEND_WRITE_ZEROES` was set in the transmission flags field. + `NBD_FLAG_SEND_WRITE_ZEROES` was set in the transmission flags + field. Additionally, a client MUST NOT send the + `NBD_CMD_FLAG_FAST_ZERO` flag unless `NBD_FLAG_SEND_FAST_ZERO` was + set in the transimssion flags field. By default, the server MAY use trimming to zero out the area, even if it did not advertise `NBD_FLAG_SEND_TRIM`; but it MUST ensure @@ -2025,6 +2046,28 @@ The following request types exist: same area will not cause fragmentation or cause failure due to insufficient space. + If the server advertised `NBD_FLAG_SEND_FAST_ZERO` but + `NBD_CMD_FLAG_FAST_ZERO` is not set, then the server MUST NOT fail + with `NBD_ENOTSUP`, even if the operation is no faster than a + corresponding `NBD_CMD_WRITE`. Conversely, if + `NBD_CMD_FLAG_FAST_ZERO` is set, the server MUST fail quickly with + `NBD_ENOTSUP` unless the request can be serviced in less time than + a corresponding `NBD_CMD_WRITE`, and SHOULD NOT alter the contents + of the export when returning this failure. The server's + determination of a fast request MAY depend on a number of factors, + such as whether the request was suitably aligned, on whether the + `NBD_CMD_FLAG_NO_HOLE` flag was present, or even on whether a + previous `NBD_CMD_TRIM` had been performed on the region. If the + server did not advertise `NBD_FLAG_SEND_FAST_ZERO`, then it SHOULD + NOT fail with `NBD_ENOTSUP`, regardless of the speed of servicing + a request, and SHOULD fail with `NBD_EINVAL` if the + `NBD_CMD_FLAG_FAST_ZERO` flag was set. A server MAY advertise + `NBD_FLAG_SEND_FAST_ZERO` whether or not it can perform fast + zeroing; similarly, a server SHOULD fail with `NBD_ENOTSUP` when + the flag is set if the server cannot quickly determine in advance + whether that request would have been fast, even if it turns out + that the same request without the flag would be fast after all. + If an error occurs, the server MUST set the appropriate error code in the error field. @@ -2125,6 +2168,7 @@ The following error values are defined: * `NBD_EINVAL` (22), Invalid argument. * `NBD_ENOSPC` (28), No space left on device. * `NBD_EOVERFLOW` (75), Value too large. +* `NBD_ENOTSUP` (95), Operation not supported. * `NBD_ESHUTDOWN` (108), Server is in the process of being shut down. The server SHOULD return `NBD_ENOSPC` if it receives a write request @@ -2139,6 +2183,10 @@ read-only export. The server SHOULD NOT return `NBD_EOVERFLOW` except as documented in response to `NBD_CMD_READ` when `NBD_CMD_FLAG_DF` is supported. +The server SHOULD NOT return `NBD_ENOTSUP` except as documented in +response to `NBD_CMD_WRITE_ZEROES` when `NBD_CMD_FLAG_FAST_ZERO` is +supported. + The server SHOULD return `NBD_EINVAL` if it receives an unknown command. The server SHOULD return `NBD_EINVAL` if it receives an unknown -- 2.21.0

23.08.2019 17:34, Eric Blake wrote:

While it may be counterintuitive at first, the introduction of NBD_CMD_WRITE_ZEROES and NBD_CMD_BLOCK_STATUS has caused a performance regression in qemu [1], when copying a sparse file. When the destination file must contain the same contents as the source, but it is not known in advance whether the destination started life with all zero content, then there are cases where it is faster to request a bulk zero of the entire device followed by writing only the portions of the device that are to contain data, as that results in fewer I/O transactions overall. In fact, there are even situations where trimming the entire device prior to writing zeroes may be faster than bare write zero request [2]. However, if a bulk zero request ever falls back to the same speed as a normal write, a bulk pre-zeroing algorithm is actually a pessimization, as it ends up writing portions of the disk twice.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg06389.html [2] https://github.com/libguestfs/nbdkit/commit/407f8dde

Hence, it is desirable to have a way for clients to specify that a particular write zero request is being attempted for a fast wipe, and get an immediate failure if the zero request would otherwise take the same time as a write. Conversely, if the client is not performing a pre-initialization pass, it is still more efficient in terms of networking traffic to send NBD_CMD_WRITE_ZERO requests where the server implements the fallback to the slower write, than it is for the client to have to perform the fallback to send NBD_CMD_WRITE with a zeroed buffer.

How are you going to finally use it in qemu-img convert? Ok, we have a loop of sending write-zero requests. And on first ENOTSUP we'll assume that there is no benefit to continue? But what if actually server returns ENOTSUP only once when we have 1000 iterations? Seems we should still do zeroing if we have only a few ENOTSUPs... I understand that fail-on-first ENOTSUP is OK for raw-without-fallocte vs qcow2, as first will always return ENOTSUP and second will never fail.. But in such way we'll OK with simpler extension, which only have one server-advirtised negotiation flag NBD_FLAG_ZERO_IS_FAST. There is not such problem if we have only one iteration, so may be new command FILL_ZERO, filling the whole device by zeros?

Add a protocol flag and corresponding transmission advertisement flag to make it easier for clients to inform the server of their intent. If the server advertises NBD_FLAG_SEND_FAST_ZERO, then it promises two things: to perform a fallback to write when the client does not request NBD_CMD_FLAG_FAST_ZERO (so that the client benefits from the lower network overhead); and to fail quickly with ENOTSUP, preferably without modifying the export, if the client requested the flag but the server cannot write zeroes more efficiently than a normal write (so that the client is not penalized with the time of writing data areas of the disk twice).

Note that the semantics are chosen so that servers should advertise the new flag whether or not they have fast zeroing (that is, this is NOT the server advertising that it has fast zeroes, but rather advertising that the client can get fast feedback as needed on whether zeroing is fast). It is also intentional that the new advertisement includes a new errno value, ENOTSUP, with rules that this error should not be returned for any pre-existing behaviors, must not happen when the client does not request a fast zero, and must be returned quickly if the client requested fast zero but anything other than the error would not be fast; while leaving it possible for clients to distinguish other errors like EINVAL if alignment constraints are not met. Clients should not send the flag unless the server advertised support, but well-behaved servers should already be reporting EINVAL to unrecognized flags. If the server does not advertise the new feature, clients can safely fall back to assuming that writing zeroes is no faster than normal writes (whether or not the assumption actually holds).

Note that the Linux fallocate(2) interface may or may not be powerful enough to easily determine if zeroing will be efficient - in particular, FALLOC_FL_ZERO_RANGE in isolation does NOT give that insight; likewise, for block devices, it is known that ioctl(BLKZEROOUT) does NOT have a way for userspace to probe if it is efficient or slow. But with enough demand, the kernel may add another FALLOC_FL_ flag to use with FALLOC_FL_ZERO_RANGE, and/or appropriate ioctls with guaranteed ENOTSUP failures if a fast path cannot be taken. If a server cannot easily determine if write zeroes will be efficient, the server should either fail all NBD_CMD_FLAG_FAST_ZERO with ENOTSUP, or else choose to not advertise NBD_FLAG_SEND_FAST_ZERO.

Signed-off-by: Eric Blake <eblake@redhat.com> --- doc/proto.md | 50 +++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/doc/proto.md b/doc/proto.md index 52d3e7b..702688b 100644 --- a/doc/proto.md +++ b/doc/proto.md @@ -1070,6 +1070,18 @@ The field has the following format: which support the command without advertising this bit, and conversely that this bit does not guarantee that the command will succeed or have an impact. +- bit 11, `NBD_FLAG_SEND_FAST_ZERO`: allow clients to detect whether + `NBD_CMD_WRITE_ZEROES` is faster than a corresponding write. The + server MUST set this transmission flag to 1 if the + `NBD_CMD_WRITE_ZEROES` request supports the `NBD_CMD_FLAG_FAST_ZERO` + flag, and MUST set this transmission flag to 0 if + `NBD_FLAG_SEND_WRITE_ZEROES` is not set. Servers MAY set this this + transmission flag even if it will always use `NBD_ENOTSUP` failures for + requests with `NBD_CMD_FLAG_FAST_ZERO` set (such as if the server + cannot quickly determine whether a particular write zeroes request + will be faster than a regular write). Clients MUST NOT set the + `NBD_CMD_FLAG_FAST_ZERO` request flag unless this transmission flag + is set.

Clients SHOULD ignore unknown flags.

@@ -1647,6 +1659,12 @@ valid may depend on negotiation during the handshake phase. MUST NOT send metadata on more than one extent in the reply. Client implementors should note that using this flag on multiple contiguous requests is likely to be inefficient. +- bit 4, `NBD_CMD_FLAG_FAST_ZERO`; valid during + `NBD_CMD_WRITE_ZEROES`. If set, but the server cannot perform the + write zeroes any faster than it would for an equivalent + `NBD_CMD_WRITE`, then the server MUST fail quickly with an error of + `NBD_ENOTSUP`. The client MUST NOT set this unless the server advertised + `NBD_FLAG_SEND_FAST_ZERO`.

##### Structured reply flags

@@ -2015,7 +2033,10 @@ The following request types exist: reached permanent storage, unless `NBD_CMD_FLAG_FUA` is in use.

A client MUST NOT send a write zeroes request unless - `NBD_FLAG_SEND_WRITE_ZEROES` was set in the transmission flags field. + `NBD_FLAG_SEND_WRITE_ZEROES` was set in the transmission flags + field. Additionally, a client MUST NOT send the + `NBD_CMD_FLAG_FAST_ZERO` flag unless `NBD_FLAG_SEND_FAST_ZERO` was + set in the transimssion flags field.

By default, the server MAY use trimming to zero out the area, even if it did not advertise `NBD_FLAG_SEND_TRIM`; but it MUST ensure @@ -2025,6 +2046,28 @@ The following request types exist: same area will not cause fragmentation or cause failure due to insufficient space.

+ If the server advertised `NBD_FLAG_SEND_FAST_ZERO` but + `NBD_CMD_FLAG_FAST_ZERO` is not set, then the server MUST NOT fail + with `NBD_ENOTSUP`, even if the operation is no faster than a + corresponding `NBD_CMD_WRITE`. Conversely, if + `NBD_CMD_FLAG_FAST_ZERO` is set, the server MUST fail quickly with + `NBD_ENOTSUP` unless the request can be serviced in less time than + a corresponding `NBD_CMD_WRITE`, and SHOULD NOT alter the contents + of the export when returning this failure. The server's + determination of a fast request MAY depend on a number of factors, + such as whether the request was suitably aligned, on whether the + `NBD_CMD_FLAG_NO_HOLE` flag was present, or even on whether a + previous `NBD_CMD_TRIM` had been performed on the region. If the + server did not advertise `NBD_FLAG_SEND_FAST_ZERO`, then it SHOULD + NOT fail with `NBD_ENOTSUP`, regardless of the speed of servicing + a request, and SHOULD fail with `NBD_EINVAL` if the + `NBD_CMD_FLAG_FAST_ZERO` flag was set. A server MAY advertise + `NBD_FLAG_SEND_FAST_ZERO` whether or not it can perform fast + zeroing; similarly, a server SHOULD fail with `NBD_ENOTSUP` when + the flag is set if the server cannot quickly determine in advance + whether that request would have been fast, even if it turns out + that the same request without the flag would be fast after all. +

What if WRITE_ZERO in the average is faster than WRITE (for example by 20%), but server never can guarantee performance for one WRITE_ZERO operation, do you restrict such case? Hmm, OK, SHOULD is not MUST actually..

If an error occurs, the server MUST set the appropriate error code in the error field.

@@ -2125,6 +2168,7 @@ The following error values are defined: * `NBD_EINVAL` (22), Invalid argument. * `NBD_ENOSPC` (28), No space left on device. * `NBD_EOVERFLOW` (75), Value too large. +* `NBD_ENOTSUP` (95), Operation not supported. * `NBD_ESHUTDOWN` (108), Server is in the process of being shut down.

The server SHOULD return `NBD_ENOSPC` if it receives a write request @@ -2139,6 +2183,10 @@ read-only export. The server SHOULD NOT return `NBD_EOVERFLOW` except as documented in response to `NBD_CMD_READ` when `NBD_CMD_FLAG_DF` is supported.

+The server SHOULD NOT return `NBD_ENOTSUP` except as documented in +response to `NBD_CMD_WRITE_ZEROES` when `NBD_CMD_FLAG_FAST_ZERO` is +supported. + The server SHOULD return `NBD_EINVAL` if it receives an unknown command.

The server SHOULD return `NBD_EINVAL` if it receives an unknown

-- Best regards, Vladimir

23.08.2019 17:37, Eric Blake wrote:

When creating a read-only image, we are still advertising support for TRIM and WRITE_ZEROES to the client, even though the client should not be issuing those commands. But seeing this requires looking across multiple functions:

All callers to nbd_export_new() passed a single flag based solely on whether the export allows writes. Later, we then pass a constant set of flags to nbd_negotiate_options() (namely, the set of flags which we always support, at least for writable images), which is then further dynamically modified based on client requests for structured options. Finally, when processing NBD_OPT_EXPORT_NAME or NBD_OPT_EXPORT_GO we bitwise-or the original caller's flag with the runtime set of flags we've built up over several functions.

Let's refactor things to instead compute a baseline of flags as soon as possible, in nbd_export_new(), and changing the signature for the callers to pass in a simpler bool rather than having to figure out flags. We can then get rid of the 'myflags' parameter to various functions, and instead refer to client for everything we need (we still have to perform a bitwise-OR during NBD_OPT_EXPORT_NAME and NBD_OPT_EXPORT_GO, but it's easier to see what is being computed). This lets us quit advertising senseless flags for read-only images, as well as making the next patch for exposing FAST_ZERO support easier to write.

Signed-off-by: Eric Blake <eblake@redhat.com> --- include/block/nbd.h | 2 +- blockdev-nbd.c | 3 +-- nbd/server.c | 62 +++++++++++++++++++++++++-------------------- qemu-nbd.c | 6 ++--- 4 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/include/block/nbd.h b/include/block/nbd.h index 991fd52a5134..2c87b42dfd48 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -326,7 +326,7 @@ typedef struct NBDClient NBDClient;

NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, uint64_t size, const char *name, const char *desc, - const char *bitmap, uint16_t nbdflags, bool shared, + const char *bitmap, bool readonly, bool shared, void (*close)(NBDExport *), bool writethrough, BlockBackend *on_eject_blk, Error **errp); void nbd_export_close(NBDExport *exp); diff --git a/blockdev-nbd.c b/blockdev-nbd.c index ecfa2ef0adb5..1cdffab4fce1 100644 --- a/blockdev-nbd.c +++ b/blockdev-nbd.c @@ -187,8 +187,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name, writable = false; }

- exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, - writable ? 0 : NBD_FLAG_READ_ONLY, !writable, + exp = nbd_export_new(bs, 0, len, name, NULL, bitmap, !writable, !writable, NULL, false, on_eject_blk, errp); if (!exp) { return; diff --git a/nbd/server.c b/nbd/server.c index 0fb41c6c50ea..b5577828aa44 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -423,14 +423,14 @@ static void nbd_check_meta_export(NBDClient *client)

/* Send a reply to NBD_OPT_EXPORT_NAME. * Return -errno on error, 0 on success. */ -static int nbd_negotiate_handle_export_name(NBDClient *client, - uint16_t myflags, bool no_zeroes, +static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, Error **errp) { char name[NBD_MAX_NAME_SIZE + 1]; char buf[NBD_REPLY_EXPORT_NAME_SIZE] = ""; size_t len; int ret; + uint16_t myflags;

/* Client sends: [20 .. xx] export name (length bytes) @@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, return -EINVAL; }

- trace_nbd_negotiate_new_style_size_flags(client->exp->size, - client->exp->nbdflags | myflags); + myflags = client->exp->nbdflags; + if (client->structured_reply) { + myflags |= NBD_FLAG_SEND_DF; + }

why we cant do just client->exp->nbdflags |= NBD_FLAG_SEND_DF ? Or even do it earlier, when client->structured_reply becomes true?

+ trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags); stq_be_p(buf, client->exp->size); - stw_be_p(buf + 8, client->exp->nbdflags | myflags); + stw_be_p(buf + 8, myflags); len = no_zeroes ? 10 : sizeof(buf); ret = nbd_write(client->ioc, buf, len, errp); if (ret < 0) { @@ -526,8 +529,7 @@ static int nbd_reject_length(NBDClient *client, bool fatal, Error **errp) /* Handle NBD_OPT_INFO and NBD_OPT_GO. * Return -errno on error, 0 if ready for next option, and 1 to move * into transmission phase. */ -static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags, - Error **errp) +static int nbd_negotiate_handle_info(NBDClient *client, Error **errp) { int rc; char name[NBD_MAX_NAME_SIZE + 1];

[..] -- Best regards, Vladimir

On 8/30/19 6:10 PM, Eric Blake wrote:

On 8/30/19 1:00 PM, Vladimir Sementsov-Ogievskiy wrote:

...

23.08.2019 17:37, Eric Blake wrote:

...

When creating a read-only image, we are still advertising support for TRIM and WRITE_ZEROES to the client, even though the client should not be issuing those commands. But seeing this requires looking across multiple functions:

...

...

@@ -458,10 +458,13 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, return -EINVAL; }

- trace_nbd_negotiate_new_style_size_flags(client->exp->size, - client->exp->nbdflags | myflags); + myflags = client->exp->nbdflags; + if (client->structured_reply) { + myflags |= NBD_FLAG_SEND_DF; + }

why we cant do just client->exp->nbdflags |= NBD_FLAG_SEND_DF ?

Because myflags is the runtime flags for _this_ client, while client->exp->nbdflags are the base flags shared by _all_ clients. If client A requests structured reply, but client B does not, then we don't want to advertise DF to client B; but amending client->exp->nbdflags would have that effect.

I stand corrected - it looks like a fresh client->exp is created per client, as evidenced by: diff --git i/nbd/client.c w/nbd/client.c index b9dc829175f9..9e05f1a0e2a3 100644 --- i/nbd/client.c +++ w/nbd/client.c @@ -1011,6 +1011,8 @@ int nbd_receive_negotiate(AioContext *aio_context, QIOChannel *ioc, assert(info->name); trace_nbd_receive_negotiate_name(info->name); + if (getenv ("MY_HACK")) + info->structured_reply = false; result = nbd_start_negotiate(aio_context, ioc, tlscreds, hostname, outioc, info->structured_reply, &zeroes, errp); diff --git i/nbd/server.c w/nbd/server.c index d5078f7468af..6f3a83704fb3 100644 --- i/nbd/server.c +++ w/nbd/server.c @@ -457,6 +457,7 @@ static int nbd_negotiate_handle_export_name(NBDClient *client, bool no_zeroes, myflags = client->exp->nbdflags; if (client->structured_reply) { myflags |= NBD_FLAG_SEND_DF; + client->exp->nbdflags |= NBD_FLAG_SEND_DF; } trace_nbd_negotiate_new_style_size_flags(client->exp->size, myflags); stq_be_p(buf, client->exp->size); $ ./qemu-nbd -r -f raw file -t & $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags nbd://localhost:10809 -c quit 32145@1567207628.519883:nbd_receive_negotiate_size_flags Size is 1049088, export flags 0x48f $ MY_HACK=1 ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags nbd://localhost:10809 -c quit 32156@1567207630.417815:nbd_receive_negotiate_size_flags Size is 1049088, export flags 0x40f $ ~/qemu/qemu-io -r -f raw --trace=nbd_\*size_flags nbd://localhost:10809 -c quit 32167@1567207635.202940:nbd_receive_negotiate_size_flags Size is 1049088, export flags 0x48f The export flags change per client, so I _can_ store into client->exp->nbdflags. Will do that for v2. Meanwhile, this points out a missing feature in libnbd - for testing purposes, it would be really nice to be able to purposefully cripple the client to NOT request structured replies automatically (default enabled, but the ability to turn it off is useful for interop testing, as in this thread). I already recently added a --no-sr flag to nbdkit for a similar reason (but that's creating a server which refuses to advertise, where here I want a guest that refuses to ask). Guess I'll be adding a patch for that, too :) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

Commit fe0480d6 and friends added BDRV_REQ_NO_FALLBACK as a way to avoid wasting time on a preliminary write-zero request that will later be rewritten by actual data, if it is known that the write-zero request will use a slow fallback; but in doing so, could not optimize for NBD. The NBD specification is now considering an extension that will allow passing on those semantics; this patch updates the new protocol bits and 'qemu-nbd --list' output to recognize the bit, as well as the new errno value possible when using the new flag; while upcoming patches will improve the client to use the feature when present, and the server to advertise support for it. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/interop/nbd.txt | 3 ++- include/block/nbd.h | 4 ++++ nbd/common.c | 5 +++++ nbd/server.c | 2 ++ qemu-nbd.c | 1 + 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt index 6dfec7f47647..45118809618e 100644 --- a/docs/interop/nbd.txt +++ b/docs/interop/nbd.txt @@ -53,4 +53,5 @@ the operation of that feature. * 2.12: NBD_CMD_BLOCK_STATUS for "base:allocation" * 3.0: NBD_OPT_STARTTLS with TLS Pre-Shared Keys (PSK), NBD_CMD_BLOCK_STATUS for "qemu:dirty-bitmap:", NBD_CMD_CACHE -* 4.2: NBD_FLAG_CAN_MULTI_CONN for sharable read-only exports +* 4.2: NBD_FLAG_CAN_MULTI_CONN for sharable read-only exports, +NBD_CMD_FLAG_FAST_ZERO diff --git a/include/block/nbd.h b/include/block/nbd.h index 2c87b42dfd48..21550747cf35 100644 --- a/include/block/nbd.h +++ b/include/block/nbd.h @@ -140,6 +140,7 @@ enum { NBD_FLAG_CAN_MULTI_CONN_BIT = 8, /* Multi-client cache consistent */ NBD_FLAG_SEND_RESIZE_BIT = 9, /* Send resize */ NBD_FLAG_SEND_CACHE_BIT = 10, /* Send CACHE (prefetch) */ + NBD_FLAG_SEND_FAST_ZERO_BIT = 11, /* FAST_ZERO flag for WRITE_ZEROES */ }; #define NBD_FLAG_HAS_FLAGS (1 << NBD_FLAG_HAS_FLAGS_BIT) @@ -153,6 +154,7 @@ enum { #define NBD_FLAG_CAN_MULTI_CONN (1 << NBD_FLAG_CAN_MULTI_CONN_BIT) #define NBD_FLAG_SEND_RESIZE (1 << NBD_FLAG_SEND_RESIZE_BIT) #define NBD_FLAG_SEND_CACHE (1 << NBD_FLAG_SEND_CACHE_BIT) +#define NBD_FLAG_SEND_FAST_ZERO (1 << NBD_FLAG_SEND_FAST_ZERO_BIT) /* New-style handshake (global) flags, sent from server to client, and control what will happen during handshake phase. */ @@ -205,6 +207,7 @@ enum { #define NBD_CMD_FLAG_DF (1 << 2) /* don't fragment structured read */ #define NBD_CMD_FLAG_REQ_ONE (1 << 3) /* only one extent in BLOCK_STATUS * reply chunk */ +#define NBD_CMD_FLAG_FAST_ZERO (1 << 4) /* fail if WRITE_ZEROES is not fast */ /* Supported request types */ enum { @@ -270,6 +273,7 @@ static inline bool nbd_reply_type_is_error(int type) #define NBD_EINVAL 22 #define NBD_ENOSPC 28 #define NBD_EOVERFLOW 75 +#define NBD_ENOTSUP 95 #define NBD_ESHUTDOWN 108 /* Details collected by NBD_OPT_EXPORT_NAME and NBD_OPT_GO */ diff --git a/nbd/common.c b/nbd/common.c index cc8b278e541d..ddfe7d118371 100644 --- a/nbd/common.c +++ b/nbd/common.c @@ -201,6 +201,8 @@ const char *nbd_err_lookup(int err) return "ENOSPC"; case NBD_EOVERFLOW: return "EOVERFLOW"; + case NBD_ENOTSUP: + return "ENOTSUP"; case NBD_ESHUTDOWN: return "ESHUTDOWN"; default: @@ -231,6 +233,9 @@ int nbd_errno_to_system_errno(int err) case NBD_EOVERFLOW: ret = EOVERFLOW; break; + case NBD_ENOTSUP: + ret = ENOTSUP; + break; case NBD_ESHUTDOWN: ret = ESHUTDOWN; break; diff --git a/nbd/server.c b/nbd/server.c index b5577828aa44..981bc3cb1151 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -55,6 +55,8 @@ static int system_errno_to_nbd_errno(int err) return NBD_ENOSPC; case EOVERFLOW: return NBD_EOVERFLOW; + case ENOTSUP: + return NBD_ENOTSUP; case ESHUTDOWN: return NBD_ESHUTDOWN; case EINVAL: diff --git a/qemu-nbd.c b/qemu-nbd.c index 079702bb837f..dce52f564b5a 100644 --- a/qemu-nbd.c +++ b/qemu-nbd.c @@ -294,6 +294,7 @@ static int qemu_nbd_client_list(SocketAddress *saddr, QCryptoTLSCreds *tls, [NBD_FLAG_CAN_MULTI_CONN_BIT] = "multi", [NBD_FLAG_SEND_RESIZE_BIT] = "resize", [NBD_FLAG_SEND_CACHE_BIT] = "cache", + [NBD_FLAG_SEND_FAST_ZERO_BIT] = "fast-zero", }; printf(" size: %" PRIu64 "\n", list[i].size); -- 2.21.0

On 8/30/19 1:07 PM, Vladimir Sementsov-Ogievskiy wrote:

23.08.2019 17:37, Eric Blake wrote:

...

Commit fe0480d6 and friends added BDRV_REQ_NO_FALLBACK as a way to avoid wasting time on a preliminary write-zero request that will later be rewritten by actual data, if it is known that the write-zero request will use a slow fallback; but in doing so, could not optimize for NBD. The NBD specification is now considering an extension that will allow passing on those semantics; this patch updates the new protocol bits and 'qemu-nbd --list' output to recognize the bit, as well as the new errno value possible when using the new flag; while upcoming patches will improve the client to use the feature when present, and the server to advertise support for it.

Signed-off-by: Eric Blake <eblake@redhat.com>

...

+++ b/nbd/server.c @@ -55,6 +55,8 @@ static int system_errno_to_nbd_errno(int err) return NBD_ENOSPC; case EOVERFLOW: return NBD_EOVERFLOW; + case ENOTSUP: + return NBD_ENOTSUP;

This may provoke returning NBD_ENOTSUP in other cases, not only new one we are going to add.

Correct. But the spec only said SHOULD avoid ENOTSUP in those other cases, not MUST avoid ENOTSUP; and in practice, either the client that is not suspecting it will treat it the same as NBD_EINVAL, or we've managed to get a slightly better error message across than normal. I don't see that as a real show-stopper. But if it bothers you, note that in nbdkit, I actually coded things up to refuse to send NBD_EOVERFLOW unless NBD_CMD_FLAG_DF was set, and to refuse to send NBD_ENOTSUP unless NBD_CMD_FLAG_FAST_ZERO was set. I could copy that behavior here, if we want qemu to be that much stricter as a server. (Note to self: also check if, when using structured replies to send error messages, in addition to the code, if the string contains strerror(errno) from BEFORE the mapping, rather than after we've lost information to the more-limited NBD_E* values) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

23.08.2019 17:37, Eric Blake wrote:

The client side is fairly straightforward: if the server advertised fast zero support, then we can map that to BDRV_REQ_NO_FALLBACK support. A server that advertises FAST_ZERO but not WRITE_ZEROES is technically broken, but we can ignore that situation as it does not change our behavior.

Signed-off-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

---

Perhaps this is worth merging with the previous patch. --- block/nbd.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/block/nbd.c b/block/nbd.c index beed46fb3414..8339d7106366 100644 --- a/block/nbd.c +++ b/block/nbd.c @@ -1044,6 +1044,10 @@ static int nbd_client_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, if (!(flags & BDRV_REQ_MAY_UNMAP)) { request.flags |= NBD_CMD_FLAG_NO_HOLE; } + if (flags & BDRV_REQ_NO_FALLBACK) { + assert(s->info.flags & NBD_FLAG_SEND_FAST_ZERO); + request.flags |= NBD_CMD_FLAG_FAST_ZERO; + }

if (!bytes) { return 0; @@ -1239,6 +1243,9 @@ static int nbd_client_connect(BlockDriverState *bs, Error **errp) } if (s->info.flags & NBD_FLAG_SEND_WRITE_ZEROES) { bs->supported_zero_flags |= BDRV_REQ_MAY_UNMAP; + if (s->info.flags & NBD_FLAG_SEND_FAST_ZERO) { + bs->supported_zero_flags |= BDRV_REQ_NO_FALLBACK; + } }

s->sioc = sioc;

-- Best regards, Vladimir

Qemu was able to demonstrate that knowing whether a zero operation is fast is useful when copying from one image to another: there is a choice between bulk pre-zeroing and then revisiting the data sections (fewer transactions, but depends on the zeroing to be fast), vs. visiting every portion of the disk only once (more transactions, but no time lost to duplicated I/O due to slow zeroes). As such, the NBD protocol is adding an extension to allow clients to request fast failure when zero is not efficient, from servers that advertise support for the new flag. In nbdkit, start by plumbing a new .can_fast_zero through the backends (although it stays 0 in this patch, later patches will actually expose it to plugins and filters to override). Advertise the flag to the client when the plugin provides a .can_fast_zero, and wire in passing the flag down to .zero in the plugin. In turn, if the flag is set and the implementation .zero fails with ENOTSUP/EOPNOTSUPP, we no longer attempt the .pwrite fallback. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-protocol.pod | 11 +++++++++ server/internal.h | 2 ++ common/protocol/protocol.h | 11 +++++---- server/filters.c | 20 +++++++++++++--- server/plugins.c | 28 +++++++++++++++++++--- server/protocol-handshake.c | 7 ++++++ server/protocol.c | 46 +++++++++++++++++++++++++++++-------- include/nbdkit-common.h | 7 +++--- plugins/ocaml/ocaml.c | 1 - plugins/sh/call.c | 1 - 10 files changed, 110 insertions(+), 24 deletions(-) diff --git a/docs/nbdkit-protocol.pod b/docs/nbdkit-protocol.pod index 35db07b3..76c50bb8 100644 --- a/docs/nbdkit-protocol.pod +++ b/docs/nbdkit-protocol.pod @@ -173,6 +173,17 @@ This protocol extension allows a client to inform the server about intent to access a portion of the export, to allow the server an opportunity to cache things appropriately. +=item C<NBD_CMD_FLAG_FAST_ZERO> + +Supported in nbdkit E<ge> 1.13.9. + +This protocol extension allows a server to advertise that it can rank +all zero requests as fast or slow, at which point the client can make +fast zero requests which fail immediately with C<ENOTSUP> if the +request is no faster than a counterpart write would be, while normal +zero requests still benefit from compressed network traffic regardless +of the time taken. + =item Resize Extension I<Not supported>. diff --git a/server/internal.h b/server/internal.h index 22e13b6d..784c8b5c 100644 --- a/server/internal.h +++ b/server/internal.h @@ -168,6 +168,7 @@ struct connection { bool is_rotational; bool can_trim; bool can_zero; + bool can_fast_zero; bool can_fua; bool can_multi_conn; bool can_cache; @@ -275,6 +276,7 @@ struct backend { int (*is_rotational) (struct backend *, struct connection *conn); int (*can_trim) (struct backend *, struct connection *conn); int (*can_zero) (struct backend *, struct connection *conn); + int (*can_fast_zero) (struct backend *, struct connection *conn); int (*can_extents) (struct backend *, struct connection *conn); int (*can_fua) (struct backend *, struct connection *conn); int (*can_multi_conn) (struct backend *, struct connection *conn); diff --git a/common/protocol/protocol.h b/common/protocol/protocol.h index e9386430..bf548390 100644 --- a/common/protocol/protocol.h +++ b/common/protocol/protocol.h @@ -96,6 +96,7 @@ extern const char *name_of_nbd_flag (int); #define NBD_FLAG_SEND_DF (1 << 7) #define NBD_FLAG_CAN_MULTI_CONN (1 << 8) #define NBD_FLAG_SEND_CACHE (1 << 10) +#define NBD_FLAG_SEND_FAST_ZERO (1 << 11) /* NBD options (new style handshake only). */ extern const char *name_of_nbd_opt (int); @@ -223,10 +224,11 @@ extern const char *name_of_nbd_cmd (int); #define NBD_CMD_BLOCK_STATUS 7 extern const char *name_of_nbd_cmd_flag (int); -#define NBD_CMD_FLAG_FUA (1<<0) -#define NBD_CMD_FLAG_NO_HOLE (1<<1) -#define NBD_CMD_FLAG_DF (1<<2) -#define NBD_CMD_FLAG_REQ_ONE (1<<3) +#define NBD_CMD_FLAG_FUA (1<<0) +#define NBD_CMD_FLAG_NO_HOLE (1<<1) +#define NBD_CMD_FLAG_DF (1<<2) +#define NBD_CMD_FLAG_REQ_ONE (1<<3) +#define NBD_CMD_FLAG_FAST_ZERO (1<<4) /* Error codes (previously errno). * See http://git.qemu.org/?p=qemu.git;a=commitdiff;h=ca4414804114fd0095b317785bc0b... @@ -239,6 +241,7 @@ extern const char *name_of_nbd_error (int); #define NBD_EINVAL 22 #define NBD_ENOSPC 28 #define NBD_EOVERFLOW 75 +#define NBD_ENOTSUP 95 #define NBD_ESHUTDOWN 108 #endif /* NBDKIT_PROTOCOL_H */ diff --git a/server/filters.c b/server/filters.c index 14ca0cc6..0dd2393e 100644 --- a/server/filters.c +++ b/server/filters.c @@ -403,8 +403,11 @@ next_zero (void *nxdata, uint32_t count, uint64_t offset, uint32_t flags, int r; r = b_conn->b->zero (b_conn->b, b_conn->conn, count, offset, flags, err); - if (r == -1) - assert (*err && *err != ENOTSUP && *err != EOPNOTSUPP); + if (r == -1) { + assert (*err); + if (!(flags & NBDKIT_FLAG_FAST_ZERO)) + assert (*err != ENOTSUP && *err != EOPNOTSUPP); + } return r; } @@ -586,6 +589,15 @@ filter_can_zero (struct backend *b, struct connection *conn) return f->backend.next->can_zero (f->backend.next, conn); } +static int +filter_can_fast_zero (struct backend *b, struct connection *conn) +{ + struct backend_filter *f = container_of (b, struct backend_filter, backend); + + debug ("%s: can_fast_zero", f->name); + return 0; /* Next patch will query or pass through */ +} + static int filter_can_extents (struct backend *b, struct connection *conn) { @@ -738,7 +750,8 @@ filter_zero (struct backend *b, struct connection *conn, void *handle = connection_get_handle (conn, f->backend.i); struct b_conn nxdata = { .b = f->backend.next, .conn = conn }; - assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); + assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA | + NBDKIT_FLAG_FAST_ZERO))); debug ("%s: zero count=%" PRIu32 " offset=%" PRIu64 " flags=0x%" PRIx32, f->name, count, offset, flags); @@ -818,6 +831,7 @@ static struct backend filter_functions = { .is_rotational = filter_is_rotational, .can_trim = filter_can_trim, .can_zero = filter_can_zero, + .can_fast_zero = filter_can_fast_zero, .can_extents = filter_can_extents, .can_fua = filter_can_fua, .can_multi_conn = filter_can_multi_conn, diff --git a/server/plugins.c b/server/plugins.c index 34cc3cb5..c6dcf408 100644 --- a/server/plugins.c +++ b/server/plugins.c @@ -401,6 +401,16 @@ plugin_can_zero (struct backend *b, struct connection *conn) return plugin_can_write (b, conn); } +static int +plugin_can_fast_zero (struct backend *b, struct connection *conn) +{ + assert (connection_get_handle (conn, 0)); + + debug ("can_fast_zero"); + + return 0; /* Upcoming patch will actually add support. */ +} + static int plugin_can_extents (struct backend *b, struct connection *conn) { @@ -621,15 +631,18 @@ plugin_zero (struct backend *b, struct connection *conn, int r = -1; bool may_trim = flags & NBDKIT_FLAG_MAY_TRIM; bool fua = flags & NBDKIT_FLAG_FUA; + bool fast_zero = flags & NBDKIT_FLAG_FAST_ZERO; bool emulate = false; bool need_flush = false; int can_zero = 1; /* TODO cache this per-connection? */ assert (connection_get_handle (conn, 0)); - assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA))); + assert (!(flags & ~(NBDKIT_FLAG_MAY_TRIM | NBDKIT_FLAG_FUA | + NBDKIT_FLAG_FAST_ZERO))); - debug ("zero count=%" PRIu32 " offset=%" PRIu64 " may_trim=%d fua=%d", - count, offset, may_trim, fua); + debug ("zero count=%" PRIu32 " offset=%" PRIu64 + " may_trim=%d fua=%d fast_zero=%d", + count, offset, may_trim, fua, fast_zero); if (fua && plugin_can_fua (b, conn) != NBDKIT_FUA_NATIVE) { flags &= ~NBDKIT_FLAG_FUA; @@ -643,6 +656,8 @@ plugin_zero (struct backend *b, struct connection *conn, } if (can_zero) { + /* if (!can_fast_zero) */ + flags &= ~NBDKIT_FLAG_FAST_ZERO; errno = 0; if (p->plugin.zero) r = p->plugin.zero (connection_get_handle (conn, 0), count, offset, @@ -658,6 +673,12 @@ plugin_zero (struct backend *b, struct connection *conn, goto done; } + if (fast_zero) { + assert (r == -1); + *err = EOPNOTSUPP; + goto done; + } + assert (p->plugin.pwrite || p->plugin._pwrite_old); flags &= ~NBDKIT_FLAG_MAY_TRIM; threadlocal_set_error (0); @@ -762,6 +783,7 @@ static struct backend plugin_functions = { .is_rotational = plugin_is_rotational, .can_trim = plugin_can_trim, .can_zero = plugin_can_zero, + .can_fast_zero = plugin_can_fast_zero, .can_extents = plugin_can_extents, .can_fua = plugin_can_fua, .can_multi_conn = plugin_can_multi_conn, diff --git a/server/protocol-handshake.c b/server/protocol-handshake.c index 0f3bd280..84fcacfd 100644 --- a/server/protocol-handshake.c +++ b/server/protocol-handshake.c @@ -66,6 +66,13 @@ protocol_compute_eflags (struct connection *conn, uint16_t *flags) if (fl) { eflags |= NBD_FLAG_SEND_WRITE_ZEROES; conn->can_zero = true; + fl = backend->can_fast_zero (backend, conn); + if (fl == -1) + return -1; + if (fl) { + eflags |= NBD_FLAG_SEND_FAST_ZERO; + conn->can_fast_zero = true; + } } fl = backend->can_trim (backend, conn); diff --git a/server/protocol.c b/server/protocol.c index 72f6f2a8..77a045dc 100644 --- a/server/protocol.c +++ b/server/protocol.c @@ -110,7 +110,8 @@ validate_request (struct connection *conn, /* Validate flags */ if (flags & ~(NBD_CMD_FLAG_FUA | NBD_CMD_FLAG_NO_HOLE | - NBD_CMD_FLAG_DF | NBD_CMD_FLAG_REQ_ONE)) { + NBD_CMD_FLAG_DF | NBD_CMD_FLAG_REQ_ONE | + NBD_CMD_FLAG_FAST_ZERO)) { nbdkit_error ("invalid request: unknown flag (0x%x)", flags); *error = EINVAL; return false; @@ -121,6 +122,21 @@ validate_request (struct connection *conn, *error = EINVAL; return false; } + if (flags & NBD_CMD_FLAG_FAST_ZERO) { + if (cmd != NBD_CMD_WRITE_ZEROES) { + nbdkit_error ("invalid request: " + "FAST_ZERO flag needs WRITE_ZEROES request"); + *error = EINVAL; + return false; + } + if (!conn->can_fast_zero) { + nbdkit_error ("invalid request: " + "%s: fast zero support was not advertised", + name_of_nbd_cmd (cmd)); + *error = EINVAL; + return false; + } + } if (flags & NBD_CMD_FLAG_DF) { if (cmd != NBD_CMD_READ) { nbdkit_error ("invalid request: DF flag needs READ request"); @@ -297,8 +313,12 @@ handle_request (struct connection *conn, f |= NBDKIT_FLAG_MAY_TRIM; if (fua) f |= NBDKIT_FLAG_FUA; + if (flags & NBD_CMD_FLAG_FAST_ZERO) + f |= NBDKIT_FLAG_FAST_ZERO; if (backend->zero (backend, conn, count, offset, f, &err) == -1) { - assert (err && err != ENOTSUP && err != EOPNOTSUPP); + assert (err); + if (!(flags & NBD_CMD_FLAG_FAST_ZERO)) + assert (err != ENOTSUP && err != EOPNOTSUPP); return err; } break; @@ -368,7 +388,7 @@ skip_over_write_buffer (int sock, size_t count) /* Convert a system errno to an NBD_E* error code. */ static int -nbd_errno (int error, bool flag_df) +nbd_errno (int error, uint16_t flags) { switch (error) { case 0: @@ -390,10 +410,17 @@ nbd_errno (int error, bool flag_df) case ESHUTDOWN: return NBD_ESHUTDOWN; #endif + case ENOTSUP: +#if ENOTSUP != EOPNOTSUPP + case EOPNOTSUPP: +#endif + if (flags & NBD_CMD_FLAG_FAST_ZERO) + return NBD_ENOTSUP; + return NBD_EINVAL; case EOVERFLOW: - if (flag_df) + if (flags & NBD_CMD_FLAG_DF) return NBD_EOVERFLOW; - /* fallthrough */ + return NBD_EINVAL; case EINVAL: default: return NBD_EINVAL; @@ -402,7 +429,7 @@ nbd_errno (int error, bool flag_df) static int send_simple_reply (struct connection *conn, - uint64_t handle, uint16_t cmd, + uint64_t handle, uint16_t cmd, uint16_t flags, const char *buf, uint32_t count, uint32_t error) { @@ -413,7 +440,7 @@ send_simple_reply (struct connection *conn, reply.magic = htobe32 (NBD_SIMPLE_REPLY_MAGIC); reply.handle = handle; - reply.error = htobe32 (nbd_errno (error, false)); + reply.error = htobe32 (nbd_errno (error, flags)); r = conn->send (conn, &reply, sizeof reply, f); if (r == -1) { @@ -640,7 +667,7 @@ send_structured_reply_error (struct connection *conn, } /* Send the error. */ - error_data.error = htobe32 (nbd_errno (error, flags & NBD_CMD_FLAG_DF)); + error_data.error = htobe32 (nbd_errno (error, flags)); error_data.len = htobe16 (0); r = conn->send (conn, &error_data, sizeof error_data, 0); if (r == -1) { @@ -790,5 +817,6 @@ protocol_recv_request_send_reply (struct connection *conn) error); } else - return send_simple_reply (conn, request.handle, cmd, buf, count, error); + return send_simple_reply (conn, request.handle, cmd, flags, buf, count, + error); } diff --git a/include/nbdkit-common.h b/include/nbdkit-common.h index e004aa18..f33ce133 100644 --- a/include/nbdkit-common.h +++ b/include/nbdkit-common.h @@ -57,9 +57,10 @@ extern "C" { #define NBDKIT_THREAD_MODEL_SERIALIZE_REQUESTS 2 #define NBDKIT_THREAD_MODEL_PARALLEL 3 -#define NBDKIT_FLAG_MAY_TRIM (1<<0) /* Maps to !NBD_CMD_FLAG_NO_HOLE */ -#define NBDKIT_FLAG_FUA (1<<1) /* Maps to NBD_CMD_FLAG_FUA */ -#define NBDKIT_FLAG_REQ_ONE (1<<2) /* Maps to NBD_CMD_FLAG_REQ_ONE */ +#define NBDKIT_FLAG_MAY_TRIM (1<<0) /* Maps to !NBD_CMD_FLAG_NO_HOLE */ +#define NBDKIT_FLAG_FUA (1<<1) /* Maps to NBD_CMD_FLAG_FUA */ +#define NBDKIT_FLAG_REQ_ONE (1<<2) /* Maps to NBD_CMD_FLAG_REQ_ONE */ +#define NBDKIT_FLAG_FAST_ZERO (1<<3) /* Maps to NBD_CMD_FLAG_FAST_ZERO */ #define NBDKIT_FUA_NONE 0 #define NBDKIT_FUA_EMULATE 1 diff --git a/plugins/ocaml/ocaml.c b/plugins/ocaml/ocaml.c index c472281f..144a449e 100644 --- a/plugins/ocaml/ocaml.c +++ b/plugins/ocaml/ocaml.c @@ -857,7 +857,6 @@ ocaml_nbdkit_set_error (value nv) case 5: err = ENOSPC; break; case 6: err = ESHUTDOWN; break; case 7: err = EOVERFLOW; break; - /* Necessary for .zero support */ case 8: err = EOPNOTSUPP; break; /* Other errno values that server/protocol.c treats specially */ case 9: err = EROFS; break; diff --git a/plugins/sh/call.c b/plugins/sh/call.c index b86e7c9c..ab43e5ea 100644 --- a/plugins/sh/call.c +++ b/plugins/sh/call.c @@ -347,7 +347,6 @@ handle_script_error (char *ebuf, size_t len) err = ESHUTDOWN; skip = 9; } - /* Necessary for .zero support */ else if (strncasecmp (ebuf, "ENOTSUP", 7) == 0) { err = ENOTSUP; skip = 7; -- 2.21.0

Allow filters to affect the handling of the new NBD_CMD_FLAG_FAST_ZERO flag, then update affected filters. In particular, the log filter logs the state of the new flag (requires a tweak to expected output in test-fua.sh), the delay filter gains a bool parameter delay-fast-zero, several filters reject all fast requests because of local writes or splitting a single client request into multiple plugin requests, and the nozero filter gains additional modes for controlling fast zero advertisement and support: zeromode→ none emulate notrim plugin ↓fastzeromode (new) --------------------------------------------- default 0 2 4 4 none 0 1 1 1 slow 0 2 2 2 ignore 0 3 3 3 0 - no zero advertised, thus no fast zero advertised 1 - fast zero not advertised 2 - fast zero advertised, but fast zero requests fail with ENOTSUP (ie. a fast zero was not possible) 3 - fast zero advertised, but fast zero requests are treated the same as normal requests (ignoring the fast zero flag, aids testing at the probable cost of spec non-compliance) 4 - fast zero advertisement/reaction is up to the plugin Mode none/default remains the default for back-compat, and mode plugin/default has no semantic change compared to omitting the nozero filter from the command line. Filters untouched by this patch are fine inheriting whatever fast-zero behavior the underlying plugin uses. Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/nbdkit-filter.pod | 27 +++++++---- filters/delay/nbdkit-delay-filter.pod | 15 +++++- filters/log/nbdkit-log-filter.pod | 2 +- filters/nozero/nbdkit-nozero-filter.pod | 41 +++++++++++++--- server/filters.c | 15 +++++- include/nbdkit-filter.h | 3 ++ filters/blocksize/blocksize.c | 12 +++++ filters/cache/cache.c | 20 ++++++++ filters/cow/cow.c | 20 ++++++++ filters/delay/delay.c | 28 ++++++++++- filters/log/log.c | 16 ++++--- filters/nozero/nozero.c | 62 +++++++++++++++++++++++-- filters/truncate/truncate.c | 15 ++++++ tests/test-fua.sh | 4 +- 14 files changed, 248 insertions(+), 32 deletions(-) diff --git a/docs/nbdkit-filter.pod b/docs/nbdkit-filter.pod index 6e2bea61..ebce8961 100644 --- a/docs/nbdkit-filter.pod +++ b/docs/nbdkit-filter.pod @@ -361,6 +361,8 @@ calls. =head2 C<.can_zero> +=head2 C<.can_fast_zero> + =head2 C<.can_extents> =head2 C<.can_fua> @@ -380,6 +382,8 @@ calls. void *handle); int (*can_zero) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); + int (*can_fast_zero) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle); int (*can_extents) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -517,22 +521,25 @@ turn, the filter should not call C<next_ops-E<gt>zero> if C<next_ops-E<gt>can_zero> did not return true. On input, the parameter C<flags> may include C<NBDKIT_FLAG_MAY_TRIM> -unconditionally, and C<NBDKIT_FLAG_FUA> based on the result of -C<.can_fua>. In turn, the filter may pass C<NBDKIT_FLAG_MAY_TRIM> -unconditionally, but should only pass C<NBDKIT_FLAG_FUA> on to -C<next_ops-E<gt>zero> if C<next_ops-E<gt>can_fua> returned a positive -value. +unconditionally, C<NBDKIT_FLAG_FUA> based on the result of +C<.can_fua>, and C<NBDKIT_FLAG_FAST_ZERO> based on the result of +C<.can_fast_zero>. In turn, the filter may pass +C<NBDKIT_FLAG_MAY_TRIM> unconditionally, but should only pass +C<NBDKIT_FLAG_FUA> or C<NBDKIT_FLAG_FAST_ZERO> on to +C<next_ops-E<gt>zero> if the corresponding C<next_ops-E<gt>can_fua> or +C<next_ops-E<gt>can_fast_zero> returned a positive value. Note that unlike the plugin C<.zero> which is permitted to fail with C<ENOTSUP> or C<EOPNOTSUPP> to force a fallback to C<.pwrite>, the -function C<next_ops-E<gt>zero> will never fail with C<err> set to -C<ENOTSUP> or C<EOPNOTSUPP> because the fallback has already taken -place. +function C<next_ops-E<gt>zero> will not fail with C<err> set to +C<ENOTSUP> or C<EOPNOTSUPP> unless C<NBDKIT_FLAG_FAST_ZERO> was used, +because otherwise the fallback has already taken place. If there is an error, C<.zero> should call C<nbdkit_error> with an error message B<and> return -1 with C<err> set to the positive errno -value to return to the client. The filter should never fail with -C<ENOTSUP> or C<EOPNOTSUPP> (while plugins have automatic fallback to +value to return to the client. The filter should not fail with +C<ENOTSUP> or C<EOPNOTSUPP> unless C<flags> includes +C<NBDKIT_FLAG_FAST_ZERO> (while plugins have automatic fallback to C<.pwrite>, filters do not). =head2 C<.extents> diff --git a/filters/delay/nbdkit-delay-filter.pod b/filters/delay/nbdkit-delay-filter.pod index 730cea4c..0a9c77f7 100644 --- a/filters/delay/nbdkit-delay-filter.pod +++ b/filters/delay/nbdkit-delay-filter.pod @@ -12,6 +12,7 @@ nbdkit-delay-filter - nbdkit delay filter delay-read=(SECS|NNms) delay-write=(SECS|NNms) delay-zero=(SECS|NNms) delay-trim=(SECS|NNms) delay-extents=(SECS|NNms) delay-cache=(SECS|NNms) + delay-fast-zero=BOOL =head1 DESCRIPTION @@ -56,7 +57,8 @@ Delay write operations by C<SECS> seconds or C<NN> milliseconds. =item B<delay-zero=>NNB<ms> -Delay zero operations by C<SECS> seconds or C<NN> milliseconds. +Delay zero operations by C<SECS> seconds or C<NN> milliseconds. See +also B<delay-fast-zero>. =item B<delay-trim=>SECS @@ -85,6 +87,17 @@ milliseconds. Delay write, zero and trim operations by C<SECS> seconds or C<NN> milliseconds. +=item B<delay-fast-zero=>BOOL + +The NBD specification documents an extension called fast zero, in +which the client may request that a server should reply with +C<ENOTSUP> as soon as possible if the zero operation offers no real +speedup over a corresponding write. By default, this parameter is +true, and fast zero requests are serviced by the plugin after the same +delay as any other zero request; but setting this parameter to false +instantly fails a fast zero response without waiting for or consulting +the plugin. + =back =head1 SEE ALSO diff --git a/filters/log/nbdkit-log-filter.pod b/filters/log/nbdkit-log-filter.pod index 9e102bc0..5d9625a1 100644 --- a/filters/log/nbdkit-log-filter.pod +++ b/filters/log/nbdkit-log-filter.pod @@ -55,7 +55,7 @@ on the connection). An example logging session of a client that performs a single successful read is: - 2018-01-27 20:38:22.959984 connection=1 Connect size=0x400 write=1 flush=1 rotational=0 trim=0 zero=1 fua=1 + 2018-01-27 20:38:22.959984 connection=1 Connect size=0x400 write=1 flush=1 rotational=0 trim=0 zero=1 fua=1 extents=1 cache=0 fast_zero=0 2018-01-27 20:38:23.001720 connection=1 Read id=1 offset=0x0 count=0x100 ... 2018-01-27 20:38:23.001995 connection=1 ...Read id=1 return=0 (Success) 2018-01-27 20:38:23.044259 connection=1 Disconnect transactions=1 diff --git a/filters/nozero/nbdkit-nozero-filter.pod b/filters/nozero/nbdkit-nozero-filter.pod index 144b8230..4fc7dc63 100644 --- a/filters/nozero/nbdkit-nozero-filter.pod +++ b/filters/nozero/nbdkit-nozero-filter.pod @@ -4,7 +4,8 @@ nbdkit-nozero-filter - nbdkit nozero filter =head1 SYNOPSIS - nbdkit --filter=nozero plugin [zeromode=MODE] [plugin-args...] + nbdkit --filter=nozero plugin [plugin-args...] \ + [zeromode=MODE] [fastzeromode=MODE] =head1 DESCRIPTION @@ -18,7 +19,7 @@ testing client or server fallbacks. =over 4 -=item B<zeromode=none|emulate|notrim> +=item B<zeromode=none|emulate|notrim|plugin> Optional, controls which mode the filter will use. Mode B<none> (default) means that zero support is not advertised to the @@ -29,8 +30,30 @@ efficient way to write zeros. Since nbdkit E<ge> 1.13.4, mode B<notrim> means that zero requests are forwarded on to the plugin, except that the plugin will never see the NBDKIT_MAY_TRIM flag, to determine if the client permitting trimming during zero operations -makes a difference (it is an error to request this mode if the plugin -does not support the C<zero> callback). +makes a difference. Since nbdkit E<ge> 1.13.9, mode B<plugin> leaves +normal zero requests up to the plugin, useful when combined with +C<fastzeromode> for experimenting with the effects of fast zero +requests. It is an error to request B<notrim> or B<plugin> if the +plugin does not support the C<zero> callback. + +=item B<fastzeromode=none|slow|ignore|default> + +Optional since nbdkit E<ge> 1.13.9, controls whether fast zeroes are +advertised to the client, and if so, how the filter will react to a +client fast zero request. Mode B<none> avoids advertising fast zero +support. Mode B<slow> advertises fast zero support unconditionally, +but treats all fast zero requests as an immediate C<ENOTSUP> failure +rather than performing a fallback. Mode B<ignore> advertises fast +zero support, but treats all client fast zero requests as if the flag +had not been used (this behavior is typically contrary to the NBD +specification, but can be useful for comparison against the actual +fast zero implementation to see if fast zeroes make a difference). +Mode B<default> is selected by default; when paired with +C<zeromode=emulate>, fast zeroes are advertised but fast zero requests +always fail (similar to C<slow>); when paired with C<zeromode=notrim> +or C<zeromode=plugin>, fast zero support is left to the plugin +(although in the latter case, the nozero filter could be omitted for +the same behavior). =back @@ -42,11 +65,17 @@ explicitly rather than with C<NBD_CMD_WRITE_ZEROES>: nbdkit --filter=nozero file disk.img Serve the file F<disk.img>, allowing the client to take advantage of -less network traffic via C<NBD_CMD_WRITE_ZEROES>, but still forcing -the data to be written explicitly rather than punching any holes: +less network traffic via C<NBD_CMD_WRITE_ZEROES>, but fail any fast +zero requests up front and force all other zero requests to write data +explicitly rather than punching any holes: nbdkit --filter=nozero file zeromode=emulate disk.img +Serve the file F<disk.img>, but do not advertise fast zero support to +the client even if the plugin supports it: + + nbdkit --filter=nozero file zeromode=plugin fastzeromode=none disk.img + =head1 SEE ALSO L<nbdkit(1)>, diff --git a/server/filters.c b/server/filters.c index 0dd2393e..f2de5e4e 100644 --- a/server/filters.c +++ b/server/filters.c @@ -314,6 +314,13 @@ next_can_zero (void *nxdata) return b_conn->b->can_zero (b_conn->b, b_conn->conn); } +static int +next_can_fast_zero (void *nxdata) +{ + struct b_conn *b_conn = nxdata; + return b_conn->b->can_fast_zero (b_conn->b, b_conn->conn); +} + static int next_can_extents (void *nxdata) { @@ -445,6 +452,7 @@ static struct nbdkit_next_ops next_ops = { .is_rotational = next_is_rotational, .can_trim = next_can_trim, .can_zero = next_can_zero, + .can_fast_zero = next_can_fast_zero, .can_extents = next_can_extents, .can_fua = next_can_fua, .can_multi_conn = next_can_multi_conn, @@ -593,9 +601,14 @@ static int filter_can_fast_zero (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_fast_zero", f->name); - return 0; /* Next patch will query or pass through */ + if (f->filter.can_fast_zero) + return f->filter.can_fast_zero (&next_ops, &nxdata, handle); + else + return f->backend.next->can_fast_zero (f->backend.next, conn); } static int diff --git a/include/nbdkit-filter.h b/include/nbdkit-filter.h index 94f17789..d11cf881 100644 --- a/include/nbdkit-filter.h +++ b/include/nbdkit-filter.h @@ -71,6 +71,7 @@ struct nbdkit_next_ops { int (*is_rotational) (void *nxdata); int (*can_trim) (void *nxdata); int (*can_zero) (void *nxdata); + int (*can_fast_zero) (void *nxdata); int (*can_extents) (void *nxdata); int (*can_fua) (void *nxdata); int (*can_multi_conn) (void *nxdata); @@ -139,6 +140,8 @@ struct nbdkit_filter { void *handle); int (*can_zero) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); + int (*can_fast_zero) (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle); int (*can_extents) (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle); int (*can_fua) (struct nbdkit_next_ops *next_ops, void *nxdata, diff --git a/filters/blocksize/blocksize.c b/filters/blocksize/blocksize.c index 0978887f..47638c74 100644 --- a/filters/blocksize/blocksize.c +++ b/filters/blocksize/blocksize.c @@ -307,6 +307,18 @@ blocksize_zero (struct nbdkit_next_ops *next_ops, void *nxdata, uint32_t drop; bool need_flush = false; + if (flags & NBDKIT_FLAG_FAST_ZERO) { + /* If we have to split the transaction, an ENOTSUP fast failure in + * a later call would be unnecessarily delayed behind earlier + * calls; it's easier to just declare that anything that can't be + * done in one call to the plugin is not fast. + */ + if ((offs | count) & (minblock - 1) || count > maxlen) { + *err = ENOTSUP; + return -1; + } + } + if ((flags & NBDKIT_FLAG_FUA) && next_ops->can_fua (nxdata) == NBDKIT_FUA_EMULATE) { flags &= ~NBDKIT_FLAG_FUA; diff --git a/filters/cache/cache.c b/filters/cache/cache.c index b5dbccd2..7c1d6c4f 100644 --- a/filters/cache/cache.c +++ b/filters/cache/cache.c @@ -250,6 +250,17 @@ cache_can_cache (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) return NBDKIT_CACHE_NATIVE; } +/* Override the plugin's .can_fast_zero, because our .zero is not fast */ +static int +cache_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + /* It is better to advertise support even when we always reject fast + * zero attempts. + */ + return 1; +} + /* Read data. */ static int cache_pread (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -418,6 +429,14 @@ cache_zero (struct nbdkit_next_ops *next_ops, void *nxdata, int r; bool need_flush = false; + /* We are purposefully avoiding next_ops->zero, so a zero request is + * never faster than plain writes. + */ + if (flags & NBDKIT_FLAG_FAST_ZERO) { + *err = ENOTSUP; + return -1; + } + block = malloc (blksize); if (block == NULL) { *err = errno; @@ -624,6 +643,7 @@ static struct nbdkit_filter filter = { .prepare = cache_prepare, .get_size = cache_get_size, .can_cache = cache_can_cache, + .can_fast_zero = cache_can_fast_zero, .pread = cache_pread, .pwrite = cache_pwrite, .zero = cache_zero, diff --git a/filters/cow/cow.c b/filters/cow/cow.c index 9d91d432..a5c1f978 100644 --- a/filters/cow/cow.c +++ b/filters/cow/cow.c @@ -179,6 +179,17 @@ cow_can_cache (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) return NBDKIT_FUA_NATIVE; } +/* Override the plugin's .can_fast_zero, because our .zero is not fast */ +static int +cow_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + /* It is better to advertise support even when we always reject fast + * zero attempts. + */ + return 1; +} + static int cow_flush (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, uint32_t flags, int *err); /* Read data. */ @@ -340,6 +351,14 @@ cow_zero (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t blknum, blkoffs; int r; + /* We are purposefully avoiding next_ops->zero, so a zero request is + * never faster than plain writes. + */ + if (flags & NBDKIT_FLAG_FAST_ZERO) { + *err = ENOTSUP; + return -1; + } + block = malloc (BLKSIZE); if (block == NULL) { *err = errno; @@ -496,6 +515,7 @@ static struct nbdkit_filter filter = { .can_extents = cow_can_extents, .can_fua = cow_can_fua, .can_cache = cow_can_cache, + .can_fast_zero = cow_can_fast_zero, .pread = cow_pread, .pwrite = cow_pwrite, .zero = cow_zero, diff --git a/filters/delay/delay.c b/filters/delay/delay.c index c92a12d7..207d101e 100644 --- a/filters/delay/delay.c +++ b/filters/delay/delay.c @@ -37,6 +37,8 @@ #include <stdlib.h> #include <stdint.h> #include <string.h> +#include <stdbool.h> +#include <errno.h> #include <nbdkit-filter.h> @@ -46,6 +48,7 @@ static int delay_zero_ms = 0; /* zero delay (milliseconds) */ static int delay_trim_ms = 0; /* trim delay (milliseconds) */ static int delay_extents_ms = 0;/* extents delay (milliseconds) */ static int delay_cache_ms = 0; /* cache delay (milliseconds) */ +static int delay_fast_zero = 1; /* whether delaying zero includes fast zero */ static int parse_delay (const char *key, const char *value) @@ -182,6 +185,12 @@ delay_config (nbdkit_next_config *next, void *nxdata, return -1; return 0; } + else if (strcmp (key, "delay-fast-zero") == 0) { + delay_fast_zero = nbdkit_parse_bool (value); + if (delay_fast_zero < 0) + return -1; + return 0; + } else return next (nxdata, key, value); } @@ -194,7 +203,19 @@ delay_config (nbdkit_next_config *next, void *nxdata, "delay-trim=<NN>[ms] Trim delay in seconds/milliseconds.\n" \ "delay-extents=<NN>[ms] Extents delay in seconds/milliseconds.\n" \ "delay-cache=<NN>[ms] Cache delay in seconds/milliseconds.\n" \ - "wdelay=<NN>[ms] Write, zero and trim delay in secs/msecs." + "wdelay=<NN>[ms] Write, zero and trim delay in secs/msecs.\n" \ + "delay-fast-zero=<BOOL> Delay fast zero requests (default true).\n" + +/* Override the plugin's .can_fast_zero if needed */ +static int +delay_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + /* Advertise if we are handling fast zero requests locally */ + if (delay_zero_ms && !delay_fast_zero) + return 1; + return next_ops->can_fast_zero (nxdata); +} /* Read data. */ static int @@ -225,6 +246,10 @@ delay_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, uint32_t count, uint64_t offset, uint32_t flags, int *err) { + if ((flags & NBDKIT_FLAG_FAST_ZERO) && delay_zero_ms && !delay_fast_zero) { + *err = ENOTSUP; + return -1; + } if (zero_delay (err) == -1) return -1; return next_ops->zero (nxdata, count, offset, flags, err); @@ -269,6 +294,7 @@ static struct nbdkit_filter filter = { .version = PACKAGE_VERSION, .config = delay_config, .config_help = delay_config_help, + .can_fast_zero = delay_can_fast_zero, .pread = delay_pread, .pwrite = delay_pwrite, .zero = delay_zero, diff --git a/filters/log/log.c b/filters/log/log.c index 7cf741e1..95667c61 100644 --- a/filters/log/log.c +++ b/filters/log/log.c @@ -260,14 +260,15 @@ log_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) int F = next_ops->can_fua (nxdata); int e = next_ops->can_extents (nxdata); int c = next_ops->can_cache (nxdata); + int Z = next_ops->can_fast_zero (nxdata); if (size < 0 || w < 0 || f < 0 || r < 0 || t < 0 || z < 0 || F < 0 || - e < 0 || c < 0) + e < 0 || c < 0 || Z < 0) return -1; output (h, "Connect", 0, "size=0x%" PRIx64 " write=%d flush=%d " - "rotational=%d trim=%d zero=%d fua=%d extents=%d cache=%d", - size, w, f, r, t, z, F, e, c); + "rotational=%d trim=%d zero=%d fua=%d extents=%d cache=%d " + "fast_zero=%d", size, w, f, r, t, z, F, e, c, Z); return 0; } @@ -360,10 +361,13 @@ log_zero (struct nbdkit_next_ops *next_ops, void *nxdata, uint64_t id = get_id (h); int r; - assert (!(flags & ~(NBDKIT_FLAG_FUA | NBDKIT_FLAG_MAY_TRIM))); - output (h, "Zero", id, "offset=0x%" PRIx64 " count=0x%x trim=%d fua=%d ...", + assert (!(flags & ~(NBDKIT_FLAG_FUA | NBDKIT_FLAG_MAY_TRIM | + NBDKIT_FLAG_FAST_ZERO))); + output (h, "Zero", id, + "offset=0x%" PRIx64 " count=0x%x trim=%d fua=%d fast=%d...", offs, count, !!(flags & NBDKIT_FLAG_MAY_TRIM), - !!(flags & NBDKIT_FLAG_FUA)); + !!(flags & NBDKIT_FLAG_FUA), + !!(flags & NBDKIT_FLAG_FAST_ZERO)); r = next_ops->zero (nxdata, count, offs, flags, err); output_return (h, "...Zero", id, r, err); return r; diff --git a/filters/nozero/nozero.c b/filters/nozero/nozero.c index 964cce9f..e54f7c62 100644 --- a/filters/nozero/nozero.c +++ b/filters/nozero/nozero.c @@ -38,6 +38,7 @@ #include <string.h> #include <stdbool.h> #include <assert.h> +#include <errno.h> #include <nbdkit-filter.h> @@ -49,8 +50,16 @@ static enum ZeroMode { NONE, EMULATE, NOTRIM, + PLUGIN, } zeromode; +static enum FastZeroMode { + DEFAULT, + SLOW, + IGNORE, + NOFAST, +} fastzeromode; + static int nozero_config (nbdkit_next_config *next, void *nxdata, const char *key, const char *value) @@ -60,17 +69,35 @@ nozero_config (nbdkit_next_config *next, void *nxdata, zeromode = EMULATE; else if (strcmp (value, "notrim") == 0) zeromode = NOTRIM; + else if (strcmp (value, "plugin") == 0) + zeromode = PLUGIN; else if (strcmp (value, "none") != 0) { nbdkit_error ("unknown zeromode '%s'", value); return -1; } return 0; } + + if (strcmp (key, "fastzeromode") == 0) { + if (strcmp (value, "none") == 0) + fastzeromode = NOFAST; + else if (strcmp (value, "ignore") == 0) + fastzeromode = IGNORE; + else if (strcmp (value, "slow") == 0) + fastzeromode = SLOW; + else if (strcmp (value, "default") != 0) { + nbdkit_error ("unknown fastzeromode '%s'", value); + return -1; + } + return 0; + } + return next (nxdata, key, value); } #define nozero_config_help \ - "zeromode=<MODE> Either 'none' (default), 'emulate', or 'notrim'.\n" \ + "zeromode=<MODE> One of 'none' (default), 'emulate', 'notrim', 'plugin'.\n" \ + "fastzeromode=<MODE> One of 'default', 'none', 'slow', 'ignore'.\n" /* Check that desired mode is supported by plugin. */ static int @@ -78,12 +105,13 @@ nozero_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) { int r; - if (zeromode == NOTRIM) { + if (zeromode == NOTRIM || zeromode == PLUGIN) { r = next_ops->can_zero (nxdata); if (r == -1) return -1; if (!r) { - nbdkit_error ("zeromode 'notrim' requires plugin zero support"); + nbdkit_error ("zeromode '%s' requires plugin zero support", + zeromode == NOTRIM ? "notrim" : "plugin"); return -1; } } @@ -94,9 +122,22 @@ nozero_prepare (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) static int nozero_can_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle) { + /* For NOTRIM and PLUGIN modes, we've already verified next_ops->can_zero */ return zeromode != NONE; } +/* Advertise desired FAST_ZERO mode. */ +static int +nozero_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + if (zeromode == NONE) + return 0; + if (zeromode != EMULATE && fastzeromode == DEFAULT) + return next_ops->can_fast_zero (nxdata); + return fastzeromode != NOFAST; +} + static int nozero_zero (struct nbdkit_next_ops *next_ops, void *nxdata, void *handle, uint32_t count, uint64_t offs, uint32_t flags, @@ -106,9 +147,21 @@ nozero_zero (struct nbdkit_next_ops *next_ops, void *nxdata, bool need_flush = false; assert (zeromode != NONE); - flags &= ~NBDKIT_FLAG_MAY_TRIM; + if (flags & NBDKIT_FLAG_FAST_ZERO) { + assert (fastzeromode != NOFAST); + if (fastzeromode == SLOW || + (fastzeromode == DEFAULT && zeromode == EMULATE)) { + *err = ENOTSUP; + return -1; + } + if (fastzeromode == IGNORE) + flags &= ~NBDKIT_FLAG_FAST_ZERO; + } if (zeromode == NOTRIM) + flags &= ~NBDKIT_FLAG_MAY_TRIM; + + if (zeromode != EMULATE) return next_ops->zero (nxdata, count, offs, flags, err); if (flags & NBDKIT_FLAG_FUA) { @@ -144,6 +197,7 @@ static struct nbdkit_filter filter = { .config_help = nozero_config_help, .prepare = nozero_prepare, .can_zero = nozero_can_zero, + .can_fast_zero = nozero_can_fast_zero, .zero = nozero_zero, }; diff --git a/filters/truncate/truncate.c b/filters/truncate/truncate.c index 93d8f074..47d70b31 100644 --- a/filters/truncate/truncate.c +++ b/filters/truncate/truncate.c @@ -201,6 +201,14 @@ truncate_get_size (struct nbdkit_next_ops *next_ops, void *nxdata, return h->size; } +/* Override the plugin's .can_fast_zero, because zeroing a tail is fast. */ +static int +truncate_can_fast_zero (struct nbdkit_next_ops *next_ops, void *nxdata, + void *handle) +{ + return 1; +} + /* Read data. */ static int truncate_pread (struct nbdkit_next_ops *next_ops, void *nxdata, @@ -297,6 +305,12 @@ truncate_zero (struct nbdkit_next_ops *next_ops, void *nxdata, n = count; else n = h->real_size - offset; + if (flags & NBDKIT_FLAG_FAST_ZERO && + next_ops->can_fast_zero (nxdata) <= 0) { + /* TODO: Cache per connection? */ + *err = ENOTSUP; + return -1; + } return next_ops->zero (nxdata, n, offset, flags, err); } return 0; @@ -392,6 +406,7 @@ static struct nbdkit_filter filter = { .close = truncate_close, .prepare = truncate_prepare, .get_size = truncate_get_size, + .can_fast_zero = truncate_can_fast_zero, .pread = truncate_pread, .pwrite = truncate_pwrite, .trim = truncate_trim, diff --git a/tests/test-fua.sh b/tests/test-fua.sh index c0d82db7..1c869e96 100755 --- a/tests/test-fua.sh +++ b/tests/test-fua.sh @@ -106,14 +106,14 @@ test $(grep -c 'connection=1 Flush' fua1.log) -lt \ # all earlier parts of the transaction do not have fua flush1=$(grep -c 'connection=1 Flush' fua2.log || :) flush2=$(grep -c 'connection=2 Flush' fua2.log || :) -fua=$(grep -c 'connection=2.*fua=1 \.' fua2.log || :) +fua=$(grep -c 'connection=2.*fua=1 .*\.' fua2.log || :) test $(( $flush2 - $flush1 + $fua )) = 2 # Test 3: every part of split has fua, and no flushes are added flush1=$(grep -c 'connection=1 Flush' fua3.log || :) flush2=$(grep -c 'connection=2 Flush' fua3.log || :) test $flush1 = $flush2 -test $(grep -c 'connection=2.*fua=1 \.' fua3.log) = 32 +test $(grep -c 'connection=2.*fua=1 .*\.' fua3.log) = 32 # Test 4: flush is no-op, and every transaction has fua if grep 'fua=0' fua4.log; then -- 2.21.0

On 8/23/19 9:40 AM, Eric Blake wrote:

Allow plugins to affect the handling of the new NBD_CMD_FLAG_FAST_ZERO flag, then update affected plugins. In particular, in-memory plugins are always fast; the full plugin is better served by omitting .zero and relying on .pwrite fallback; nbd takes advantage of libnbd extensions proposed in parallel to pass through support; and v2 language bindings expose the choice to their scripts.

+++ b/server/plugins.c @@ -404,11 +404,25 @@ plugin_can_zero (struct backend *b, struct connection *conn) static int plugin_can_fast_zero (struct backend *b, struct connection *conn) { + struct backend_plugin *p = container_of (b, struct backend_plugin, backend); + int r; + assert (connection_get_handle (conn, 0));

debug ("can_fast_zero");

- return 0; /* Upcoming patch will actually add support. */ + if (p->plugin.can_fast_zero) + return p->plugin.can_fast_zero (connection_get_handle (conn, 0)); + /* 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_old. + */ + if (p->plugin.zero == NULL) + return 1; + r = plugin_can_zero (b, conn); + if (r == -1) + return -1; + return !r; }

Needs this squashed in for libnbd to pass rather than skip its new 'can-fast-zero-flag' test: diff --git i/server/plugins.c w/server/plugins.c index 84329cf4..695a77ab 100644 --- i/server/plugins.c +++ w/server/plugins.c @@ -208,6 +208,7 @@ plugin_dump_fields (struct backend *b) HAS (can_cache); HAS (cache); HAS (thread_model); + HAS (can_fast_zero); #undef HAS /* Custom fields. */ -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org