The following changes since commit ac5f7bf8e208cd7893dbb1a9520559e569a4677c:

Merge tag 'migration-20230424-pull-request' of https://gitlab.com/juan.quintela/qemu into staging (2023-04-24 15:00:39 +0100)

https://repo.or.cz/qemu/kevin.git tags/for-upstream

for you to fetch changes up to 8c1e8fb2e7fc2cbeb57703e143965a4cd3ad301a:

block/monitor: Fix crash when executing HMP commit (2023-04-25 15:11:57 +0200)

Block layer patches

- Protect BlockBackend.queued_requests with its own lock

- Switch to AIO_WAIT_WHILE_UNLOCKED() where possible

- AioContext removal: LinuxAioState/LuringState/ThreadPool

- Add more coroutine_fn annotations, use bdrv/blk_co_*

- Fix crash when execute hmp_commit

Emanuele Giuseppe Esposito (4):

linux-aio: use LinuxAioState from the running thread

io_uring: use LuringState from the running thread

thread-pool: use ThreadPool from the running thread

thread-pool: avoid passing the pool parameter every time

Paolo Bonzini (9):

vvfat: mark various functions as coroutine_fn

blkdebug: add missing coroutine_fn annotation

mirror: make mirror_flush a coroutine_fn, do not use co_wrappers

nbd: mark more coroutine_fns, do not use co_wrappers

9pfs: mark more coroutine_fns

qemu-pr-helper: mark more coroutine_fns

tests: mark more coroutine_fns

qcow2: mark various functions as coroutine_fn and GRAPH_RDLOCK

vmdk: make vmdk_is_cid_valid a coroutine_fn

Stefan Hajnoczi (10):

block: make BlockBackend->quiesce_counter atomic

block: make BlockBackend->disable_request_queuing atomic

block: protect BlockBackend->queued_requests with a lock

block: don't acquire AioContext lock in bdrv_drain_all()

block: convert blk_exp_close_all_type() to AIO_WAIT_WHILE_UNLOCKED()

block: convert bdrv_graph_wrlock() to AIO_WAIT_WHILE_UNLOCKED()

block: convert bdrv_drain_all_begin() to AIO_WAIT_WHILE_UNLOCKED()

hmp: convert handle_hmp_command() to AIO_WAIT_WHILE_UNLOCKED()

monitor: convert monitor_cleanup() to AIO_WAIT_WHILE_UNLOCKED()

block: add missing coroutine_fn to bdrv_sum_allocated_file_size()

Wang Liang (1):

block/monitor: Fix crash when executing HMP commit

Wilfred Mallawa (1):

include/block: fixup typos

block/qcow2.h | 15 +++++-----

hw/9pfs/9p.h | 4 +--

include/block/aio-wait.h | 2 +-

include/block/aio.h | 8 ------

include/block/block_int-common.h | 2 +-

include/block/raw-aio.h | 33 +++++++++++++++-------

include/block/thread-pool.h | 15 ++++++----

include/sysemu/block-backend-io.h | 5 ++++

backends/tpm/tpm_backend.c | 4 +--

block.c | 2 +-

block/blkdebug.c | 4 +--

block/block-backend.c | 45 ++++++++++++++++++------------

block/export/export.c | 2 +-

block/file-posix.c | 45 ++++++++++++------------------

block/file-win32.c | 4 +--

block/graph-lock.c | 2 +-

block/io.c | 2 +-

block/io_uring.c | 23 ++++++++++------

block/linux-aio.c | 29 ++++++++++++--------

block/mirror.c | 4 +--

block/monitor/block-hmp-cmds.c | 10 ++++---

block/qcow2-bitmap.c | 2 +-

block/qcow2-cluster.c | 21 ++++++++------

block/qcow2-refcount.c | 8 +++---

block/qcow2-snapshot.c | 25 +++++++++--------

block/qcow2-threads.c | 3 +-

block/qcow2.c | 27 +++++++++---------

block/vmdk.c | 2 +-

block/vvfat.c | 58 ++++++++++++++++++++-------------------

hw/9pfs/codir.c | 6 ++--

hw/9pfs/coth.c | 3 +-

hw/ppc/spapr_nvdimm.c | 6 ++--

hw/virtio/virtio-pmem.c | 3 +-

monitor/hmp.c | 2 +-

monitor/monitor.c | 4 +--

nbd/server.c | 48 ++++++++++++++++----------------

scsi/pr-manager.c | 3 +-

scsi/qemu-pr-helper.c | 25 ++++++++---------

tests/unit/test-thread-pool.c | 14 ++++------

util/thread-pool.c | 25 ++++++++---------

40 files changed, 283 insertions(+), 262 deletions(-)

From: Stefan Hajnoczi <stefanha@redhat.com>

The main loop thread increments/decrements BlockBackend->quiesce_counter

when drained sections begin/end. The counter is read in the I/O code

path. Therefore this field is used to communicate between threads

without a lock.

Acquire/release are not necessary because the BlockBackend->in_flight

counter already uses sequentially consistent accesses and running I/O

requests hold that counter when blk_wait_while_drained() is called.

qatomic_read() can be used.

Use qatomic_fetch_inc()/qatomic_fetch_dec() for modifications even

though sequentially consistent atomic accesses are not strictly required

here. They are, however, nicer to read than multiple calls to

qatomic_read() and qatomic_set(). Since beginning and ending drain is

not a hot path the extra cost doesn't matter.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20230307210427.269214-2-stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

block/block-backend.c | 14 +++++++-------

1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ struct BlockBackend {

NotifierList remove_bs_notifiers, insert_bs_notifiers;

QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;

- int quiesce_counter;

+ int quiesce_counter; /* atomic: written under BQL, read by other threads */

CoQueue queued_requests;

bool disable_request_queuing;

@@ -XXX,XX +XXX,XX @@ void blk_set_dev_ops(BlockBackend *blk, const BlockDevOps *ops,

blk->dev_opaque = opaque;

/* Are we currently quiesced? Should we enforce this right now? */

- if (blk->quiesce_counter && ops && ops->drained_begin) {

+ if (qatomic_read(&blk->quiesce_counter) && ops && ops->drained_begin) {

ops->drained_begin(opaque);

}

}

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)

{

assert(blk->in_flight > 0);

- if (blk->quiesce_counter && !blk->disable_request_queuing) {

+ if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {

blk_dec_in_flight(blk);

qemu_co_queue_wait(&blk->queued_requests, NULL);

blk_inc_in_flight(blk);

@@ -XXX,XX +XXX,XX @@ static void blk_root_drained_begin(BdrvChild *child)

BlockBackend *blk = child->opaque;

ThrottleGroupMember *tgm = &blk->public.throttle_group_member;

- if (++blk->quiesce_counter == 1) {

+ if (qatomic_fetch_inc(&blk->quiesce_counter) == 0) {

if (blk->dev_ops && blk->dev_ops->drained_begin) {

blk->dev_ops->drained_begin(blk->dev_opaque);

}

@@ -XXX,XX +XXX,XX @@ static bool blk_root_drained_poll(BdrvChild *child)

{

BlockBackend *blk = child->opaque;

bool busy = false;

- assert(blk->quiesce_counter);

+ assert(qatomic_read(&blk->quiesce_counter));

if (blk->dev_ops && blk->dev_ops->drained_poll) {

busy = blk->dev_ops->drained_poll(blk->dev_opaque);

@@ -XXX,XX +XXX,XX @@ static bool blk_root_drained_poll(BdrvChild *child)

static void blk_root_drained_end(BdrvChild *child)

{

BlockBackend *blk = child->opaque;

- assert(blk->quiesce_counter);

+ assert(qatomic_read(&blk->quiesce_counter));

assert(blk->public.throttle_group_member.io_limits_disabled);

qatomic_dec(&blk->public.throttle_group_member.io_limits_disabled);

- if (--blk->quiesce_counter == 0) {

+ if (qatomic_fetch_dec(&blk->quiesce_counter) == 1) {

if (blk->dev_ops && blk->dev_ops->drained_end) {

blk->dev_ops->drained_end(blk->dev_opaque);

}

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

This field is accessed by multiple threads without a lock. Use explicit

qatomic_read()/qatomic_set() calls. There is no need for acquire/release

because blk_set_disable_request_queuing() doesn't provide any

guarantees (it helps that it's used at BlockBackend creation time and

not when there is I/O in flight).

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Message-Id: <20230307210427.269214-3-stefanha@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

block/block-backend.c | 7 ++++---

1 file changed, 4 insertions(+), 3 deletions(-)

@@ -XXX,XX +XXX,XX @@ struct BlockBackend {

int quiesce_counter; /* atomic: written under BQL, read by other threads */

CoQueue queued_requests;

- bool disable_request_queuing;

+ bool disable_request_queuing; /* atomic */

VMChangeStateEntry *vmsh;

bool force_allow_inactivate;

@@ -XXX,XX +XXX,XX @@ void blk_set_allow_aio_context_change(BlockBackend *blk, bool allow)

void blk_set_disable_request_queuing(BlockBackend *blk, bool disable)

{

IO_CODE();

- blk->disable_request_queuing = disable;

+ qatomic_set(&blk->disable_request_queuing, disable);

static int coroutine_fn GRAPH_RDLOCK

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)

assert(blk->in_flight > 0);

- if (qatomic_read(&blk->quiesce_counter) && !blk->disable_request_queuing) {

+ if (qatomic_read(&blk->quiesce_counter) &&

+ !qatomic_read(&blk->disable_request_queuing)) {

blk_dec_in_flight(blk);

qemu_co_queue_wait(&blk->queued_requests, NULL);

blk_inc_in_flight(blk);

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

The CoQueue API offers thread-safety via the lock argument that

qemu_co_queue_wait() and qemu_co_enter_next() take. BlockBackend

currently does not make use of the lock argument. This means that

multiple threads submitting I/O requests can corrupt the CoQueue's

QSIMPLEQ.

Add a QemuMutex and pass it to CoQueue APIs so that the queue is

protected. While we're at it, also assert that the queue is empty when

the BlockBackend is deleted.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Reviewed-by: Hanna Czenczek <hreitz@redhat.com>

Message-Id: <20230307210427.269214-4-stefanha@redhat.com>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

block/block-backend.c | 18 ++++++++++++++++--

1 file changed, 16 insertions(+), 2 deletions(-)

@@ -XXX,XX +XXX,XX @@ struct BlockBackend {

QLIST_HEAD(, BlockBackendAioNotifier) aio_notifiers;

int quiesce_counter; /* atomic: written under BQL, read by other threads */

+ QemuMutex queued_requests_lock; /* protects queued_requests */

CoQueue queued_requests;

bool disable_request_queuing; /* atomic */

@@ -XXX,XX +XXX,XX @@ BlockBackend *blk_new(AioContext *ctx, uint64_t perm, uint64_t shared_perm)

block_acct_init(&blk->stats);

+ qemu_mutex_init(&blk->queued_requests_lock);

qemu_co_queue_init(&blk->queued_requests);

notifier_list_init(&blk->remove_bs_notifiers);

notifier_list_init(&blk->insert_bs_notifiers);

@@ -XXX,XX +XXX,XX @@ static void blk_delete(BlockBackend *blk)

assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));

assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));

assert(QLIST_EMPTY(&blk->aio_notifiers));

+ assert(qemu_co_queue_empty(&blk->queued_requests));

+ qemu_mutex_destroy(&blk->queued_requests_lock);

QTAILQ_REMOVE(&block_backends, blk, link);

drive_info_del(blk->legacy_dinfo);

block_acct_cleanup(&blk->stats);

@@ -XXX,XX +XXX,XX @@ static void coroutine_fn blk_wait_while_drained(BlockBackend *blk)

if (qatomic_read(&blk->quiesce_counter) &&

!qatomic_read(&blk->disable_request_queuing)) {

+ /*

+ * Take lock before decrementing in flight counter so main loop thread

+ * waits for us to enqueue ourselves before it can leave the drained

+ * section.

+ */

+ qemu_mutex_lock(&blk->queued_requests_lock);

blk_dec_in_flight(blk);

- qemu_co_queue_wait(&blk->queued_requests, NULL);

+ qemu_co_queue_wait(&blk->queued_requests, &blk->queued_requests_lock);

blk_inc_in_flight(blk);

+ qemu_mutex_unlock(&blk->queued_requests_lock);

@@ -XXX,XX +XXX,XX @@ static void blk_root_drained_end(BdrvChild *child)

if (blk->dev_ops && blk->dev_ops->drained_end) {

blk->dev_ops->drained_end(blk->dev_opaque);

}

- while (qemu_co_enter_next(&blk->queued_requests, NULL)) {

+ qemu_mutex_lock(&blk->queued_requests_lock);

+ while (qemu_co_enter_next(&blk->queued_requests,

+ &blk->queued_requests_lock)) {

/* Resume all queued requests */

}

+ qemu_mutex_unlock(&blk->queued_requests_lock);

}

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

There is no change in behavior. Switch to AIO_WAIT_WHILE_UNLOCKED()

instead of AIO_WAIT_WHILE() to document that this code has already been

audited and converted. The AioContext argument is already NULL so

aio_context_release() is never called anyway.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20230309190855.414275-3-stefanha@redhat.com>

Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

block/export/export.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/export.c b/block/export/export.c

--- a/block/export/export.c

+++ b/block/export/export.c

@@ -XXX,XX +XXX,XX @@ void blk_exp_close_all_type(BlockExportType type)

blk_exp_request_shutdown(exp);

}

- AIO_WAIT_WHILE(NULL, blk_exp_has_type(type));

+ AIO_WAIT_WHILE_UNLOCKED(NULL, blk_exp_has_type(type));

void blk_exp_close_all(void)

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

The following conversion is safe and does not change behavior:

GLOBAL_STATE_CODE();

...

- AIO_WAIT_WHILE(qemu_get_aio_context(), ...);

+ AIO_WAIT_WHILE_UNLOCKED(NULL, ...);

Since we're in GLOBAL_STATE_CODE(), qemu_get_aio_context() is our home

thread's AioContext. Thus AIO_WAIT_WHILE() does not unlock the

AioContext:

if (ctx_ && in_aio_context_home_thread(ctx_)) { \

while ((cond)) { \

aio_poll(ctx_, true); \

waited_ = true; \

} \

And that means AIO_WAIT_WHILE_UNLOCKED(NULL, ...) can be substituted.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20230309190855.414275-4-stefanha@redhat.com>

Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

block/graph-lock.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/graph-lock.c b/block/graph-lock.c

--- a/block/graph-lock.c

+++ b/block/graph-lock.c

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(void)

* reader lock.

*/

qatomic_set(&has_writer, 0);

- AIO_WAIT_WHILE(qemu_get_aio_context(), reader_count() >= 1);

+ AIO_WAIT_WHILE_UNLOCKED(NULL, reader_count() >= 1);

qatomic_set(&has_writer, 1);

/*

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

Since the AioContext argument was already NULL, AIO_WAIT_WHILE() was

never going to unlock the AioContext. Therefore it is possible to

replace AIO_WAIT_WHILE() with AIO_WAIT_WHILE_UNLOCKED().

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Tested-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Message-Id: <20230309190855.414275-5-stefanha@redhat.com>

Reviewed-by: Wilfred Mallawa <wilfred.mallawa@wdc.com>

block/io.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

@@ -XXX,XX +XXX,XX @@ void bdrv_drain_all_begin(void)

bdrv_drain_all_begin_nopoll();

/* Now poll the in-flight requests */

- AIO_WAIT_WHILE(NULL, bdrv_drain_all_poll());

+ AIO_WAIT_WHILE_UNLOCKED(NULL, bdrv_drain_all_poll());

while ((bs = bdrv_next_all_states(bs))) {

bdrv_drain_assert_idle(bs);

2.40.0

From: Stefan Hajnoczi <stefanha@redhat.com>

The HMP monitor runs in the main loop thread. Calling

AIO_WAIT_WHILE(qemu_get_aio_context(), ...) from the main loop thread is

equivalent to AIO_WAIT_WHILE_UNLOCKED(NULL, ...) because neither unlocks

the AioContext and the latter's assertion that we're in the main loop

succeeds.

monitor/hmp.c | 2 +-

1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/monitor/hmp.c b/monitor/hmp.c

--- a/monitor/hmp.c

+++ b/monitor/hmp.c

@@ -XXX,XX +XXX,XX @@ void handle_hmp_command(MonitorHMP *mon, const char *cmdline)

Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, &data);

monitor_set_cur(co, &mon->common);

aio_co_enter(qemu_get_aio_context(), co);

- AIO_WAIT_WHILE(qemu_get_aio_context(), !data.done);

+ AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);