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

[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 <address@hidden>
> ---
>  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.

signature.asc
Description: PGP signature