From: Kevin Wolf <kwolf@redhat.com>
To: Sergio Lopez <slp@redhat.com>
Cc: Fam Zheng <fam@euphon.net>,
Stefano Stabellini <sstabellini@kernel.org>,
qemu-block@nongnu.org, Paul Durrant <paul@xen.org>,
"Michael S. Tsirkin" <mst@redhat.com>,
qemu-devel@nongnu.org, Max Reitz <mreitz@redhat.com>,
Stefan Hajnoczi <stefanha@redhat.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Anthony Perard <anthony.perard@citrix.com>,
xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 4/4] block: Close block exports in two steps
Date: Tue, 15 Dec 2020 16:34:05 +0100 [thread overview]
Message-ID: <20201215153405.GF8185@merkur.fritz.box> (raw)
In-Reply-To: <20201214170519.223781-5-slp@redhat.com>
Am 14.12.2020 um 18:05 hat Sergio Lopez geschrieben:
> There's a cross-dependency between closing the block exports and
> draining the block layer. The latter needs that we close all export's
> client connections to ensure they won't queue more requests, but the
> exports may have coroutines yielding in the block layer, which implies
> they can't be fully closed until we drain it.
A coroutine that yielded must have some way to be reentered. So I guess
the quesiton becomes why they aren't reentered until drain. We do
process events:
AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
So in theory, anything that would finalise the block export closing
should still execute.
What is the difference that drain makes compared to a simple
AIO_WAIT_WHILE, so that coroutine are reentered during drain, but not
during AIO_WAIT_WHILE?
This is an even more interesting question because the NBD server isn't a
block node nor a BdrvChildClass implementation, so it shouldn't even
notice a drain operation.
Kevin
> To break this cross-dependency, this change adds a "bool wait"
> argument to blk_exp_close_all() and blk_exp_close_all_type(), so
> callers can decide whether they want to wait for the exports to be
> fully quiesced, or just return after requesting them to shut down.
>
> Then, in bdrv_close_all we make two calls, one without waiting to
> close all client connections, and another after draining the block
> layer, this time waiting for the exports to be fully quiesced.
>
> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1900505
> Signed-off-by: Sergio Lopez <slp@redhat.com>
> ---
> block.c | 20 +++++++++++++++++++-
> block/export/export.c | 10 ++++++----
> blockdev-nbd.c | 2 +-
> include/block/export.h | 4 ++--
> qemu-nbd.c | 2 +-
> stubs/blk-exp-close-all.c | 2 +-
> 6 files changed, 30 insertions(+), 10 deletions(-)
>
> diff --git a/block.c b/block.c
> index bc8a66ab6e..41db70ac07 100644
> --- a/block.c
> +++ b/block.c
> @@ -4472,13 +4472,31 @@ static void bdrv_close(BlockDriverState *bs)
> void bdrv_close_all(void)
> {
> assert(job_next(NULL) == NULL);
> - blk_exp_close_all();
> +
> + /*
> + * There's a cross-dependency between closing the block exports and
> + * draining the block layer. The latter needs that we close all export's
> + * client connections to ensure they won't queue more requests, but the
> + * exports may have coroutines yielding in the block layer, which implies
> + * they can't be fully closed until we drain it.
> + *
> + * Make a first call to close all export's client connections, without
> + * waiting for each export to be fully quiesced.
> + */
> + blk_exp_close_all(false);
>
> /* Drop references from requests still in flight, such as canceled block
> * jobs whose AIO context has not been polled yet */
> bdrv_drain_all();
>
> blk_remove_all_bs();
> +
> + /*
> + * Make a second call to shut down the exports, this time waiting for them
> + * to be fully quiesced.
> + */
> + blk_exp_close_all(true);
> +
> blockdev_close_all_bdrv_states();
>
> assert(QTAILQ_EMPTY(&all_bdrv_states));
> diff --git a/block/export/export.c b/block/export/export.c
> index bad6f21b1c..0124ebd9f9 100644
> --- a/block/export/export.c
> +++ b/block/export/export.c
> @@ -280,7 +280,7 @@ static bool blk_exp_has_type(BlockExportType type)
> }
>
> /* type == BLOCK_EXPORT_TYPE__MAX for all types */
> -void blk_exp_close_all_type(BlockExportType type)
> +void blk_exp_close_all_type(BlockExportType type, bool wait)
> {
> BlockExport *exp, *next;
>
> @@ -293,12 +293,14 @@ void blk_exp_close_all_type(BlockExportType type)
> blk_exp_request_shutdown(exp);
> }
>
> - AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
> + if (wait) {
> + AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));
> + }
> }
>
> -void blk_exp_close_all(void)
> +void blk_exp_close_all(bool wait)
> {
> - blk_exp_close_all_type(BLOCK_EXPORT_TYPE__MAX);
> + blk_exp_close_all_type(BLOCK_EXPORT_TYPE__MAX, wait);
> }
>
> void qmp_block_export_add(BlockExportOptions *export, Error **errp)
> diff --git a/blockdev-nbd.c b/blockdev-nbd.c
> index d8443d235b..d71d4da7c2 100644
> --- a/blockdev-nbd.c
> +++ b/blockdev-nbd.c
> @@ -266,7 +266,7 @@ void qmp_nbd_server_stop(Error **errp)
> return;
> }
>
> - blk_exp_close_all_type(BLOCK_EXPORT_TYPE_NBD);
> + blk_exp_close_all_type(BLOCK_EXPORT_TYPE_NBD, true);
>
> nbd_server_free(nbd_server);
> nbd_server = NULL;
> diff --git a/include/block/export.h b/include/block/export.h
> index 7feb02e10d..71c25928ce 100644
> --- a/include/block/export.h
> +++ b/include/block/export.h
> @@ -83,7 +83,7 @@ BlockExport *blk_exp_find(const char *id);
> void blk_exp_ref(BlockExport *exp);
> void blk_exp_unref(BlockExport *exp);
> void blk_exp_request_shutdown(BlockExport *exp);
> -void blk_exp_close_all(void);
> -void blk_exp_close_all_type(BlockExportType type);
> +void blk_exp_close_all(bool wait);
> +void blk_exp_close_all_type(BlockExportType type, bool wait);
>
> #endif
> diff --git a/qemu-nbd.c b/qemu-nbd.c
> index a7075c5419..928f4466f6 100644
> --- a/qemu-nbd.c
> +++ b/qemu-nbd.c
> @@ -1122,7 +1122,7 @@ int main(int argc, char **argv)
> do {
> main_loop_wait(false);
> if (state == TERMINATE) {
> - blk_exp_close_all();
> + blk_exp_close_all(true);
> state = TERMINATED;
> }
> } while (state != TERMINATED);
> diff --git a/stubs/blk-exp-close-all.c b/stubs/blk-exp-close-all.c
> index 1c71316763..ecd0ce611f 100644
> --- a/stubs/blk-exp-close-all.c
> +++ b/stubs/blk-exp-close-all.c
> @@ -2,6 +2,6 @@
> #include "block/export.h"
>
> /* Only used in programs that support block exports (libblockdev.fa) */
> -void blk_exp_close_all(void)
> +void blk_exp_close_all(bool wait)
> {
> }
> --
> 2.26.2
>
next prev parent reply other threads:[~2020-12-15 15:40 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-12-14 17:05 [PATCH v2 0/4] nbd/server: Quiesce coroutines on context switch Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 1/4] block: Honor blk_set_aio_context() context requirements Sergio Lopez
2020-12-15 11:58 ` Kevin Wolf
2020-12-14 17:05 ` [PATCH v2 2/4] block: Avoid processing BDS twice in bdrv_set_aio_context_ignore() Sergio Lopez
2020-12-15 12:12 ` Kevin Wolf
2020-12-15 13:15 ` Sergio Lopez
2020-12-15 15:01 ` Kevin Wolf
2020-12-15 17:23 ` Sergio Lopez
2020-12-16 12:35 ` Kevin Wolf
2020-12-16 14:55 ` Sergio Lopez
2020-12-16 18:31 ` Kevin Wolf
2020-12-17 9:37 ` Sergio Lopez
2020-12-17 10:58 ` Kevin Wolf
2020-12-17 12:50 ` Vladimir Sementsov-Ogievskiy
2020-12-17 13:06 ` Kevin Wolf
2020-12-17 13:27 ` Sergio Lopez
2020-12-17 14:01 ` Vladimir Sementsov-Ogievskiy
2020-12-17 13:09 ` Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 3/4] nbd/server: Quiesce coroutines on context switch Sergio Lopez
2020-12-14 17:05 ` [PATCH v2 4/4] block: Close block exports in two steps Sergio Lopez
2020-12-15 15:34 ` Kevin Wolf [this message]
2020-12-15 17:26 ` Sergio Lopez
2020-12-21 17:07 ` Sergio Lopez
2021-01-20 20:49 ` [PATCH v2 0/4] nbd/server: Quiesce coroutines on context switch Eric Blake
2021-01-21 5:57 ` Sergio Lopez
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=20201215153405.GF8185@merkur.fritz.box \
--to=kwolf@redhat.com \
--cc=anthony.perard@citrix.com \
--cc=fam@euphon.net \
--cc=mreitz@redhat.com \
--cc=mst@redhat.com \
--cc=paul@xen.org \
--cc=pbonzini@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=slp@redhat.com \
--cc=sstabellini@kernel.org \
--cc=stefanha@redhat.com \
--cc=xen-devel@lists.xenproject.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).