[Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Yi Fang]

NBD client has not implemented callback for .bdrv_has_zero_init. So
bdrv_has_zero_init always returns 0 when doing non-shared storage
migration.
This patch implemented NBD_CMD_HAS_ZERO_INIT and will avoid unnecessary
set-dirty.

Signed-off-by: Yi Fang <eric.fangyi@huawei.com>
---
 block/block-backend.c          |  5 +++++
 block/nbd-client.c             | 28 ++++++++++++++++++++++++++++
 block/nbd-client.h             |  1 +
 block/nbd.c                    |  8 ++++++++
 include/block/nbd.h            |  3 +++
 include/sysemu/block-backend.h |  1 +
 nbd/server.c                   | 10 +++++++++-
 7 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index efbf398..4369c85 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1239,6 +1239,11 @@ void blk_drain_all(void)
     bdrv_drain_all();
 }
 
+int blk_has_zero_init(BlockBackend *blk)
+{
+        return bdrv_has_zero_init(blk_bs(blk));
+}
+
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
                       BlockdevOnError on_write_error)
 {
diff --git a/block/nbd-client.c b/block/nbd-client.c
index 3779c6c..8b1d98d 100644
--- a/block/nbd-client.c
+++ b/block/nbd-client.c
@@ -376,6 +376,34 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
                        false, nbd_reply_ready, NULL, bs);
 }
 
+int nbd_client_co_has_zero_init(BlockDriverState *bs)
+{
+    NBDClientSession *client = nbd_get_client_session(bs);
+    NBDRequest request = { .type = NBD_CMD_HAS_ZERO_INIT };
+    NBDReply reply;
+    ssize_t ret;
+
+    if (!(client->nbdflags & NBD_FLAG_HAS_ZERO_INIT)) {
+        return 0;
+    }
+
+    request.from = 0;
+    request.len = 0;
+
+    nbd_coroutine_start(client, &request);
+    ret = nbd_co_send_request(bs, &request, NULL);
+    if (ret < 0) {
+        reply.error = -ret;
+    } else {
+        nbd_co_receive_reply(client, &request, &reply, NULL);
+    }
+    nbd_coroutine_end(client, &request);
+    if (reply.error == 0) {
+        return 1;
+    }
+    return 0;
+}
+
 void nbd_client_close(BlockDriverState *bs)
 {
     NBDClientSession *client = nbd_get_client_session(bs);
diff --git a/block/nbd-client.h b/block/nbd-client.h
index f8d6006..ec01938 100644
--- a/block/nbd-client.h
+++ b/block/nbd-client.h
@@ -56,5 +56,6 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
 void nbd_client_detach_aio_context(BlockDriverState *bs);
 void nbd_client_attach_aio_context(BlockDriverState *bs,
                                    AioContext *new_context);
+int nbd_client_co_has_zero_init(BlockDriverState *bs);
 
 #endif /* NBD_CLIENT_H */
diff --git a/block/nbd.c b/block/nbd.c
index 35f24be..40dd9a2 100644
--- a/block/nbd.c
+++ b/block/nbd.c
@@ -552,6 +552,11 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
     bs->full_open_options = opts;
 }
 
+static int nbd_co_has_zero_init(BlockDriverState *bs)
+{
+    return nbd_client_co_has_zero_init(bs);
+}
+
 static BlockDriver bdrv_nbd = {
     .format_name                = "nbd",
     .protocol_name              = "nbd",
@@ -569,6 +574,7 @@ static BlockDriver bdrv_nbd = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_co_has_zero_init,
 };
 
 static BlockDriver bdrv_nbd_tcp = {
@@ -588,6 +594,7 @@ static BlockDriver bdrv_nbd_tcp = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_co_has_zero_init,
 };
 
 static BlockDriver bdrv_nbd_unix = {
@@ -607,6 +614,7 @@ static BlockDriver bdrv_nbd_unix = {
     .bdrv_detach_aio_context    = nbd_detach_aio_context,
     .bdrv_attach_aio_context    = nbd_attach_aio_context,
     .bdrv_refresh_filename      = nbd_refresh_filename,
+    .bdrv_has_zero_init         = nbd_co_has_zero_init,
 };
 
 static void bdrv_nbd_init(void)
diff --git a/include/block/nbd.h b/include/block/nbd.h
index 3e373f0..9aa9265 100644
--- a/include/block/nbd.h
+++ b/include/block/nbd.h
@@ -72,6 +72,7 @@ typedef struct NBDReply NBDReply;
 #define NBD_FLAG_ROTATIONAL     (1 << 4)        /* Use elevator algorithm - rotational media */
 #define NBD_FLAG_SEND_TRIM      (1 << 5)        /* Send TRIM (discard) */
 #define NBD_FLAG_SEND_WRITE_ZEROES (1 << 6)     /* Send WRITE_ZEROES */
+#define NBD_FLAG_HAS_ZERO_INIT  (1 << 8)        /* Send HAS_ZERO_INIT */
 
 /* New-style handshake (global) flags, sent from server to client, and
    control what will happen during handshake phase. */
@@ -109,6 +110,8 @@ enum {
     NBD_CMD_TRIM = 4,
     /* 5 reserved for failed experiment NBD_CMD_CACHE */
     NBD_CMD_WRITE_ZEROES = 6,
+    /* 7 reserved for STRUCTURED_REPLY */
+    NBD_CMD_HAS_ZERO_INIT = 8,
 };
 
 #define NBD_DEFAULT_PORT	10809
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 6444e41..9bd0822 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -156,6 +156,7 @@ int blk_flush(BlockBackend *blk);
 int blk_commit_all(void);
 void blk_drain(BlockBackend *blk);
 void blk_drain_all(void);
+int blk_has_zero_init(BlockBackend *blk);
 void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
                       BlockdevOnError on_write_error);
 BlockdevOnError blk_get_on_error(BlockBackend *blk, bool is_read);
diff --git a/nbd/server.c b/nbd/server.c
index 5b76261..7ae91b7 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -617,7 +617,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData *data)
     int rc;
     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_WRITE_ZEROES |
+                              NBD_FLAG_HAS_ZERO_INIT);
     bool oldStyle;
     size_t len;
 
@@ -1321,6 +1322,13 @@ static void nbd_trip(void *opaque)
             goto out;
         }
         break;
+    case NBD_CMD_HAS_ZERO_INIT:
+        TRACE("Request type is HAS_ZERO_INIT");
+        reply.error = !blk_has_zero_init(exp->blk);
+        if (nbd_co_send_reply(req, &reply, 0) < 0) {
+            goto out;
+        }
+        break;
     default:
         LOG("invalid request type (%" PRIu32 ") received", request.type);
         reply.error = EINVAL;
-- 
1.8.5

Re: [Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Stefan Hajnoczi]

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

On Sun, Dec 04, 2016 at 11:44:57PM +0000, Yi Fang wrote:
> NBD client has not implemented callback for .bdrv_has_zero_init. So
> bdrv_has_zero_init always returns 0 when doing non-shared storage
> migration.
> This patch implemented NBD_CMD_HAS_ZERO_INIT and will avoid unnecessary
> set-dirty.

Please link to the NBD spec where this new command is defined.  Has it
already been accepted by the NBD community?

I think this is a weird command because it's information that doesn't
change over the lifetime of an NBD session.  Your patch sends a command
and waits for the reply every time has_zero_init() is queried.

Is there a better place to put this information in the NBD protocols,
like some export information that is retried during connection?  (I
haven't checked the protocol.)  It seems weird to send a command and
wait for the response.

> Signed-off-by: Yi Fang <eric.fangyi@huawei.com>
> ---
>  block/block-backend.c          |  5 +++++
>  block/nbd-client.c             | 28 ++++++++++++++++++++++++++++
>  block/nbd-client.h             |  1 +
>  block/nbd.c                    |  8 ++++++++
>  include/block/nbd.h            |  3 +++
>  include/sysemu/block-backend.h |  1 +
>  nbd/server.c                   | 10 +++++++++-
>  7 files changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/block/block-backend.c b/block/block-backend.c
> index efbf398..4369c85 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -1239,6 +1239,11 @@ void blk_drain_all(void)
>      bdrv_drain_all();
>  }
>  
> +int blk_has_zero_init(BlockBackend *blk)
> +{
> +        return bdrv_has_zero_init(blk_bs(blk));

Please run scripts/checkpatch.pl to check for coding style issues.
Indentation should be 4 spaces.

> +}
> +
>  void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
>                        BlockdevOnError on_write_error)
>  {
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 3779c6c..8b1d98d 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -376,6 +376,34 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
>                         false, nbd_reply_ready, NULL, bs);
>  }
>  
> +int nbd_client_co_has_zero_init(BlockDriverState *bs)

Coroutine functions must be marked:

int coroutine_fn nbd_client_co_has_zero_init()

> +{
> +    NBDClientSession *client = nbd_get_client_session(bs);
> +    NBDRequest request = { .type = NBD_CMD_HAS_ZERO_INIT };
> +    NBDReply reply;
> +    ssize_t ret;
> +
> +    if (!(client->nbdflags & NBD_FLAG_HAS_ZERO_INIT)) {
> +        return 0;
> +    }
> +
> +    request.from = 0;
> +    request.len = 0;
> +
> +    nbd_coroutine_start(client, &request);
> +    ret = nbd_co_send_request(bs, &request, NULL);
> +    if (ret < 0) {
> +        reply.error = -ret;
> +    } else {
> +        nbd_co_receive_reply(client, &request, &reply, NULL);
> +    }
> +    nbd_coroutine_end(client, &request);
> +    if (reply.error == 0) {
> +        return 1;
> +    }
> +    return 0;
> +}
> +
>  void nbd_client_close(BlockDriverState *bs)
>  {
>      NBDClientSession *client = nbd_get_client_session(bs);
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index f8d6006..ec01938 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -56,5 +56,6 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
>  void nbd_client_detach_aio_context(BlockDriverState *bs);
>  void nbd_client_attach_aio_context(BlockDriverState *bs,
>                                     AioContext *new_context);
> +int nbd_client_co_has_zero_init(BlockDriverState *bs);
>  
>  #endif /* NBD_CLIENT_H */
> diff --git a/block/nbd.c b/block/nbd.c
> index 35f24be..40dd9a2 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -552,6 +552,11 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>      bs->full_open_options = opts;
>  }
>  
> +static int nbd_co_has_zero_init(BlockDriverState *bs)
> +{
> +    return nbd_client_co_has_zero_init(bs);
> +}
> +
>  static BlockDriver bdrv_nbd = {
>      .format_name                = "nbd",
>      .protocol_name              = "nbd",
> @@ -569,6 +574,7 @@ static BlockDriver bdrv_nbd = {
>      .bdrv_detach_aio_context    = nbd_detach_aio_context,
>      .bdrv_attach_aio_context    = nbd_attach_aio_context,
>      .bdrv_refresh_filename      = nbd_refresh_filename,
> +    .bdrv_has_zero_init         = nbd_co_has_zero_init,

.bdrv_has_zero_init() is not a coroutine_fn.  It is not okay to yield!

It would be best to fetch the information during connection so that we
don't have to send a command and wait for a reply every time
.bdrv_has_zero_init() is called.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Eric Blake]

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

On 12/05/2016 05:09 AM, Stefan Hajnoczi wrote:
> On Sun, Dec 04, 2016 at 11:44:57PM +0000, Yi Fang wrote:
>> NBD client has not implemented callback for .bdrv_has_zero_init. So
>> bdrv_has_zero_init always returns 0 when doing non-shared storage
>> migration.
>> This patch implemented NBD_CMD_HAS_ZERO_INIT and will avoid unnecessary
>> set-dirty.
> 
> Please link to the NBD spec where this new command is defined.  Has it
> already been accepted by the NBD community?

No. This is the first I've seen of such a proposal.

> 
> I think this is a weird command because it's information that doesn't
> change over the lifetime of an NBD session.  Your patch sends a command
> and waits for the reply every time has_zero_init() is queried.
> 
> Is there a better place to put this information in the NBD protocols,
> like some export information that is retried during connection?  (I
> haven't checked the protocol.)  It seems weird to send a command and
> wait for the response.

Agreed - this patch is wrong. Even if you DO get the NBD community to
buy into this, it should be done as a new NBD_FLAG_* sent once up-front
during handshake phase in reply to NBD_OPT_EXPORT_NAME/NBD_OPT_GO, and
NOT a new command that can be arbitrarily invoked multiple times during
transmission phase.

Is the goal of the flag for the server to be able to advertise to the
client that upon initial connection, the server asserts that the volume
currently reads as all zeroes (and therefore the client can optimize by
not writing zeroes)?

Do you need me to help re-write this into a proposal to the NBD
community that might stand a chance of being accepted?

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Fangyi (C)]

Yes, you are right. It's not necessary to use such a command. I will 
subscribe nbd-general mail-list and implement has_zero_init during 
connection.
Thank you!

在 2016/12/5 19:09, Stefan Hajnoczi 写道:
> On Sun, Dec 04, 2016 at 11:44:57PM +0000, Yi Fang wrote:
>> NBD client has not implemented callback for .bdrv_has_zero_init. So
>> bdrv_has_zero_init always returns 0 when doing non-shared storage
>> migration.
>> This patch implemented NBD_CMD_HAS_ZERO_INIT and will avoid unnecessary
>> set-dirty.
>
> Please link to the NBD spec where this new command is defined.  Has it
> already been accepted by the NBD community?
>
> I think this is a weird command because it's information that doesn't
> change over the lifetime of an NBD session.  Your patch sends a command
> and waits for the reply every time has_zero_init() is queried.
>
> Is there a better place to put this information in the NBD protocols,
> like some export information that is retried during connection?  (I
> haven't checked the protocol.)  It seems weird to send a command and
> wait for the response.
>
>> Signed-off-by: Yi Fang <eric.fangyi@huawei.com>
>> ---
>>   block/block-backend.c          |  5 +++++
>>   block/nbd-client.c             | 28 ++++++++++++++++++++++++++++
>>   block/nbd-client.h             |  1 +
>>   block/nbd.c                    |  8 ++++++++
>>   include/block/nbd.h            |  3 +++
>>   include/sysemu/block-backend.h |  1 +
>>   nbd/server.c                   | 10 +++++++++-
>>   7 files changed, 55 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/block-backend.c b/block/block-backend.c
>> index efbf398..4369c85 100644
>> --- a/block/block-backend.c
>> +++ b/block/block-backend.c
>> @@ -1239,6 +1239,11 @@ void blk_drain_all(void)
>>       bdrv_drain_all();
>>   }
>>
>> +int blk_has_zero_init(BlockBackend *blk)
>> +{
>> +        return bdrv_has_zero_init(blk_bs(blk));
>
> Please run scripts/checkpatch.pl to check for coding style issues.
> Indentation should be 4 spaces.
>
>> +}
>> +
>>   void blk_set_on_error(BlockBackend *blk, BlockdevOnError on_read_error,
>>                         BlockdevOnError on_write_error)
>>   {
>> diff --git a/block/nbd-client.c b/block/nbd-client.c
>> index 3779c6c..8b1d98d 100644
>> --- a/block/nbd-client.c
>> +++ b/block/nbd-client.c
>> @@ -376,6 +376,34 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
>>                          false, nbd_reply_ready, NULL, bs);
>>   }
>>
>> +int nbd_client_co_has_zero_init(BlockDriverState *bs)
>
> Coroutine functions must be marked:
>
> int coroutine_fn nbd_client_co_has_zero_init()
>
>> +{
>> +    NBDClientSession *client = nbd_get_client_session(bs);
>> +    NBDRequest request = { .type = NBD_CMD_HAS_ZERO_INIT };
>> +    NBDReply reply;
>> +    ssize_t ret;
>> +
>> +    if (!(client->nbdflags & NBD_FLAG_HAS_ZERO_INIT)) {
>> +        return 0;
>> +    }
>> +
>> +    request.from = 0;
>> +    request.len = 0;
>> +
>> +    nbd_coroutine_start(client, &request);
>> +    ret = nbd_co_send_request(bs, &request, NULL);
>> +    if (ret < 0) {
>> +        reply.error = -ret;
>> +    } else {
>> +        nbd_co_receive_reply(client, &request, &reply, NULL);
>> +    }
>> +    nbd_coroutine_end(client, &request);
>> +    if (reply.error == 0) {
>> +        return 1;
>> +    }
>> +    return 0;
>> +}
>> +
>>   void nbd_client_close(BlockDriverState *bs)
>>   {
>>       NBDClientSession *client = nbd_get_client_session(bs);
>> diff --git a/block/nbd-client.h b/block/nbd-client.h
>> index f8d6006..ec01938 100644
>> --- a/block/nbd-client.h
>> +++ b/block/nbd-client.h
>> @@ -56,5 +56,6 @@ int nbd_client_co_preadv(BlockDriverState *bs, uint64_t offset,
>>   void nbd_client_detach_aio_context(BlockDriverState *bs);
>>   void nbd_client_attach_aio_context(BlockDriverState *bs,
>>                                      AioContext *new_context);
>> +int nbd_client_co_has_zero_init(BlockDriverState *bs);
>>
>>   #endif /* NBD_CLIENT_H */
>> diff --git a/block/nbd.c b/block/nbd.c
>> index 35f24be..40dd9a2 100644
>> --- a/block/nbd.c
>> +++ b/block/nbd.c
>> @@ -552,6 +552,11 @@ static void nbd_refresh_filename(BlockDriverState *bs, QDict *options)
>>       bs->full_open_options = opts;
>>   }
>>
>> +static int nbd_co_has_zero_init(BlockDriverState *bs)
>> +{
>> +    return nbd_client_co_has_zero_init(bs);
>> +}
>> +
>>   static BlockDriver bdrv_nbd = {
>>       .format_name                = "nbd",
>>       .protocol_name              = "nbd",
>> @@ -569,6 +574,7 @@ static BlockDriver bdrv_nbd = {
>>       .bdrv_detach_aio_context    = nbd_detach_aio_context,
>>       .bdrv_attach_aio_context    = nbd_attach_aio_context,
>>       .bdrv_refresh_filename      = nbd_refresh_filename,
>> +    .bdrv_has_zero_init         = nbd_co_has_zero_init,
>
> .bdrv_has_zero_init() is not a coroutine_fn.  It is not okay to yield!
>
> It would be best to fetch the information during connection so that we
> don't have to send a command and wait for a reply every time
> .bdrv_has_zero_init() is called.
>

Re: [Qemu-devel] [PATCH] nbd: Implement NBD_CMD_HAS_ZERO_INIT

[Fangyi (C)]

I have already seen your proposal mail for NBD_FLAG_INIT_ZEROES extension.
Thank you!


在 2016/12/6 0:54, Eric Blake 写道:
> On 12/05/2016 05:09 AM, Stefan Hajnoczi wrote:
>> On Sun, Dec 04, 2016 at 11:44:57PM +0000, Yi Fang wrote:
>>> NBD client has not implemented callback for .bdrv_has_zero_init. So
>>> bdrv_has_zero_init always returns 0 when doing non-shared storage
>>> migration.
>>> This patch implemented NBD_CMD_HAS_ZERO_INIT and will avoid unnecessary
>>> set-dirty.
>>
>> Please link to the NBD spec where this new command is defined.  Has it
>> already been accepted by the NBD community?
>
> No. This is the first I've seen of such a proposal.
>
>>
>> I think this is a weird command because it's information that doesn't
>> change over the lifetime of an NBD session.  Your patch sends a command
>> and waits for the reply every time has_zero_init() is queried.
>>
>> Is there a better place to put this information in the NBD protocols,
>> like some export information that is retried during connection?  (I
>> haven't checked the protocol.)  It seems weird to send a command and
>> wait for the response.
>
> Agreed - this patch is wrong. Even if you DO get the NBD community to
> buy into this, it should be done as a new NBD_FLAG_* sent once up-front
> during handshake phase in reply to NBD_OPT_EXPORT_NAME/NBD_OPT_GO, and
> NOT a new command that can be arbitrarily invoked multiple times during
> transmission phase.
>
> Is the goal of the flag for the server to be able to advertise to the
> client that upon initial connection, the server asserts that the volume
> currently reads as all zeroes (and therefore the client can optimize by
> not writing zeroes)?
>
> Do you need me to help re-write this into a proposal to the NBD
> community that might stand a chance of being accepted?
>