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 <[email protected]> > --- > 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
