[Qemu-devel] [PATCH 1/1] nbd: fix block-mirror NBD target

[Qemu-devel] [PATCH 1/1] nbd: fix block-mirror NBD target

[Denis V. Lunev]

There is VM with 8 GB QCOW2 storage. Real size of the image file is 2 GB.
'drive-mirror' redirected to NBD target creates 8 GB image at destination.
The situation is even worse as zeroes are sent through the channel.

The patch simply adds .bdrv_co_write_zeroes callback to NBD block driver
which works though NBD_TRIM to avoid transfer of zeroes.

Signed-off-by: Denis V. Lunev <den@openvz.org>
CC: Paolo Bonzini <pbonzini@redhat.com>
---
 block/nbd-client.c | 10 ++++++++++
 block/nbd-client.h |  2 ++
 block/nbd.c        |  9 +++++++++
 3 files changed, 21 insertions(+)

diff --git a/block/nbd-client.c b/block/nbd-client.c
index 568c56c..ef0c5e8 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -344,7 +344,17 @@ int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
     }
     nbd_coroutine_end(client, &request);
     return -reply.error;
+}
+
+int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                          int nb_sectors, BdrvRequestFlags flags)
+{
+    NbdClientSession *client = nbd_get_client_session(bs);
 
+    if (!(client->nbdflags & NBD_FLAG_SEND_TRIM)) {
+        return -ENOTSUP;
+    }
+    return nbd_client_co_discard(bs, sector_num, nb_sectors);
 }
 
 void nbd_client_detach_aio_context(BlockDriverState *bs)
diff --git a/block/nbd-client.h b/block/nbd-client.h
index e841340..aaef18d 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -40,6 +40,8 @@ void nbd_client_close(BlockDriverState *bs);
 
 int nbd_client_co_discard(BlockDriverState *bs, int64_t sector_num,
                           int nb_sectors);
+int nbd_client_co_write_zeroes(BlockDriverState *bs, int64_t sector_num,
+                          int nb_sectors, BdrvRequestFlags flags);
 int nbd_client_co_flush(BlockDriverState *bs);
 int nbd_client_co_writev(BlockDriverState *bs, int64_t sector_num,
                          int nb_sectors, QEMUIOVector *qiov);
diff --git a/block/nbd.c b/block/nbd.c
index 1a90bc7..8b05480 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -318,6 +318,13 @@ static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
     return nbd_client_co_discard(bs, sector_num, nb_sectors);
 }
 
+static coroutine_fn int nbd_co_write_zeroes(BlockDriverState *bs,
+        int64_t sector_num, int nb_sectors, BdrvRequestFlags flags)
+{
+    return nbd_client_co_write_zeroes(bs, sector_num, nb_sectors, flags);
+}
+
+
 static void nbd_close(BlockDriverState *bs)
 {
     nbd_client_close(bs);
@@ -415,6 +422,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_co_write_zeroes       = nbd_co_write_zeroes,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
@@ -433,6 +441,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_close                 = nbd_close,
     .bdrv_co_flush_to_os        = nbd_co_flush,
     .bdrv_co_discard            = nbd_co_discard,
+    .bdrv_co_write_zeroes       = nbd_co_write_zeroes,
     .bdrv_refresh_limits        = nbd_refresh_limits,
     .bdrv_getlength             = nbd_getlength,
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
-- 
2.5.0

Re: [Qemu-devel] [PATCH 1/1] nbd: fix block-mirror NBD target

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 1217 bytes --]

On 17.02.2016 16:08, Denis V. Lunev wrote:
> There is VM with 8 GB QCOW2 storage. Real size of the image file is 2 GB.
> 'drive-mirror' redirected to NBD target creates 8 GB image at destination.
> The situation is even worse as zeroes are sent through the channel.
> 
> The patch simply adds .bdrv_co_write_zeroes callback to NBD block driver
> which works though NBD_TRIM to avoid transfer of zeroes.

The specification[1] says the following about TRIM:

> After issuing this command, a client MUST NOT make any assumptions
> about the contents of the export affected by this command, until
> overwriting it again with NBD_CMD_WRITE.

So I don't think this is correct.

The correct solution would probably to introduce a specific command to
write zeroes, or at least to use detect-zeroes on the NBD server side
(but this will still lead to the zeroes being sent over the line).

Max

[1] https://github.com/yoe/nbd/blob/master/doc/proto.md

> Signed-off-by: Denis V. Lunev <den@openvz.org>
> CC: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  block/nbd-client.c | 10 ++++++++++
>  block/nbd-client.h |  2 ++
>  block/nbd.c        |  9 +++++++++
>  3 files changed, 21 insertions(+)



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 1/1] nbd: fix block-mirror NBD target

[Denis V. Lunev]

On 02/17/2016 06:21 PM, Max Reitz wrote:
> On 17.02.2016 16:08, Denis V. Lunev wrote:
>> There is VM with 8 GB QCOW2 storage. Real size of the image file is 2 GB.
>> 'drive-mirror' redirected to NBD target creates 8 GB image at destination.
>> The situation is even worse as zeroes are sent through the channel.
>>
>> The patch simply adds .bdrv_co_write_zeroes callback to NBD block driver
>> which works though NBD_TRIM to avoid transfer of zeroes.
> The specification[1] says the following about TRIM:
>
>> After issuing this command, a client MUST NOT make any assumptions
>> about the contents of the export affected by this command, until
>> overwriting it again with NBD_CMD_WRITE.
> So I don't think this is correct.
>
> The correct solution would probably to introduce a specific command to
> write zeroes, or at least to use detect-zeroes on the NBD server side
> (but this will still lead to the zeroes being sent over the line).
>
> Max
>
> [1] https://github.com/yoe/nbd/blob/master/doc/proto.md
>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>   block/nbd-client.c | 10 ++++++++++
>>   block/nbd-client.h |  2 ++
>>   block/nbd.c        |  9 +++++++++
>>   3 files changed, 21 insertions(+)
>
This sounds not that good. OK. At least there is correct
lengthy way for this. White-outs must be supported
by the protocol.

Den