From: Paolo Bonzini <pbonzini@redhat.com>
To: Max Reitz <mreitz@redhat.com>, qemu-devel@nongnu.org
Cc: Kevin Wolf <kwolf@redhat.com>, Stefan Hajnoczi <stefanha@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 1/3] nbd: Drop BDS backpointer
Date: Tue, 03 Feb 2015 09:37:00 +0100 [thread overview]
Message-ID: <54D088AC.8030607@redhat.com> (raw)
In-Reply-To: <1422913238-7280-2-git-send-email-mreitz@redhat.com>
On 02/02/2015 22:40, Max Reitz wrote:
> Before this patch, the "opaque" pointer in an NBD BDS points to a
> BDRVNBDState, which contains an NbdClientSession object, which in turn
> contains a pointer to the BDS. This pointer may become invalid due to
> bdrv_swap(), so drop it, and instead pass the BDS directly to the
> nbd-client.c functions which then retrieve the NbdClientSession object
> from there.
Looks good, but please change function names from nbd_client_session_foo
to nbd_client_foo or even just nbd_foo if they do not take an
NbdClientSession* as the first parameter.
Thanks,
Paolo
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> block/nbd-client.c | 95 ++++++++++++++++++++++++++++--------------------------
> block/nbd-client.h | 20 ++++++------
> block/nbd.c | 37 ++++++++-------------
> 3 files changed, 73 insertions(+), 79 deletions(-)
>
> diff --git a/block/nbd-client.c b/block/nbd-client.c
> index 28bfb62..4ede714 100644
> --- a/block/nbd-client.c
> +++ b/block/nbd-client.c
> @@ -43,20 +43,23 @@ static void nbd_recv_coroutines_enter_all(NbdClientSession *s)
> }
> }
>
> -static void nbd_teardown_connection(NbdClientSession *client)
> +static void nbd_teardown_connection(BlockDriverState *bs)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> +
> /* finish any pending coroutines */
> shutdown(client->sock, 2);
> nbd_recv_coroutines_enter_all(client);
>
> - nbd_client_session_detach_aio_context(client);
> + nbd_client_session_detach_aio_context(bs);
> closesocket(client->sock);
> client->sock = -1;
> }
>
> static void nbd_reply_ready(void *opaque)
> {
> - NbdClientSession *s = opaque;
> + BlockDriverState *bs = opaque;
> + NbdClientSession *s = nbd_get_client_session(bs);
> uint64_t i;
> int ret;
>
> @@ -89,28 +92,29 @@ static void nbd_reply_ready(void *opaque)
> }
>
> fail:
> - nbd_teardown_connection(s);
> + nbd_teardown_connection(bs);
> }
>
> static void nbd_restart_write(void *opaque)
> {
> - NbdClientSession *s = opaque;
> + BlockDriverState *bs = opaque;
>
> - qemu_coroutine_enter(s->send_coroutine, NULL);
> + qemu_coroutine_enter(nbd_get_client_session(bs)->send_coroutine, NULL);
> }
>
> -static int nbd_co_send_request(NbdClientSession *s,
> - struct nbd_request *request,
> - QEMUIOVector *qiov, int offset)
> +static int nbd_co_send_request(BlockDriverState *bs,
> + struct nbd_request *request,
> + QEMUIOVector *qiov, int offset)
> {
> + NbdClientSession *s = nbd_get_client_session(bs);
> AioContext *aio_context;
> int rc, ret;
>
> qemu_co_mutex_lock(&s->send_mutex);
> s->send_coroutine = qemu_coroutine_self();
> - aio_context = bdrv_get_aio_context(s->bs);
> + aio_context = bdrv_get_aio_context(bs);
> aio_set_fd_handler(aio_context, s->sock,
> - nbd_reply_ready, nbd_restart_write, s);
> + nbd_reply_ready, nbd_restart_write, bs);
> if (qiov) {
> if (!s->is_unix) {
> socket_set_cork(s->sock, 1);
> @@ -129,7 +133,7 @@ static int nbd_co_send_request(NbdClientSession *s,
> } else {
> rc = nbd_send_request(s->sock, request);
> }
> - aio_set_fd_handler(aio_context, s->sock, nbd_reply_ready, NULL, s);
> + aio_set_fd_handler(aio_context, s->sock, nbd_reply_ready, NULL, bs);
> s->send_coroutine = NULL;
> qemu_co_mutex_unlock(&s->send_mutex);
> return rc;
> @@ -195,10 +199,11 @@ static void nbd_coroutine_end(NbdClientSession *s,
> }
> }
>
> -static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num,
> +static int nbd_co_readv_1(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov,
> int offset)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> struct nbd_request request = { .type = NBD_CMD_READ };
> struct nbd_reply reply;
> ssize_t ret;
> @@ -207,7 +212,7 @@ static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num,
> request.len = nb_sectors * 512;
>
> nbd_coroutine_start(client, &request);
> - ret = nbd_co_send_request(client, &request, NULL, 0);
> + ret = nbd_co_send_request(bs, &request, NULL, 0);
> if (ret < 0) {
> reply.error = -ret;
> } else {
> @@ -218,15 +223,16 @@ static int nbd_co_readv_1(NbdClientSession *client, int64_t sector_num,
>
> }
>
> -static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num,
> +static int nbd_co_writev_1(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov,
> int offset)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> struct nbd_request request = { .type = NBD_CMD_WRITE };
> struct nbd_reply reply;
> ssize_t ret;
>
> - if (!bdrv_enable_write_cache(client->bs) &&
> + if (!bdrv_enable_write_cache(bs) &&
> (client->nbdflags & NBD_FLAG_SEND_FUA)) {
> request.type |= NBD_CMD_FLAG_FUA;
> }
> @@ -235,7 +241,7 @@ static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num,
> request.len = nb_sectors * 512;
>
> nbd_coroutine_start(client, &request);
> - ret = nbd_co_send_request(client, &request, qiov, offset);
> + ret = nbd_co_send_request(bs, &request, qiov, offset);
> if (ret < 0) {
> reply.error = -ret;
> } else {
> @@ -249,14 +255,13 @@ static int nbd_co_writev_1(NbdClientSession *client, int64_t sector_num,
> * remain aligned to 4K. */
> #define NBD_MAX_SECTORS 2040
>
> -int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num,
> - int nb_sectors, QEMUIOVector *qiov)
> +int nbd_client_session_co_readv(BlockDriverState *bs, int64_t sector_num,
> + int nb_sectors, QEMUIOVector *qiov)
> {
> int offset = 0;
> int ret;
> while (nb_sectors > NBD_MAX_SECTORS) {
> - ret = nbd_co_readv_1(client, sector_num,
> - NBD_MAX_SECTORS, qiov, offset);
> + ret = nbd_co_readv_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset);
> if (ret < 0) {
> return ret;
> }
> @@ -264,17 +269,16 @@ int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num,
> sector_num += NBD_MAX_SECTORS;
> nb_sectors -= NBD_MAX_SECTORS;
> }
> - return nbd_co_readv_1(client, sector_num, nb_sectors, qiov, offset);
> + return nbd_co_readv_1(bs, sector_num, nb_sectors, qiov, offset);
> }
>
> -int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num,
> +int nbd_client_session_co_writev(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> int offset = 0;
> int ret;
> while (nb_sectors > NBD_MAX_SECTORS) {
> - ret = nbd_co_writev_1(client, sector_num,
> - NBD_MAX_SECTORS, qiov, offset);
> + ret = nbd_co_writev_1(bs, sector_num, NBD_MAX_SECTORS, qiov, offset);
> if (ret < 0) {
> return ret;
> }
> @@ -282,11 +286,12 @@ int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num,
> sector_num += NBD_MAX_SECTORS;
> nb_sectors -= NBD_MAX_SECTORS;
> }
> - return nbd_co_writev_1(client, sector_num, nb_sectors, qiov, offset);
> + return nbd_co_writev_1(bs, sector_num, nb_sectors, qiov, offset);
> }
>
> -int nbd_client_session_co_flush(NbdClientSession *client)
> +int nbd_client_session_co_flush(BlockDriverState *bs)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> struct nbd_request request = { .type = NBD_CMD_FLUSH };
> struct nbd_reply reply;
> ssize_t ret;
> @@ -303,7 +308,7 @@ int nbd_client_session_co_flush(NbdClientSession *client)
> request.len = 0;
>
> nbd_coroutine_start(client, &request);
> - ret = nbd_co_send_request(client, &request, NULL, 0);
> + ret = nbd_co_send_request(bs, &request, NULL, 0);
> if (ret < 0) {
> reply.error = -ret;
> } else {
> @@ -313,9 +318,10 @@ int nbd_client_session_co_flush(NbdClientSession *client)
> return -reply.error;
> }
>
> -int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num,
> - int nb_sectors)
> +int nbd_client_session_co_discard(BlockDriverState *bs, int64_t sector_num,
> + int nb_sectors)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> struct nbd_request request = { .type = NBD_CMD_TRIM };
> struct nbd_reply reply;
> ssize_t ret;
> @@ -327,7 +333,7 @@ int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num,
> request.len = nb_sectors * 512;
>
> nbd_coroutine_start(client, &request);
> - ret = nbd_co_send_request(client, &request, NULL, 0);
> + ret = nbd_co_send_request(bs, &request, NULL, 0);
> if (ret < 0) {
> reply.error = -ret;
> } else {
> @@ -338,43 +344,41 @@ int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num,
>
> }
>
> -void nbd_client_session_detach_aio_context(NbdClientSession *client)
> +void nbd_client_session_detach_aio_context(BlockDriverState *bs)
> {
> - aio_set_fd_handler(bdrv_get_aio_context(client->bs), client->sock,
> - NULL, NULL, NULL);
> + aio_set_fd_handler(bdrv_get_aio_context(bs),
> + nbd_get_client_session(bs)->sock, NULL, NULL, NULL);
> }
>
> -void nbd_client_session_attach_aio_context(NbdClientSession *client,
> +void nbd_client_session_attach_aio_context(BlockDriverState *bs,
> AioContext *new_context)
> {
> - aio_set_fd_handler(new_context, client->sock,
> - nbd_reply_ready, NULL, client);
> + aio_set_fd_handler(new_context, nbd_get_client_session(bs)->sock,
> + nbd_reply_ready, NULL, bs);
> }
>
> -void nbd_client_session_close(NbdClientSession *client)
> +void nbd_client_session_close(BlockDriverState *bs)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> struct nbd_request request = {
> .type = NBD_CMD_DISC,
> .from = 0,
> .len = 0
> };
>
> - if (!client->bs) {
> - return;
> - }
> if (client->sock == -1) {
> return;
> }
>
> nbd_send_request(client->sock, &request);
>
> - nbd_teardown_connection(client);
> - client->bs = NULL;
> + nbd_teardown_connection(bs);
> }
>
> -int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs,
> +int nbd_client_session_init(BlockDriverState *bs,
> int sock, const char *export, Error **errp)
> {
> + NbdClientSession *client = nbd_get_client_session(bs);
> int ret;
>
> /* NBD handshake */
> @@ -391,13 +395,12 @@ int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs,
>
> qemu_co_mutex_init(&client->send_mutex);
> qemu_co_mutex_init(&client->free_sema);
> - client->bs = bs;
> client->sock = sock;
>
> /* Now that we're connected, set the socket to be non-blocking and
> * kick the reply mechanism. */
> qemu_set_nonblock(sock);
> - nbd_client_session_attach_aio_context(client, bdrv_get_aio_context(bs));
> + nbd_client_session_attach_aio_context(bs, bdrv_get_aio_context(bs));
>
> logout("Established connection with NBD server\n");
> return 0;
> diff --git a/block/nbd-client.h b/block/nbd-client.h
> index cfeecc2..49daccc 100644
> --- a/block/nbd-client.h
> +++ b/block/nbd-client.h
> @@ -31,24 +31,24 @@ typedef struct NbdClientSession {
> struct nbd_reply reply;
>
> bool is_unix;
> -
> - BlockDriverState *bs;
> } NbdClientSession;
>
> -int nbd_client_session_init(NbdClientSession *client, BlockDriverState *bs,
> +NbdClientSession *nbd_get_client_session(BlockDriverState *bs);
> +
> +int nbd_client_session_init(BlockDriverState *bs,
> int sock, const char *export_name, Error **errp);
> -void nbd_client_session_close(NbdClientSession *client);
> +void nbd_client_session_close(BlockDriverState *bs);
>
> -int nbd_client_session_co_discard(NbdClientSession *client, int64_t sector_num,
> +int nbd_client_session_co_discard(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors);
> -int nbd_client_session_co_flush(NbdClientSession *client);
> -int nbd_client_session_co_writev(NbdClientSession *client, int64_t sector_num,
> +int nbd_client_session_co_flush(BlockDriverState *bs);
> +int nbd_client_session_co_writev(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov);
> -int nbd_client_session_co_readv(NbdClientSession *client, int64_t sector_num,
> +int nbd_client_session_co_readv(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov);
>
> -void nbd_client_session_detach_aio_context(NbdClientSession *client);
> -void nbd_client_session_attach_aio_context(NbdClientSession *client,
> +void nbd_client_session_detach_aio_context(BlockDriverState *bs);
> +void nbd_client_session_attach_aio_context(BlockDriverState *bs,
> AioContext *new_context);
>
> #endif /* NBD_CLIENT_H */
> diff --git a/block/nbd.c b/block/nbd.c
> index 2e20831..f71fba0 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -224,6 +224,12 @@ static void nbd_config(BDRVNBDState *s, QDict *options, char **export,
> }
> }
>
> +NbdClientSession *nbd_get_client_session(BlockDriverState *bs)
> +{
> + BDRVNBDState *s = bs->opaque;
> + return &s->client;
> +}
> +
> static int nbd_establish_connection(BlockDriverState *bs, Error **errp)
> {
> BDRVNBDState *s = bs->opaque;
> @@ -271,7 +277,7 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
> }
>
> /* NBD handshake */
> - result = nbd_client_session_init(&s->client, bs, sock, export, errp);
> + result = nbd_client_session_init(bs, sock, export, errp);
> g_free(export);
> return result;
> }
> @@ -279,35 +285,24 @@ static int nbd_open(BlockDriverState *bs, QDict *options, int flags,
> static int nbd_co_readv(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - return nbd_client_session_co_readv(&s->client, sector_num,
> - nb_sectors, qiov);
> + return nbd_client_session_co_readv(bs, sector_num, nb_sectors, qiov);
> }
>
> static int nbd_co_writev(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors, QEMUIOVector *qiov)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - return nbd_client_session_co_writev(&s->client, sector_num,
> - nb_sectors, qiov);
> + return nbd_client_session_co_writev(bs, sector_num, nb_sectors, qiov);
> }
>
> static int nbd_co_flush(BlockDriverState *bs)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - return nbd_client_session_co_flush(&s->client);
> + return nbd_client_session_co_flush(bs);
> }
>
> static int nbd_co_discard(BlockDriverState *bs, int64_t sector_num,
> int nb_sectors)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - return nbd_client_session_co_discard(&s->client, sector_num,
> - nb_sectors);
> + return nbd_client_session_co_discard(bs, sector_num, nb_sectors);
> }
>
> static void nbd_close(BlockDriverState *bs)
> @@ -315,7 +310,7 @@ static void nbd_close(BlockDriverState *bs)
> BDRVNBDState *s = bs->opaque;
>
> qemu_opts_del(s->socket_opts);
> - nbd_client_session_close(&s->client);
> + nbd_client_session_close(bs);
> }
>
> static int64_t nbd_getlength(BlockDriverState *bs)
> @@ -327,17 +322,13 @@ static int64_t nbd_getlength(BlockDriverState *bs)
>
> static void nbd_detach_aio_context(BlockDriverState *bs)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - nbd_client_session_detach_aio_context(&s->client);
> + nbd_client_session_detach_aio_context(bs);
> }
>
> static void nbd_attach_aio_context(BlockDriverState *bs,
> AioContext *new_context)
> {
> - BDRVNBDState *s = bs->opaque;
> -
> - nbd_client_session_attach_aio_context(&s->client, new_context);
> + nbd_client_session_attach_aio_context(bs, new_context);
> }
>
> static void nbd_refresh_filename(BlockDriverState *bs)
>
next prev parent reply other threads:[~2015-02-03 8:37 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-02-02 21:40 [Qemu-devel] [PATCH 0/3] nbd: Drop BDS backpointer Max Reitz
2015-02-02 21:40 ` [Qemu-devel] [PATCH 1/3] " Max Reitz
2015-02-03 8:37 ` Paolo Bonzini [this message]
2015-02-03 13:54 ` Max Reitz
2015-02-02 21:40 ` [Qemu-devel] [PATCH 2/3] iotests: Add "wait" functionality to _cleanup_qemu Max Reitz
2015-02-03 8:38 ` Paolo Bonzini
2015-02-02 21:40 ` [Qemu-devel] [PATCH 3/3] iotests: Add test for drive-mirror with NBD target Max Reitz
2015-02-03 8:38 ` Paolo Bonzini
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54D088AC.8030607@redhat.com \
--to=pbonzini@redhat.com \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.