Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
On 10/18/19 11:07 AM, Vladimir Sementsov-Ogievskiy wrote:
static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
- NBDExtent *extents, unsigned int nb_extents,
- uint64_t length, bool last,
- uint32_t context_id, Error **errp)
+ NBDExtentArray *ea,
+ bool last, uint32_t context_id, Error **errp)
{
NBDStructuredMeta chunk;
-
+ size_t len = ea->count * sizeof(ea->extents[0]);
+ g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
Why do we need memdup here? What's wrong with modifying ea->extents in
place?...
To not make ea to be IN-OUT parameter.. I don't like functions with side
effects.
It will break the code if at some point we call nbd_co_send_extents twice on
same
ea object.
What is the true way? To not memdup, nbd_co_send_extents should consume the
whole
ea object..
Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
1983 |g_autoptr(NBDExtentArray) ea,
|^
so, is it better
to call nbd_co_send_external(... g_steal_pointer() ...)
and than in nbd_co_send_external do
g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;
?
No, that makes it worse. It's that much more confusing to track who is
allocating what and where it gets cleaned up.
I personally don't see the need to avoid jumping through hoops to avoid
an in-out parameter (if we're going to rework code later, we'll notice
that we documented how things are supposed to be used), but if in-out
parameters bother you, then the approach you used, even with an extra
memdup(), is the simplest way to maintain, even if it is not the most
efficient.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
09.10.2019 20:02, Eric Blake wrote:
> On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Introduce NBDExtentArray class, to handle extents list creation in more
>> controlled way and with less OUT parameters in functions.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy
>> ---
>> nbd/server.c | 184 +--
>> 1 file changed, 90 insertions(+), 94 deletions(-)
>>
>
>> +static void nbd_extent_array_free(NBDExtentArray *ea)
>> +{
>> + g_free(ea->extents);
>> + g_free(ea);
>> +}
>> +G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
>
> Nice to see this getting more popular :)
>
>> +
>> +static int nbd_extent_array_add(NBDExtentArray *ea,
>> + uint32_t length, uint32_t flags)
>> {
>
>> - assert(*nb_extents);
>> - while (remaining_bytes) {
>> + if (ea->count >= ea->nb_alloc) {
>> + return -1;
>> + }
>
> Returning -1 is not a failure in the protocol, just failure to add any more
> information to the reply. A function comment might help, but this looks like
> a good helper function.
Something like
/*
* Add extent to NBDExtentArray. If extent can't be added (no available space),
* return -1.
*/
above the function?
>
>> +static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
>> + uint64_t bytes, NBDExtentArray *ea)
>> +{
>> + while (bytes) {
>> uint32_t flags;
>> int64_t num;
>> - int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
>> - , NULL, NULL);
>> + int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
>> + NULL, NULL);
>
>> + if (nbd_extent_array_add(ea, num, flags) < 0) {
>> + return 0;
>> }
>> - offset += num;
>> - remaining_bytes -= num;
>> - }
>> - extents_end = extent + 1;
>> -
>> - for (extent = extents; extent < extents_end; extent++) {
>> - extent->flags = cpu_to_be32(extent->flags);
>> - extent->length = cpu_to_be32(extent->length);
>> + offset += num;
>> + bytes -= num;
>> }
>> - *bytes -= remaining_bytes;
>> - *nb_extents = extents_end - extents;
>> -
>> return 0;
>
> Also looks good (return 0 if we populated until we either ran out of reply
> space or out of bytes to report on).
>
>> static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
>> - NBDExtent *extents, unsigned int nb_extents,
>> - uint64_t length, bool last,
>> - uint32_t context_id, Error **errp)
>> + NBDExtentArray *ea,
>> + bool last, uint32_t context_id, Error **errp)
>> {
>> NBDStructuredMeta chunk;
>> -
>> + size_t len = ea->count * sizeof(ea->extents[0]);
>> + g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
>
> Why do we need memdup here? What's wrong with modifying ea->extents in
> place?...
To not make ea to be IN-OUT parameter.. I don't like functions with side
effects.
It will break the code if at some point we call nbd_co_send_extents twice on
same
ea object.
What is the true way? To not memdup, nbd_co_send_extents should consume the
whole
ea object..
Seems, g_autoptr attribute can't be used for function parameter, gcc complains:
nbd/server.c:1983:32: error: ‘cleanup’ attribute ignored [-Werror=attributes]
1983 |g_autoptr(NBDExtentArray) ea,
|^
so, is it better
to call nbd_co_send_external(... g_steal_pointer() ...)
and than in nbd_co_send_external do
g_autoptr(NBDExtentArray) local_ea = ea;
NBDExtent *extents = local_ea->extents;
?
>
>> + NBDExtent *extent, *extents_end = extents + ea->count;
>> struct iovec iov[] = {
>> {.iov_base = , .iov_len = sizeof(chunk)},
>> - {.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
>> + {.iov_base = extents, .iov_len = len}
>> };
>> - trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
>> + for (extent = extents; extent < extents_end; extent++) {
>> + extent->flags = cpu_to_be32(extent->flags);
>> + extent->length = cpu_to_be32(extent->length);
>> + }
>> +
>> + trace_nbd_co_send_extents(handle, ea->count, context_id,
>> ea->total_length,
>> + last);
>> set_be_chunk(, last ? NBD_REPLY_FLAG_DONE : 0,
>> NBD_REPLY_TYPE_BLOCK_STATUS,
>> handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
>> @@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient
>> *client, uint64_t handle,
>> {
>> int ret;
>> unsigned int nb_extents = dont_fragment ? 1 :
>>
Re: [PATCH 08/10] nbd/server: introduce NBDExtentArray
On 9/30/19 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with less OUT parameters in functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
nbd/server.c | 184 +--
1 file changed, 90 insertions(+), 94 deletions(-)
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+g_free(ea->extents);
+g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
Nice to see this getting more popular :)
+
+static int nbd_extent_array_add(NBDExtentArray *ea,
+uint32_t length, uint32_t flags)
{
-assert(*nb_extents);
-while (remaining_bytes) {
+if (ea->count >= ea->nb_alloc) {
+return -1;
+}
Returning -1 is not a failure in the protocol, just failure to add any
more information to the reply. A function comment might help, but this
looks like a good helper function.
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, NBDExtentArray *ea)
+{
+while (bytes) {
uint32_t flags;
int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
- , NULL, NULL);
+int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
+ NULL, NULL);
+if (nbd_extent_array_add(ea, num, flags) < 0) {
+return 0;
}
-offset += num;
-remaining_bytes -= num;
-}
-extents_end = extent + 1;
-
-for (extent = extents; extent < extents_end; extent++) {
-extent->flags = cpu_to_be32(extent->flags);
-extent->length = cpu_to_be32(extent->length);
+offset += num;
+bytes -= num;
}
-*bytes -= remaining_bytes;
-*nb_extents = extents_end - extents;
-
return 0;
Also looks good (return 0 if we populated until we either ran out of
reply space or out of bytes to report on).
static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
- NBDExtent *extents, unsigned int nb_extents,
- uint64_t length, bool last,
- uint32_t context_id, Error **errp)
+ NBDExtentArray *ea,
+ bool last, uint32_t context_id, Error **errp)
{
NBDStructuredMeta chunk;
-
+size_t len = ea->count * sizeof(ea->extents[0]);
+g_autofree NBDExtent *extents = g_memdup(ea->extents, len);
Why do we need memdup here? What's wrong with modifying ea->extents in
place?...
+NBDExtent *extent, *extents_end = extents + ea->count;
struct iovec iov[] = {
{.iov_base = , .iov_len = sizeof(chunk)},
-{.iov_base = extents, .iov_len = nb_extents * sizeof(extents[0])}
+{.iov_base = extents, .iov_len = len}
};
-trace_nbd_co_send_extents(handle, nb_extents, context_id, length, last);
+for (extent = extents; extent < extents_end; extent++) {
+extent->flags = cpu_to_be32(extent->flags);
+extent->length = cpu_to_be32(extent->length);
+}
+
+trace_nbd_co_send_extents(handle, ea->count, context_id, ea->total_length,
+ last);
set_be_chunk(, last ? NBD_REPLY_FLAG_DONE : 0,
NBD_REPLY_TYPE_BLOCK_STATUS,
handle, sizeof(chunk) - sizeof(chunk.h) + iov[1].iov_len);
@@ -1994,39 +2012,27 @@ static int nbd_co_send_block_status(NBDClient *client,
uint64_t handle,
{
int ret;
unsigned int nb_extents = dont_fragment ? 1 :
NBD_MAX_BLOCK_STATUS_EXTENTS;
-NBDExtent *extents = g_new(NBDExtent, nb_extents);
-uint64_t final_length = length;
+g_autoptr(NBDExtentArray) ea = nbd_extent_array_new(nb_extents);
-ret = blockstatus_to_extents(bs, offset, _length, extents,
- _extents);
+ret = blockstatus_to_extents(bs, offset, length, ea);
if (ret < 0) {
-g_free(extents);
return nbd_co_send_structured_error(
client, handle, -ret, "can't get block status", errp);
}
-ret = nbd_co_send_extents(client, handle, extents, nb_extents,
- final_length, last, context_id, errp);
-
-g_free(extents);
-
-return ret;
+return nbd_co_send_extents(client, handle, ea, last, context_id, errp);
...especially since ea goes out of scope right after the helper function
finishes?
Overall looks like a nice refactoring.
Reviewed-by: Eric Blake
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[PATCH 08/10] nbd/server: introduce NBDExtentArray
Introduce NBDExtentArray class, to handle extents list creation in more
controlled way and with less OUT parameters in functions.
Signed-off-by: Vladimir Sementsov-Ogievskiy
---
nbd/server.c | 184 +--
1 file changed, 90 insertions(+), 94 deletions(-)
diff --git a/nbd/server.c b/nbd/server.c
index cc0069c15b..cc63d8ad21 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1894,27 +1894,63 @@ static int coroutine_fn
nbd_co_send_sparse_read(NBDClient *client,
return ret;
}
-/*
- * Populate @extents from block status. Update @bytes to be the actual
- * length encoded (which may be smaller than the original), and update
- * @nb_extents to the number of extents used.
- *
- * Returns zero on success and -errno on bdrv_block_status_above failure.
- */
-static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
- uint64_t *bytes, NBDExtent *extents,
- unsigned int *nb_extents)
+typedef struct NBDExtentArray {
+NBDExtent *extents;
+unsigned int nb_alloc;
+unsigned int count;
+uint64_t total_length;
+} NBDExtentArray;
+
+static NBDExtentArray *nbd_extent_array_new(unsigned int nb_alloc)
+{
+NBDExtentArray *ea = g_new0(NBDExtentArray, 1);
+
+ea->nb_alloc = nb_alloc;
+ea->extents = g_new(NBDExtent, nb_alloc);
+
+return ea;
+}
+
+static void nbd_extent_array_free(NBDExtentArray *ea)
+{
+g_free(ea->extents);
+g_free(ea);
+}
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(NBDExtentArray, nbd_extent_array_free);
+
+static int nbd_extent_array_add(NBDExtentArray *ea,
+uint32_t length, uint32_t flags)
{
-uint64_t remaining_bytes = *bytes;
-NBDExtent *extent = extents, *extents_end = extents + *nb_extents;
-bool first_extent = true;
+if (!length) {
+return 0;
+}
+
+/* Extend previous extent if flags are the same */
+if (ea->count > 0 && flags == ea->extents[ea->count - 1].flags) {
+ea->extents[ea->count - 1].length += length;
+ea->total_length += length;
+return 0;
+}
-assert(*nb_extents);
-while (remaining_bytes) {
+if (ea->count >= ea->nb_alloc) {
+return -1;
+}
+
+ea->total_length += length;
+ea->extents[ea->count] = (NBDExtent) {.length = length, .flags = flags};
+ea->count++;
+
+return 0;
+}
+
+static int blockstatus_to_extents(BlockDriverState *bs, uint64_t offset,
+ uint64_t bytes, NBDExtentArray *ea)
+{
+while (bytes) {
uint32_t flags;
int64_t num;
-int ret = bdrv_block_status_above(bs, NULL, offset, remaining_bytes,
- , NULL, NULL);
+int ret = bdrv_block_status_above(bs, NULL, offset, bytes, ,
+ NULL, NULL);
if (ret < 0) {
return ret;
@@ -1923,60 +1959,42 @@ static int blockstatus_to_extents(BlockDriverState *bs,
uint64_t offset,
flags = (ret & BDRV_BLOCK_ALLOCATED ? 0 : NBD_STATE_HOLE) |
(ret & BDRV_BLOCK_ZERO ? NBD_STATE_ZERO : 0);
-if (first_extent) {
-extent->flags = flags;
-extent->length = num;
-first_extent = false;
-} else if (flags == extent->flags) {
-/* extend current extent */
-extent->length += num;
-} else {
-if (extent + 1 == extents_end) {
-break;
-}
-
-/* start new extent */
-extent++;
-extent->flags = flags;
-extent->length = num;
+if (nbd_extent_array_add(ea, num, flags) < 0) {
+return 0;
}
-offset += num;
-remaining_bytes -= num;
-}
-extents_end = extent + 1;
-
-for (extent = extents; extent < extents_end; extent++) {
-extent->flags = cpu_to_be32(extent->flags);
-extent->length = cpu_to_be32(extent->length);
+offset += num;
+bytes -= num;
}
-*bytes -= remaining_bytes;
-*nb_extents = extents_end - extents;
-
return 0;
}
-/* nbd_co_send_extents
+/*
+ * nbd_co_send_extents
*
- * @length is only for tracing purposes (and may be smaller or larger
- * than the client's original request). @last controls whether
- * NBD_REPLY_FLAG_DONE is sent. @extents should already be in
- * big-endian format.
+ * @last controls whether NBD_REPLY_FLAG_DONE is sent.
*/
static int nbd_co_send_extents(NBDClient *client, uint64_t handle,
- NBDExtent *extents, unsigned int nb_extents,
- uint64_t length, bool last,
- uint32_t context_id, Error **errp)
+ NBDExtentArray *ea,
+ bool last, uint32_t context_id, Error **errp)
{
NBDStructuredMeta chunk;
-
+size_t len = ea->count *
