05.07.2018 18:59, Eric Blake wrote:
On 07/04/2018 06:23 AM, Vladimir Sementsov-Ogievskiy wrote:
This is necessary for efficient block-status export, for clients which
support it.
qemu as client doesn't currently process additional information when
querying block status. Should it?
Are there other clients which DO make use of the additional information?
Actually, we have one in development. block status export is used for
external full backups, like bitmap export - for external incremental
backups.
This is a feature, so it is indeed 3.1 material.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
---
nbd/server.c | 77
+++++++++++++++++++++++++++++++++++++++++++++---------------
1 file changed, 58 insertions(+), 19 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index ea5fe0eb33..a6d013aec4 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1844,11 +1844,22 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
-static int blockstatus_to_extent_be(BlockDriverState *bs, uint64_t
offset,
- uint64_t bytes, NBDExtent *extent)
+/*
+ * Populate @extents from block status. Store in @bytes the byte
length encoded
+ * (which may be smaller but (unlike bitmap_to_extents) _never_
larger than the
+ * original), and store in @nb_extents the number of extents used.
I don't think the comparison to bitmap_to_extents in a nested
parenthetical buys us anything; the shorter:
(which may be smaller than the original)
would do just fine for the function contract.
+ *
+ * Returns zero on success and -errno on bdrv_block_status_above
failure.
Is it worth returning a positive value for one less pointer parameter
used for output?
I think, that if we have one such parameter anyway, let's use same path
for very similar thing, for consistency.
+ */
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t
offset,
+ uint64_t *bytes, NBDExtent *extents,
+ unsigned int *nb_extents)
{
- uint64_t remaining_bytes = bytes;
+ uint64_t remaining_bytes = *bytes;
+ NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
So both bytes and nb_extents are in-out.
@@ -1910,21 +1940,29 @@ static int nbd_co_send_extents(NBDClient
*client, uint64_t handle,
/* Get block status from the exported device and send it to the
client */
static int nbd_co_send_block_status(NBDClient *client, uint64_t
handle,
BlockDriverState *bs, uint64_t
offset,
- uint32_t length, bool last,
- uint32_t context_id, Error **errp)
+ uint32_t length, bool
dont_fragment,
+ bool last, uint32_t context_id,
+ Error **errp)
{
int ret;
- NBDExtent extent;
+ unsigned int nb_extents = dont_fragment ? 1 :
NBD_MAX_BITMAP_EXTENTS;
+ NBDExtent *extents = g_new(NBDExtent, nb_extents);
+ uint64_t final_length = length;
- ret = blockstatus_to_extent_be(bs, offset, length, &extent);
+ ret = blockstatus_to_extents(bs, offset, &final_length, extents,
+ &nb_extents);
if (ret < 0) {
+ g_free(extents);
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
}
- return nbd_co_send_extents(client, handle, &extent, 1,
- be32_to_cpu(extent.length), last,
- context_id, errp);
+ ret = nbd_co_send_extents(client, handle, extents, nb_extents,
+ final_length, last, context_id, errp);
+
+ g_free(extents);
+
At any rate, this conversion looks sane.
+ return ret;
}
/*
@@ -2228,10 +2266,11 @@ static coroutine_fn int
nbd_handle_request(NBDClient *client,
(client->export_meta.base_allocation ||
client->export_meta.bitmap))
{
+ bool dont_fragment = request->flags & NBD_CMD_FLAG_REQ_ONE;
if (client->export_meta.base_allocation) {
Blank lines between declarations and statements can aid in legibility;
I can add when queuing.
Reviewed-by: Eric Blake <ebl...@redhat.com>
Ok, thank you!
--
Best regards,
Vladimir
