* [Qemu-devel] [PULL v2 1/9] nbd: Advertise multi-conn for shared read-only connections
2019-09-05 21:15 [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Eric Blake
@ 2019-09-05 21:15 ` Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 3/9] nbd: Tolerate more errors to structured reply request Eric Blake
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-09-05 21:15 UTC (permalink / raw)
To: qemu-devel; +Cc: Kevin Wolf, John Snow, open list:Block layer core, Max Reitz
The NBD specification defines NBD_FLAG_CAN_MULTI_CONN, which can be
advertised when the server promises cache consistency between
simultaneous clients (basically, rules that determine what FUA and
flush from one client are able to guarantee for reads from another
client). When we don't permit simultaneous clients (such as qemu-nbd
without -e), the bit makes no sense; and for writable images, we
probably have a lot more work before we can declare that actions from
one client are cache-consistent with actions from another. But for
read-only images, where flush isn't changing any data, we might as
well advertise multi-conn support. What's more, advertisement of the
bit makes it easier for clients to determine if 'qemu-nbd -e' was in
use, where a second connection will succeed rather than hang until the
first client goes away.
This patch affects qemu as server in advertising the bit. We may want
to consider patches to qemu as client to attempt parallel connections
for higher throughput by spreading the load over those connections
when a server advertises multi-conn, but for now sticking to one
connection per nbd:// BDS is okay.
See also: https://bugzilla.redhat.com/1708300
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190815185024.7010-1-eblake@redhat.com>
[eblake: tweak blockdev-nbd.c to not request shared when writable,
fix iotest 233]
Reviewed-by: John Snow <jsnow@redhat.com>
---
docs/interop/nbd.txt | 1 +
include/block/nbd.h | 2 +-
blockdev-nbd.c | 2 +-
nbd/server.c | 4 +++-
qemu-nbd.c | 2 +-
tests/qemu-iotests/223.out | 2 +-
6 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/docs/interop/nbd.txt b/docs/interop/nbd.txt
index fc64473e02b2..6dfec7f47647 100644
--- a/docs/interop/nbd.txt
+++ b/docs/interop/nbd.txt
@@ -53,3 +53,4 @@ 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
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 7b36d672f046..991fd52a5134 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,
+ const char *bitmap, uint16_t nbdflags, 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 c621686131fd..1fcfdb0997c6 100644
--- a/blockdev-nbd.c
+++ b/blockdev-nbd.c
@@ -188,7 +188,7 @@ void qmp_nbd_server_add(const char *device, bool has_name, const char *name,
}
exp = nbd_export_new(bs, 0, len, name, NULL, bitmap,
- writable ? 0 : NBD_FLAG_READ_ONLY,
+ writable ? 0 : NBD_FLAG_READ_ONLY, !writable,
NULL, false, on_eject_blk, errp);
if (!exp) {
return;
diff --git a/nbd/server.c b/nbd/server.c
index f55ccf8edfde..0fb41c6c50ea 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1461,7 +1461,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
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,
+ const char *bitmap, uint16_t nbdflags, bool shared,
void (*close)(NBDExport *), bool writethrough,
BlockBackend *on_eject_blk, Error **errp)
{
@@ -1487,6 +1487,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
perm = BLK_PERM_CONSISTENT_READ;
if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
perm |= BLK_PERM_WRITE;
+ } else if (shared) {
+ nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
}
blk = blk_new(bdrv_get_aio_context(bs), perm,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 83b6c32d73aa..2403ef3d0f9f 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -1173,7 +1173,7 @@ int main(int argc, char **argv)
}
export = nbd_export_new(bs, dev_offset, fd_size, export_name,
- export_description, bitmap, nbdflags,
+ export_description, bitmap, nbdflags, shared > 1,
nbd_export_closed, writethrough, NULL,
&error_fatal);
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index d5201b2356a3..2bca28ae72f9 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -40,7 +40,7 @@ exports available: 0
exports available: 2
export: 'n'
size: 4194304
- flags: 0x4ef ( readonly flush fua trim zeroes df cache )
+ flags: 0x5ef ( readonly flush fua trim zeroes df multi cache )
min block: 1
opt block: 4096
max block: 33554432
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PULL v2 3/9] nbd: Tolerate more errors to structured reply request
2019-09-05 21:15 [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 1/9] nbd: Advertise multi-conn for shared read-only connections Eric Blake
@ 2019-09-05 21:15 ` Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 6/9] nbd: Improve per-export flag handling in server Eric Blake
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-09-05 21:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Daniel P . Berrangé,
open list:Network Block Dev..., Max Reitz
A server may have a reason to reject a request for structured replies,
beyond just not recognizing them as a valid request; similarly, it may
have a reason for rejecting a request for a meta context. It doesn't
hurt us to continue talking to such a server; otherwise 'qemu-nbd
--list' of such a server fails to display all available details about
the export.
Encountered when temporarily tweaking nbdkit to reply with
NBD_REP_ERR_POLICY. Present since structured reply support was first
added (commit d795299b reused starttls handling, but starttls is
different in that we can't fall back to other behavior on any error).
Note that for an unencrypted client trying to connect to a server that
requires encryption, this defers the point of failure to when we
finally execute a strict command (such as NBD_OPT_GO or NBD_OPT_LIST),
now that the intermediate NBD_OPT_STRUCTURED_REPLY does not diagnose
NBD_REP_ERR_TLS_REQD as fatal; but as the protocol eventually gets us
to a command where we can't continue onwards, the changed error
message doesn't cause any security concerns.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190824172813.29720-3-eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
[eblake: fix iotest 233]
---
nbd/client.c | 63 ++++++++++++++++++++------------------
nbd/trace-events | 2 +-
tests/qemu-iotests/233.out | 8 ++---
3 files changed, 38 insertions(+), 35 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index a9d8d32feff7..b9dc829175f9 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -1,5 +1,5 @@
/*
- * Copyright (C) 2016-2018 Red Hat, Inc.
+ * Copyright (C) 2016-2019 Red Hat, Inc.
* Copyright (C) 2005 Anthony Liguori <anthony@codemonkey.ws>
*
* Network Block Device Client Side
@@ -142,17 +142,18 @@ static int nbd_receive_option_reply(QIOChannel *ioc, uint32_t opt,
return 0;
}
-/* If reply represents success, return 1 without further action.
- * If reply represents an error, consume the optional payload of
- * the packet on ioc. Then return 0 for unsupported (so the client
- * can fall back to other approaches), or -1 with errp set for other
- * errors.
+/*
+ * If reply represents success, return 1 without further action. If
+ * reply represents an error, consume the optional payload of the
+ * packet on ioc. Then return 0 for unsupported (so the client can
+ * fall back to other approaches), where @strict determines if only
+ * ERR_UNSUP or all errors fit that category, or -1 with errp set for
+ * other errors.
*/
static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
- Error **errp)
+ bool strict, Error **errp)
{
- char *msg = NULL;
- int result = -1;
+ g_autofree char *msg = NULL;
if (!(reply->type & (1 << 31))) {
return 1;
@@ -163,26 +164,28 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
error_setg(errp, "server error %" PRIu32
" (%s) message is too long",
reply->type, nbd_rep_lookup(reply->type));
- goto cleanup;
+ goto err;
}
msg = g_malloc(reply->length + 1);
if (nbd_read(ioc, msg, reply->length, NULL, errp) < 0) {
error_prepend(errp, "Failed to read option error %" PRIu32
" (%s) message: ",
reply->type, nbd_rep_lookup(reply->type));
- goto cleanup;
+ goto err;
}
msg[reply->length] = '\0';
trace_nbd_server_error_msg(reply->type,
nbd_reply_type_lookup(reply->type), msg);
}
+ if (reply->type == NBD_REP_ERR_UNSUP || !strict) {
+ trace_nbd_reply_err_ignored(reply->option,
+ nbd_opt_lookup(reply->option),
+ reply->type, nbd_rep_lookup(reply->type));
+ return 0;
+ }
+
switch (reply->type) {
- case NBD_REP_ERR_UNSUP:
- trace_nbd_reply_err_unsup(reply->option, nbd_opt_lookup(reply->option));
- result = 0;
- goto cleanup;
-
case NBD_REP_ERR_POLICY:
error_setg(errp, "Denied by server for option %" PRIu32 " (%s)",
reply->option, nbd_opt_lookup(reply->option));
@@ -227,12 +230,9 @@ static int nbd_handle_reply_err(QIOChannel *ioc, NBDOptionReply *reply,
error_append_hint(errp, "server reported: %s\n", msg);
}
- cleanup:
- g_free(msg);
- if (result < 0) {
- nbd_send_opt_abort(ioc);
- }
- return result;
+ err:
+ nbd_send_opt_abort(ioc);
+ return -1;
}
/* nbd_receive_list:
@@ -257,7 +257,7 @@ static int nbd_receive_list(QIOChannel *ioc, char **name, char **description,
if (nbd_receive_option_reply(ioc, NBD_OPT_LIST, &reply, errp) < 0) {
return -1;
}
- error = nbd_handle_reply_err(ioc, &reply, errp);
+ error = nbd_handle_reply_err(ioc, &reply, true, errp);
if (error <= 0) {
return error;
}
@@ -363,7 +363,7 @@ static int nbd_opt_info_or_go(QIOChannel *ioc, uint32_t opt,
if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
return -1;
}
- error = nbd_handle_reply_err(ioc, &reply, errp);
+ error = nbd_handle_reply_err(ioc, &reply, true, errp);
if (error <= 0) {
return error;
}
@@ -538,12 +538,15 @@ static int nbd_receive_query_exports(QIOChannel *ioc,
}
}
-/* nbd_request_simple_option: Send an option request, and parse the reply
+/*
+ * nbd_request_simple_option: Send an option request, and parse the reply.
+ * @strict controls whether ERR_UNSUP or all errors produce 0 status.
* return 1 for successful negotiation,
* 0 if operation is unsupported,
* -1 with errp set for any other error
*/
-static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
+static int nbd_request_simple_option(QIOChannel *ioc, int opt, bool strict,
+ Error **errp)
{
NBDOptionReply reply;
int error;
@@ -555,7 +558,7 @@ static int nbd_request_simple_option(QIOChannel *ioc, int opt, Error **errp)
if (nbd_receive_option_reply(ioc, opt, &reply, errp) < 0) {
return -1;
}
- error = nbd_handle_reply_err(ioc, &reply, errp);
+ error = nbd_handle_reply_err(ioc, &reply, strict, errp);
if (error <= 0) {
return error;
}
@@ -587,7 +590,7 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
QIOChannelTLS *tioc;
struct NBDTLSHandshakeData data = { 0 };
- ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, errp);
+ ret = nbd_request_simple_option(ioc, NBD_OPT_STARTTLS, true, errp);
if (ret <= 0) {
if (ret == 0) {
error_setg(errp, "Server don't support STARTTLS option");
@@ -687,7 +690,7 @@ static int nbd_receive_one_meta_context(QIOChannel *ioc,
return -1;
}
- ret = nbd_handle_reply_err(ioc, &reply, errp);
+ ret = nbd_handle_reply_err(ioc, &reply, false, errp);
if (ret <= 0) {
return ret;
}
@@ -943,7 +946,7 @@ static int nbd_start_negotiate(AioContext *aio_context, QIOChannel *ioc,
if (structured_reply) {
result = nbd_request_simple_option(ioc,
NBD_OPT_STRUCTURED_REPLY,
- errp);
+ false, errp);
if (result < 0) {
return -EINVAL;
}
diff --git a/nbd/trace-events b/nbd/trace-events
index 7ab6b3788cb2..f6cde967903a 100644
--- a/nbd/trace-events
+++ b/nbd/trace-events
@@ -4,7 +4,7 @@
nbd_send_option_request(uint32_t opt, const char *name, uint32_t len) "Sending option request %" PRIu32" (%s), len %" PRIu32
nbd_receive_option_reply(uint32_t option, const char *optname, uint32_t type, const char *typename, uint32_t length) "Received option reply %" PRIu32" (%s), type %" PRIu32" (%s), len %" PRIu32
nbd_server_error_msg(uint32_t err, const char *type, const char *msg) "server reported error 0x%" PRIx32 " (%s) with additional message: %s"
-nbd_reply_err_unsup(uint32_t option, const char *name) "server doesn't understand request %" PRIu32 " (%s), attempting fallback"
+nbd_reply_err_ignored(uint32_t option, const char *name, uint32_t reply, const char *reply_name) "server failed request %" PRIu32 " (%s) with error 0x%" PRIx32 " (%s), attempting fallback"
nbd_receive_list(const char *name, const char *desc) "export list includes '%s', description '%s'"
nbd_opt_info_go_start(const char *opt, const char *name) "Attempting %s for export '%s'"
nbd_opt_info_go_success(const char *opt) "Export is ready after %s request"
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index 9b46284ab0de..a3ecc4eb5ccf 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -20,10 +20,10 @@ qemu-nbd: Denied by server for option 5 (starttls)
server reported: TLS not configured
== check plain client to TLS server fails ==
-qemu-img: Could not open 'nbd://localhost:PORT': TLS negotiation required before option 8 (structured reply)
-server reported: Option 0x8 not permitted before TLS
-qemu-nbd: TLS negotiation required before option 8 (structured reply)
-server reported: Option 0x8 not permitted before TLS
+qemu-img: Could not open 'nbd://localhost:PORT': TLS negotiation required before option 7 (go)
+server reported: Option 0x7 not permitted before TLS
+qemu-nbd: TLS negotiation required before option 3 (list)
+server reported: Option 0x3 not permitted before TLS
== check TLS works ==
image: nbd://127.0.0.1:PORT
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PULL v2 6/9] nbd: Improve per-export flag handling in server
2019-09-05 21:15 [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 1/9] nbd: Advertise multi-conn for shared read-only connections Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 3/9] nbd: Tolerate more errors to structured reply request Eric Blake
@ 2019-09-05 21:15 ` Eric Blake
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 9/9] nbd: Implement server use of NBD FAST_ZERO Eric Blake
2019-09-09 8:48 ` [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Peter Maydell
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-09-05 21:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy,
open list:Block layer core, Max Reitz
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 with NBD_FLAG_SEND_DF 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 which gets shared between multiple clients, 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 for NBD_FLAG_SEND_DF 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>
Message-Id: <20190823143726.27062-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: improve commit message, update iotest 223]
---
include/block/nbd.h | 2 +-
blockdev-nbd.c | 3 +-
nbd/server.c | 62 +++++++++++++++++++++-----------------
qemu-nbd.c | 6 ++--
tests/qemu-iotests/223.out | 2 +-
5 files changed, 40 insertions(+), 35 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 1fcfdb0997c6..213f226ac1c4 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 74d205812fee..d5078f7468af 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -419,14 +419,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)
@@ -454,10 +454,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;
+ }
+ 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) {
@@ -522,8 +525,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];
@@ -536,6 +538,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
uint32_t sizes[3];
char buf[sizeof(uint64_t) + sizeof(uint16_t)];
uint32_t check_align = 0;
+ uint16_t myflags;
/* Client sends:
4 bytes: L, name length (can be 0)
@@ -633,10 +636,13 @@ static int nbd_negotiate_handle_info(NBDClient *client, uint16_t myflags,
}
/* Send NBD_INFO_EXPORT always */
- trace_nbd_negotiate_new_style_size_flags(exp->size,
- exp->nbdflags | myflags);
+ myflags = exp->nbdflags;
+ if (client->structured_reply) {
+ myflags |= NBD_FLAG_SEND_DF;
+ }
+ trace_nbd_negotiate_new_style_size_flags(exp->size, myflags);
stq_be_p(buf, exp->size);
- stw_be_p(buf + 8, exp->nbdflags | myflags);
+ stw_be_p(buf + 8, myflags);
rc = nbd_negotiate_send_info(client, NBD_INFO_EXPORT,
sizeof(buf), buf, errp);
if (rc < 0) {
@@ -1033,8 +1039,7 @@ static int nbd_negotiate_meta_queries(NBDClient *client,
* 1 if client sent NBD_OPT_ABORT, i.e. on valid disconnect,
* errp is not set
*/
-static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
- Error **errp)
+static int nbd_negotiate_options(NBDClient *client, Error **errp)
{
uint32_t flags;
bool fixedNewstyle = false;
@@ -1168,13 +1173,12 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
return 1;
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client,
- myflags, no_zeroes,
+ return nbd_negotiate_handle_export_name(client, no_zeroes,
errp);
case NBD_OPT_INFO:
case NBD_OPT_GO:
- ret = nbd_negotiate_handle_info(client, myflags, errp);
+ ret = nbd_negotiate_handle_info(client, errp);
if (ret == 1) {
assert(option == NBD_OPT_GO);
return 0;
@@ -1205,7 +1209,6 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
} else {
ret = nbd_negotiate_send_rep(client, NBD_REP_ACK, errp);
client->structured_reply = true;
- myflags |= NBD_FLAG_SEND_DF;
}
break;
@@ -1228,8 +1231,7 @@ static int nbd_negotiate_options(NBDClient *client, uint16_t myflags,
*/
switch (option) {
case NBD_OPT_EXPORT_NAME:
- return nbd_negotiate_handle_export_name(client,
- myflags, no_zeroes,
+ return nbd_negotiate_handle_export_name(client, no_zeroes,
errp);
default:
@@ -1255,9 +1257,6 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
{
char buf[NBD_OLDSTYLE_NEGOTIATE_SIZE] = "";
int ret;
- const uint16_t myflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_TRIM |
- NBD_FLAG_SEND_FLUSH | NBD_FLAG_SEND_FUA |
- NBD_FLAG_SEND_WRITE_ZEROES | NBD_FLAG_SEND_CACHE);
/* Old style negotiation header, no room for options
[ 0 .. 7] passwd ("NBDMAGIC")
@@ -1285,7 +1284,7 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp)
error_prepend(errp, "write failed: ");
return -EINVAL;
}
- ret = nbd_negotiate_options(client, myflags, errp);
+ ret = nbd_negotiate_options(client, errp);
if (ret != 0) {
if (ret < 0) {
error_prepend(errp, "option negotiation failed: ");
@@ -1457,7 +1456,7 @@ static void nbd_eject_notifier(Notifier *n, void *data)
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)
{
@@ -1481,10 +1480,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
/* Don't allow resize while the NBD server is running, otherwise we don't
* care what happens with the node. */
perm = BLK_PERM_CONSISTENT_READ;
- if ((nbdflags & NBD_FLAG_READ_ONLY) == 0) {
+ if (!readonly) {
perm |= BLK_PERM_WRITE;
- } else if (shared) {
- nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
}
blk = blk_new(bdrv_get_aio_context(bs), perm,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
@@ -1503,7 +1500,16 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
exp->dev_offset = dev_offset;
exp->name = g_strdup(name);
exp->description = g_strdup(desc);
- exp->nbdflags = nbdflags;
+ exp->nbdflags = (NBD_FLAG_HAS_FLAGS | NBD_FLAG_SEND_FLUSH |
+ NBD_FLAG_SEND_FUA | NBD_FLAG_SEND_CACHE);
+ if (readonly) {
+ exp->nbdflags |= NBD_FLAG_READ_ONLY;
+ if (shared) {
+ exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
+ }
+ } else {
+ exp->nbdflags |= NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES;
+ }
assert(size <= INT64_MAX - dev_offset);
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
@@ -1528,7 +1534,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
goto fail;
}
- if ((nbdflags & NBD_FLAG_READ_ONLY) && bdrv_is_writable(bs) &&
+ if (readonly && bdrv_is_writable(bs) &&
bdrv_dirty_bitmap_enabled(bm)) {
error_setg(errp,
"Enabled bitmap '%s' incompatible with readonly export",
diff --git a/qemu-nbd.c b/qemu-nbd.c
index 2403ef3d0f9f..ae841150760e 100644
--- a/qemu-nbd.c
+++ b/qemu-nbd.c
@@ -600,7 +600,7 @@ int main(int argc, char **argv)
BlockBackend *blk;
BlockDriverState *bs;
uint64_t dev_offset = 0;
- uint16_t nbdflags = 0;
+ bool readonly = false;
bool disconnect = false;
const char *bindto = NULL;
const char *port = NULL;
@@ -782,7 +782,7 @@ int main(int argc, char **argv)
}
/* fall through */
case 'r':
- nbdflags |= NBD_FLAG_READ_ONLY;
+ readonly = true;
flags &= ~BDRV_O_RDWR;
break;
case 'P':
@@ -1173,7 +1173,7 @@ int main(int argc, char **argv)
}
export = nbd_export_new(bs, dev_offset, fd_size, export_name,
- export_description, bitmap, nbdflags, shared > 1,
+ export_description, bitmap, readonly, shared > 1,
nbd_export_closed, writethrough, NULL,
&error_fatal);
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 2bca28ae72f9..2db0dc991a27 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -40,7 +40,7 @@ exports available: 0
exports available: 2
export: 'n'
size: 4194304
- flags: 0x5ef ( readonly flush fua trim zeroes df multi cache )
+ flags: 0x58f ( readonly flush fua df multi cache )
min block: 1
opt block: 4096
max block: 33554432
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [Qemu-devel] [PULL v2 9/9] nbd: Implement server use of NBD FAST_ZERO
2019-09-05 21:15 [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Eric Blake
` (2 preceding siblings ...)
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 6/9] nbd: Improve per-export flag handling in server Eric Blake
@ 2019-09-05 21:15 ` Eric Blake
2019-09-09 8:48 ` [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Peter Maydell
4 siblings, 0 replies; 6+ messages in thread
From: Eric Blake @ 2019-09-05 21:15 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, Vladimir Sementsov-Ogievskiy,
open list:Network Block Dev..., Max Reitz
The server side is fairly straightforward: we can always advertise
support for detection of fast zero, and implement it by mapping the
request to the block layer BDRV_REQ_NO_FALLBACK.
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20190823143726.27062-5-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
[eblake: update iotests 223, 233]
---
nbd/server.c | 8 ++++++--
tests/qemu-iotests/223.out | 2 +-
tests/qemu-iotests/233.out | 2 +-
3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index 4992148de1c4..28c3c8be854c 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1513,7 +1513,8 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset,
exp->nbdflags |= NBD_FLAG_CAN_MULTI_CONN;
}
} else {
- exp->nbdflags |= NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES;
+ exp->nbdflags |= (NBD_FLAG_SEND_TRIM | NBD_FLAG_SEND_WRITE_ZEROES |
+ NBD_FLAG_SEND_FAST_ZERO);
}
assert(size <= INT64_MAX - dev_offset);
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
@@ -2166,7 +2167,7 @@ static int nbd_co_receive_request(NBDRequestData *req, NBDRequest *request,
if (request->type == NBD_CMD_READ && client->structured_reply) {
valid_flags |= NBD_CMD_FLAG_DF;
} else if (request->type == NBD_CMD_WRITE_ZEROES) {
- valid_flags |= NBD_CMD_FLAG_NO_HOLE;
+ valid_flags |= NBD_CMD_FLAG_NO_HOLE | NBD_CMD_FLAG_FAST_ZERO;
} else if (request->type == NBD_CMD_BLOCK_STATUS) {
valid_flags |= NBD_CMD_FLAG_REQ_ONE;
}
@@ -2305,6 +2306,9 @@ static coroutine_fn int nbd_handle_request(NBDClient *client,
if (!(request->flags & NBD_CMD_FLAG_NO_HOLE)) {
flags |= BDRV_REQ_MAY_UNMAP;
}
+ if (request->flags & NBD_CMD_FLAG_FAST_ZERO) {
+ flags |= BDRV_REQ_NO_FALLBACK;
+ }
ret = blk_pwrite_zeroes(exp->blk, request->from + exp->dev_offset,
request->len, flags);
return nbd_send_generic_reply(client, request->handle, ret,
diff --git a/tests/qemu-iotests/223.out b/tests/qemu-iotests/223.out
index 2db0dc991a27..5d00398c11cb 100644
--- a/tests/qemu-iotests/223.out
+++ b/tests/qemu-iotests/223.out
@@ -49,7 +49,7 @@ exports available: 2
qemu:dirty-bitmap:b
export: 'n2'
size: 4194304
- flags: 0x4ed ( flush fua trim zeroes df cache )
+ flags: 0xced ( flush fua trim zeroes df cache fast-zero )
min block: 1
opt block: 4096
max block: 33554432
diff --git a/tests/qemu-iotests/233.out b/tests/qemu-iotests/233.out
index a3ecc4eb5ccf..24321efa113b 100644
--- a/tests/qemu-iotests/233.out
+++ b/tests/qemu-iotests/233.out
@@ -37,7 +37,7 @@ disk size: unavailable
exports available: 1
export: ''
size: 67108864
- flags: 0x4ed ( flush fua trim zeroes df cache )
+ flags: 0xced ( flush fua trim zeroes df cache fast-zero )
min block: 1
opt block: 4096
max block: 33554432
--
2.21.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05
2019-09-05 21:15 [Qemu-devel] [PULL v2 0/9] NBD patches through 2019-09-05 Eric Blake
` (3 preceding siblings ...)
2019-09-05 21:15 ` [Qemu-devel] [PULL v2 9/9] nbd: Implement server use of NBD FAST_ZERO Eric Blake
@ 2019-09-09 8:48 ` Peter Maydell
4 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2019-09-09 8:48 UTC (permalink / raw)
To: Eric Blake; +Cc: QEMU Developers
On Thu, 5 Sep 2019 at 22:17, Eric Blake <eblake@redhat.com> wrote:
>
> The following changes since commit eac2f39602e0423adf56be410c9a22c31fec9a81:
>
> target/arm: Inline gen_bx_im into callers (2019-09-05 13:23:04 +0100)
>
> are available in the Git repository at:
>
> https://repo.or.cz/qemu/ericb.git tags/pull-nbd-2019-09-05-v2
>
> for you to fetch changes up to b491dbb7f8e09ef864770c205a3b5bce6c5c1881:
>
> nbd: Implement server use of NBD FAST_ZERO (2019-09-05 16:04:53 -0500)
>
> v2 pull request: fix iotests 223 and 233 (only re-sending the patches
> that needed tweaking, per this backport-diff):
> 001/9:[0002] [FC] 'nbd: Advertise multi-conn for shared read-only connections'
> 002/9:[----] [--] 'nbd: Use g_autofree in a few places'
> 003/9:[0008] [FC] 'nbd: Tolerate more errors to structured reply request'
> 004/9:[----] [--] 'block: workaround for unaligned byte range in fallocate()'
> 005/9:[----] [--] 'docs: Update preferred NBD device syntax'
> 006/9:[0002] [FC] 'nbd: Improve per-export flag handling in server'
> 007/9:[----] [--] 'nbd: Prepare for NBD_CMD_FLAG_FAST_ZERO'
> 008/9:[----] [--] 'nbd: Implement client use of NBD FAST_ZERO'
> 009/9:[0004] [FC] 'nbd: Implement server use of NBD FAST_ZERO'
>
> ----------------------------------------------------------------
> nbd patches for 2019-09-05
>
> - Advertise NBD_FLAG_CAN_MULTI_CONN on readonly images
> - Tolerate larger set of server error responses during handshake
> - More precision on handling fallocate() failures due to alignment
> - Better documentation of NBD connection URIs
> - Implement new extension NBD_CMD_FLAG_FAST_ZERO to benefit qemu-img convert
>
> ----------------------------------------------------------------
> Andrey Shinkevich (1):
> block: workaround for unaligned byte range in fallocate()
>
> Eric Blake (8):
> nbd: Advertise multi-conn for shared read-only connections
> nbd: Use g_autofree in a few places
> nbd: Tolerate more errors to structured reply request
> docs: Update preferred NBD device syntax
> nbd: Improve per-export flag handling in server
> nbd: Prepare for NBD_CMD_FLAG_FAST_ZERO
> nbd: Implement client use of NBD FAST_ZERO
> nbd: Implement server use of NBD FAST_ZERO
>
Applied, thanks.
Please update the changelog at https://wiki.qemu.org/ChangeLog/4.2
for any user-visible changes.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread