[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 16/18] nbd: Support NBD_CMD_CLOSE
From: |
Alex Bligh |
Subject: |
Re: [Qemu-devel] [PATCH 16/18] nbd: Support NBD_CMD_CLOSE |
Date: |
Sat, 9 Apr 2016 11:50:12 +0100 |
On 8 Apr 2016, at 23:05, Eric Blake <address@hidden> wrote:
> NBD_CMD_DISC is annoying: the server is not required to reply,
> so the client has no choice but to disconnect once it has sent
> the message; but depending on timing, the server can see the
> disconnect prior to reading the request, and treat things as
> an abrupt exit rather than a clean shutdown (which may affect
> whether the server properly fsync()s data to disk, and so on).
> The new NBD_CMD_CLOSE adds another round of handshake, where
> the client waits for the server's action before closing, to
> make sure both parties know that it was a clean close rather
> than an accidental early disconnect.
>
> In nbd-client.c, nbd_client_close() is called after we have
> already exited the normal coroutine context used by all the
> other transmission phase handlers, so the code is a bit more
> complex to build up a coroutine just for the purpose of waiting
> for the server's response.
>
> Signed-off-by: Eric Blake <address@hidden>
Wouter is not yet convinced of the merits of NBD_CMD_CLOSE
so we should probably resist applying this unless / until we
have convinced him of its benefits.
BTW there is nothing to stop you doing an fsync() on ANY
disconnect server side.
Alex
> ---
> include/block/nbd.h | 4 +++-
> block/nbd-client.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
> nbd/server.c | 19 +++++++++++++++++--
> 3 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/include/block/nbd.h b/include/block/nbd.h
> index d261dbc..4c57754 100644
> --- a/include/block/nbd.h
> +++ b/include/block/nbd.h
> @@ -70,6 +70,7 @@ typedef struct nbd_reply nbd_reply;
> #define NBD_FLAG_SEND_FUA (1 << 3) /* Send FUA (Force Unit
> Access) */
> #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_CLOSE (1 << 8) /* Send CLOSE */
>
> /* New-style handshake (global) flags, sent from server to client, and
> control what will happen during handshake phase. */
> @@ -99,7 +100,8 @@ enum {
> NBD_CMD_WRITE = 1,
> NBD_CMD_DISC = 2,
> NBD_CMD_FLUSH = 3,
> - NBD_CMD_TRIM = 4
> + NBD_CMD_TRIM = 4,
> + NBD_CMD_CLOSE = 7,
> };
>
> #define NBD_DEFAULT_PORT 10809
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 285025d..f013084 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -374,6 +374,29 @@ void nbd_client_attach_aio_context(BlockDriverState *bs,
> false, nbd_reply_ready, NULL, bs);
> }
>
> +typedef struct NbdCloseCo {
> + BlockDriverState *bs;
> + nbd_request request;
> + nbd_reply reply;
> + bool done;
> +} NbdCloseCo;
> +
> +static void coroutine_fn nbd_client_close_co(void *opaque)
> +{
> + NbdCloseCo *closeco = opaque;
> + NbdClientSession *client = nbd_get_client_session(closeco->bs);
> + ssize_t ret;
> +
> + nbd_coroutine_start(client, &closeco->request);
> + ret = nbd_co_send_request(closeco->bs, &closeco->request, NULL, 0);
> + if (ret >= 0) {
> + nbd_co_receive_reply(client, &closeco->request, &closeco->reply,
> + NULL, 0);
> + }
> + nbd_coroutine_end(client, &closeco->request);
> + closeco->done = true;
> +}
> +
> void nbd_client_close(BlockDriverState *bs)
> {
> NbdClientSession *client = nbd_get_client_session(bs);
> @@ -383,8 +406,28 @@ void nbd_client_close(BlockDriverState *bs)
> return;
> }
>
> - nbd_send_request(client->ioc, &request);
> + if (client->nbdflags & NBD_FLAG_SEND_CLOSE) {
> + /* Newer server, wants us to wait for reply before we close */
> + Coroutine *co;
> + NbdCloseCo closeco = {
> + .bs = bs,
> + .request = { .type = NBD_CMD_CLOSE },
> + };
> + AioContext *aio_context;
>
> + g_assert(!qemu_in_coroutine());
> + aio_context = bdrv_get_aio_context(bs);
> + co = qemu_coroutine_create(nbd_client_close_co);
> + qemu_coroutine_enter(co, &closeco);
> + while (!closeco.done) {
> + aio_poll(aio_context, true);
> + }
> + } else {
> + /* Older server, send request, but no reply will come */
> + nbd_send_request(client->ioc, &request);
> + }
> +
> + /* Regardless of any received errors, the connection is done. */
> nbd_teardown_connection(bs);
> }
>
> diff --git a/nbd/server.c b/nbd/server.c
> index e68e83c..2a6eaf2 100644
> --- a/nbd/server.c
> +++ b/nbd/server.c
> @@ -624,7 +624,8 @@ static coroutine_fn int nbd_negotiate(NBDClientNewData
> *data)
> char buf[8 + 8 + 8 + 128];
> 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_FLUSH | NBD_FLAG_SEND_FUA |
> + NBD_FLAG_SEND_CLOSE);
> bool oldStyle;
> size_t len;
>
> @@ -1244,7 +1245,21 @@ static void nbd_trip(void *opaque)
> break;
> case NBD_CMD_DISC:
> TRACE("Request type is DISCONNECT");
> - errno = 0;
> + goto out;
> + case NBD_CMD_CLOSE:
> + TRACE("Request type is CLOSE");
> + if (request.flags || request.from || request.len) {
> + LOG("bad parameters, skipping flush");
> + reply.error = EINVAL;
> + } else {
> + ret = blk_co_flush(exp->blk);
> + if (ret < 0) {
> + LOG("flush failed");
> + reply.error = -ret;
> + }
> + }
> + /* Attempt to send reply, but even if it fails, we are done */
> + nbd_co_send_reply(req, &reply, 0);
> goto out;
> case NBD_CMD_FLUSH:
> TRACE("Request type is FLUSH");
> --
> 2.5.5
>
>
--
Alex Bligh
[Qemu-devel] [PATCH 11/18] nbd: Let client skip portions of server reply, Eric Blake, 2016/04/08
Re: [Qemu-devel] [Nbd] [RFC PATCH 00/18] NBD protocol additions, Wouter Verhelst, 2016/04/09