This field has been unused since commit 72373e40fbc ('block:

bdrv_reopen_multiple: refresh permissions on updated graph').

Remove it.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-2-kwolf@redhat.com>

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

---

block.c | 1 -

1 file changed, 1 deletion(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static int bdrv_fill_options(QDict **options, const char *filename,

typedef struct BlockReopenQueueEntry {

bool prepared;

- bool perms_checked;

BDRVReopenState state;

QTAILQ_ENTRY(BlockReopenQueueEntry) entry;

} BlockReopenQueueEntry;

--

2.41.0

It's essentially the same code in preallocate_check_perm() and

preallocate_close(), except that the latter ignores errors.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-3-kwolf@redhat.com>

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

---

block/preallocate.c | 48 +++++++++++++++++++++------------------------

1 file changed, 22 insertions(+), 26 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block/preallocate.c

+++ b/block/preallocate.c

@@ -XXX,XX +XXX,XX @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,

return 0;

}

-static void preallocate_close(BlockDriverState *bs)

+static int preallocate_truncate_to_real_size(BlockDriverState *bs, Error **errp)

{

- int ret;

BDRVPreallocateState *s = bs->opaque;

-

- if (s->data_end < 0) {

- return;

- }

+ int ret;

if (s->file_end < 0) {

s->file_end = bdrv_getlength(bs->file->bs);

if (s->file_end < 0) {

- return;

+ error_setg_errno(errp, -s->file_end, "Failed to get file length");

+ return s->file_end;

}

}

if (s->data_end < s->file_end) {

ret = bdrv_truncate(bs->file, s->data_end, true, PREALLOC_MODE_OFF, 0,

NULL);

- s->file_end = ret < 0 ? ret : s->data_end;

+ if (ret < 0) {

+ error_setg_errno(errp, -ret, "Failed to drop preallocation");

+ s->file_end = ret;

+ return ret;

+ }

+ s->file_end = s->data_end;

+ }

+

+ return 0;

+}

+

+static void preallocate_close(BlockDriverState *bs)

+{

+ BDRVPreallocateState *s = bs->opaque;

+

+ if (s->data_end >= 0) {

+ preallocate_truncate_to_real_size(bs, NULL);

}

}

@@ -XXX,XX +XXX,XX @@ static int preallocate_check_perm(BlockDriverState *bs,

* We should truncate in check_perm, as in set_perm bs->file->perm will

* be already changed, and we should not violate it.

*/

- if (s->file_end < 0) {

- s->file_end = bdrv_getlength(bs->file->bs);

- if (s->file_end < 0) {

- error_setg(errp, "Failed to get file length");

- return s->file_end;

- }

- }

-

- if (s->data_end < s->file_end) {

- int ret = bdrv_truncate(bs->file, s->data_end, true,

- PREALLOC_MODE_OFF, 0, NULL);

- if (ret < 0) {

- error_setg(errp, "Failed to drop preallocation");

- s->file_end = ret;

- return ret;

- }

- s->file_end = s->data_end;

- }

+ return preallocate_truncate_to_real_size(bs, errp);

}

return 0;

--

2.41.0

The documentation for bdrv_append() says that the caller must hold the

AioContext lock for bs_top. Change all callers to actually adhere to the

contract.

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

Reviewed-by: Eric Blake <eblake@redhat.com>

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-5-kwolf@redhat.com>

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

---

tests/unit/test-bdrv-drain.c | 3 +++

tests/unit/test-bdrv-graph-mod.c | 6 ++++++

tests/unit/test-block-iothread.c | 3 +++

3 files changed, 12 insertions(+)

diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/unit/test-bdrv-drain.c

+++ b/tests/unit/test-bdrv-drain.c

@@ -XXX,XX +XXX,XX @@ static void test_append_to_drained(void)

g_assert_cmpint(base_s->drain_count, ==, 1);

g_assert_cmpint(base->in_flight, ==, 0);

+ aio_context_acquire(qemu_get_aio_context());

bdrv_append(overlay, base, &error_abort);

+ aio_context_release(qemu_get_aio_context());

+

g_assert_cmpint(base->in_flight, ==, 0);

g_assert_cmpint(overlay->in_flight, ==, 0);

diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/unit/test-bdrv-graph-mod.c

+++ b/tests/unit/test-bdrv-graph-mod.c

@@ -XXX,XX +XXX,XX @@ static void test_update_perm_tree(void)

bdrv_attach_child(filter, bs, "child", &child_of_bds,

BDRV_CHILD_DATA, &error_abort);

+ aio_context_acquire(qemu_get_aio_context());

ret = bdrv_append(filter, bs, NULL);

g_assert_cmpint(ret, <, 0);

+ aio_context_release(qemu_get_aio_context());

bdrv_unref(filter);

blk_unref(root);

@@ -XXX,XX +XXX,XX @@ static void test_should_update_child(void)

g_assert(target->backing->bs == bs);

bdrv_attach_child(filter, target, "target", &child_of_bds,

BDRV_CHILD_DATA, &error_abort);

+ aio_context_acquire(qemu_get_aio_context());

bdrv_append(filter, bs, &error_abort);

+ aio_context_release(qemu_get_aio_context());

g_assert(target->backing->bs == bs);

bdrv_unref(filter);

@@ -XXX,XX +XXX,XX @@ static void test_append_greedy_filter(void)

BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,

&error_abort);

+ aio_context_acquire(qemu_get_aio_context());

bdrv_append(fl, base, &error_abort);

+ aio_context_release(qemu_get_aio_context());

bdrv_unref(fl);

bdrv_unref(top);

}

diff --git a/tests/unit/test-block-iothread.c b/tests/unit/test-block-iothread.c

index XXXXXXX..XXXXXXX 100644

--- a/tests/unit/test-block-iothread.c

+++ b/tests/unit/test-block-iothread.c

@@ -XXX,XX +XXX,XX @@ static void test_propagate_mirror(void)

&error_abort);

/* Start a mirror job */

+ aio_context_acquire(main_ctx);

mirror_start("job0", src, target, NULL, JOB_DEFAULT, 0, 0, 0,

MIRROR_SYNC_MODE_NONE, MIRROR_OPEN_BACKING_CHAIN, false,

BLOCKDEV_ON_ERROR_REPORT, BLOCKDEV_ON_ERROR_REPORT,

false, "filter_node", MIRROR_COPY_MODE_BACKGROUND,

&error_abort);

+ aio_context_release(main_ctx);

+

WITH_JOB_LOCK_GUARD() {

job = job_get_locked("job0");

}

--

2.41.0

bdrv_unref() is called by a lot of places that need to hold the graph

lock (it naturally happens in the context of operations that change the

graph). However, bdrv_unref() takes the graph writer lock internally, so

it can't actually be called while already holding a graph lock without

causing a deadlock.

bdrv_unref() also can't just become GRAPH_WRLOCK because it drains the

node before closing it, and draining requires that the graph is

unlocked.

The solution is to defer deleting the node until we don't hold the lock

any more and draining is possible again.

Note that keeping images open for longer than necessary can create

problems, too: You can't open an image again before it is really closed

(if image locking didn't prevent it, it would cause corruption).

Reopening an image immediately happens at least during bdrv_open() and

bdrv_co_create().

In order to solve this problem, make sure to run the deferred unref in

bdrv_graph_wrunlock(), i.e. the first possible place where we can drain

again. This is also why bdrv_schedule_unref() is marked GRAPH_WRLOCK.

The output of iotest 051 is updated because the additional polling

changes the order of HMP output, resulting in a new "(qemu)" prompt in

the test output that was previously on a separate line and filtered out.

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

Message-ID: <20230911094620.45040-6-kwolf@redhat.com>

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

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

---

include/block/block-global-state.h | 1 +

block.c | 17 +++++++++++++++++

block/graph-lock.c | 26 +++++++++++++++++++-------

tests/qemu-iotests/051.pc.out | 6 +++---

4 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block-global-state.h

+++ b/include/block/block-global-state.h

@@ -XXX,XX +XXX,XX @@ void bdrv_img_create(const char *filename, const char *fmt,

void bdrv_ref(BlockDriverState *bs);

void no_coroutine_fn bdrv_unref(BlockDriverState *bs);

void coroutine_fn no_co_wrapper bdrv_co_unref(BlockDriverState *bs);

+void GRAPH_WRLOCK bdrv_schedule_unref(BlockDriverState *bs);

void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child);

BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,

BlockDriverState *child_bs,

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ void bdrv_unref(BlockDriverState *bs)

}

}

+/*

+ * Release a BlockDriverState reference while holding the graph write lock.

+ *

+ * Calling bdrv_unref() directly is forbidden while holding the graph lock

+ * because bdrv_close() both involves polling and taking the graph lock

+ * internally. bdrv_schedule_unref() instead delays decreasing the refcount and

+ * possibly closing @bs until the graph lock is released.

+ */

+void bdrv_schedule_unref(BlockDriverState *bs)

+{

+ if (!bs) {

+ return;

+ }

+ aio_bh_schedule_oneshot(qemu_get_aio_context(),

+ (QEMUBHFunc *) bdrv_unref, bs);

+}

+

struct BdrvOpBlocker {

Error *reason;

QLIST_ENTRY(BdrvOpBlocker) list;

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

index XXXXXXX..XXXXXXX 100644

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

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

@@ -XXX,XX +XXX,XX @@ void bdrv_graph_wrlock(BlockDriverState *bs)

void bdrv_graph_wrunlock(void)

{

GLOBAL_STATE_CODE();

- QEMU_LOCK_GUARD(&aio_context_list_lock);

assert(qatomic_read(&has_writer));

+ WITH_QEMU_LOCK_GUARD(&aio_context_list_lock) {

+ /*

+ * No need for memory barriers, this works in pair with

+ * the slow path of rdlock() and both take the lock.

+ */

+ qatomic_store_release(&has_writer, 0);

+

+ /* Wake up all coroutines that are waiting to read the graph */

+ qemu_co_enter_all(&reader_queue, &aio_context_list_lock);

+ }

+

/*

- * No need for memory barriers, this works in pair with

- * the slow path of rdlock() and both take the lock.

+ * Run any BHs that were scheduled during the wrlock section and that

+ * callers might expect to have finished (in particular, this is important

+ * for bdrv_schedule_unref()).

+ *

+ * Do this only after restarting coroutines so that nested event loops in

+ * BHs don't deadlock if their condition relies on the coroutine making

+ * progress.

*/

- qatomic_store_release(&has_writer, 0);

-

- /* Wake up all coroutine that are waiting to read the graph */

- qemu_co_enter_all(&reader_queue, &aio_context_list_lock);

+ aio_bh_poll(qemu_get_aio_context());

}

void coroutine_fn bdrv_graph_co_rdlock(void)

diff --git a/tests/qemu-iotests/051.pc.out b/tests/qemu-iotests/051.pc.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/051.pc.out

+++ b/tests/qemu-iotests/051.pc.out

@@ -XXX,XX +XXX,XX @@ QEMU_PROG: -device scsi-hd,drive=disk: Device needs media, but drive is empty

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device ide-hd,drive=disk,share-rw=on

QEMU X.Y.Z monitor - type 'help' for more information

-QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend

+(qemu) QEMU_PROG: -device ide-hd,drive=disk,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,share-rw=on

QEMU X.Y.Z monitor - type 'help' for more information

-QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend

+(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device lsi53c895a,id=lsi0 -device scsi-hd,bus=lsi0.0,drive=disk,share-rw=on

QEMU X.Y.Z monitor - type 'help' for more information

@@ -XXX,XX +XXX,XX @@ QEMU X.Y.Z monitor - type 'help' for more information

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on

QEMU X.Y.Z monitor - type 'help' for more information

-QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend

+(qemu) QEMU_PROG: -device virtio-blk-pci,drive=disk,iothread=thread0,share-rw=on: Cannot change iothread of active block backend

Testing: -drive file=TEST_DIR/t.qcow2,if=none,node-name=disk -object iothread,id=thread0 -device virtio-scsi,iothread=thread0,id=virtio-scsi0 -device scsi-hd,bus=virtio-scsi0.0,drive=disk,share-rw=on -device virtio-scsi,id=virtio-scsi1,iothread=thread0 -device scsi-hd,bus=virtio-scsi1.0,drive=disk,share-rw=on

QEMU X.Y.Z monitor - type 'help' for more information

--

2.41.0

Add a new wrapper type for GRAPH_WRLOCK functions that should be called

from coroutine context.

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

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-7-kwolf@redhat.com>

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

---

include/block/block-common.h | 4 ++++

scripts/block-coroutine-wrapper.py | 11 +++++++++++

2 files changed, 15 insertions(+)

diff --git a/include/block/block-common.h b/include/block/block-common.h

index XXXXXXX..XXXXXXX 100644

--- a/include/block/block-common.h

+++ b/include/block/block-common.h

@@ -XXX,XX +XXX,XX @@

* function. The coroutine yields after scheduling the BH and is reentered when

* the wrapped function returns.

*

+ * A no_co_wrapper_bdrv_wrlock function is a no_co_wrapper function that

+ * automatically takes the graph wrlock when calling the wrapped function.

+ *

* If the first parameter of the function is a BlockDriverState, BdrvChild or

* BlockBackend pointer, the AioContext lock for it is taken in the wrapper.

*/

#define no_co_wrapper

+#define no_co_wrapper_bdrv_wrlock

#include "block/blockjob.h"

diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py

index XXXXXXX..XXXXXXX 100644

--- a/scripts/block-coroutine-wrapper.py

+++ b/scripts/block-coroutine-wrapper.py

@@ -XXX,XX +XXX,XX @@ def __init__(self, wrapper_type: str, return_type: str, name: str,

self.args = [ParamDecl(arg.strip()) for arg in args.split(',')]

self.create_only_co = 'mixed' not in variant

self.graph_rdlock = 'bdrv_rdlock' in variant

+ self.graph_wrlock = 'bdrv_wrlock' in variant

self.wrapper_type = wrapper_type

if wrapper_type == 'co':

+ if self.graph_wrlock:

+ raise ValueError(f"co function can't be wrlock: {self.name}")

subsystem, subname = self.name.split('_', 1)

self.target_name = f'{subsystem}_co_{subname}'

else:

@@ -XXX,XX +XXX,XX @@ def gen_no_co_wrapper(func: FuncDecl) -> str:

name = func.target_name

struct_name = func.struct_name

+ graph_lock=''

+ graph_unlock=''

+ if func.graph_wrlock:

+ graph_lock=' bdrv_graph_wrlock(NULL);'

+ graph_unlock=' bdrv_graph_wrunlock();'

+

return f"""\

/*

* Wrappers for {name}

@@ -XXX,XX +XXX,XX @@ def gen_no_co_wrapper(func: FuncDecl) -> str:

{struct_name} *s = opaque;

AioContext *ctx = {func.gen_ctx('s->')};

+{graph_lock}

aio_context_acquire(ctx);

{func.get_result}{name}({ func.gen_list('s->{name}') });

aio_context_release(ctx);

+{graph_unlock}

aio_co_wake(s->co);

}}

--

2.41.0

Don't assume specific parameter names like 'bs' or 'blk' in the

generated code, but use the actual name.

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

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-8-kwolf@redhat.com>

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

---

scripts/block-coroutine-wrapper.py | 7 ++++---

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

diff --git a/scripts/block-coroutine-wrapper.py b/scripts/block-coroutine-wrapper.py

index XXXXXXX..XXXXXXX 100644

--- a/scripts/block-coroutine-wrapper.py

+++ b/scripts/block-coroutine-wrapper.py

@@ -XXX,XX +XXX,XX @@ def __init__(self, wrapper_type: str, return_type: str, name: str,

def gen_ctx(self, prefix: str = '') -> str:

t = self.args[0].type

+ name = self.args[0].name

if t == 'BlockDriverState *':

- return f'bdrv_get_aio_context({prefix}bs)'

+ return f'bdrv_get_aio_context({prefix}{name})'

elif t == 'BdrvChild *':

- return f'bdrv_get_aio_context({prefix}child->bs)'

+ return f'bdrv_get_aio_context({prefix}{name}->bs)'

elif t == 'BlockBackend *':

- return f'blk_get_aio_context({prefix}blk)'

+ return f'blk_get_aio_context({prefix}{name})'

else:

return 'qemu_get_aio_context()'

--

2.41.0

Instead of taking the writer lock internally, require callers to already

hold it when calling bdrv_replace_child_noperm(). These callers will

typically already hold the graph lock once the locking work is

completed, which means that they can't call functions that take it

internally.

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

Reviewed-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

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

Message-ID: <20230911094620.45040-9-kwolf@redhat.com>

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

---

block.c | 26 +++++++++++++++++++-------

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

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static BlockDriverState *bdrv_open_inherit(const char *filename,

static bool bdrv_recurse_has_child(BlockDriverState *bs,

BlockDriverState *child);

-static void bdrv_replace_child_noperm(BdrvChild *child,

- BlockDriverState *new_bs);

+static void GRAPH_WRLOCK

+bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs);

+

static void bdrv_remove_child(BdrvChild *child, Transaction *tran);

static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)

BlockDriverState *new_bs = s->child->bs;

GLOBAL_STATE_CODE();

+ bdrv_graph_wrlock(s->old_bs);

+

/* old_bs reference is transparently moved from @s to @s->child */

if (!s->child->bs) {

/*

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_abort(void *opaque)

}

assert(s->child->quiesced_parent);

bdrv_replace_child_noperm(s->child, s->old_bs);

+

+ bdrv_graph_wrunlock();

bdrv_unref(new_bs);

}

@@ -XXX,XX +XXX,XX @@ static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,

if (new_bs) {

bdrv_ref(new_bs);

}

+

+ bdrv_graph_wrlock(new_bs);

bdrv_replace_child_noperm(child, new_bs);

+ bdrv_graph_wrunlock();

/* old_bs reference is transparently moved from @child to @s */

}

@@ -XXX,XX +XXX,XX @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission qapi_perm)

* If @new_bs is non-NULL, the parent of @child must already be drained through

* @child and the caller must hold the AioContext lock for @new_bs.

*/

-static void bdrv_replace_child_noperm(BdrvChild *child,

- BlockDriverState *new_bs)

+static void GRAPH_WRLOCK

+bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *new_bs)

{

BlockDriverState *old_bs = child->bs;

int new_bs_quiesce_counter;

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

assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs));

}

- /* TODO Pull this up into the callers to avoid polling here */

- bdrv_graph_wrlock(new_bs);

if (old_bs) {

if (child->klass->detach) {

child->klass->detach(child);

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

child->klass->attach(child);

}

}

- bdrv_graph_wrunlock();

/*

* If the parent was drained through this BdrvChild previously, but new_bs

@@ -XXX,XX +XXX,XX @@ static void bdrv_attach_child_common_abort(void *opaque)

BlockDriverState *bs = s->child->bs;

GLOBAL_STATE_CODE();

+

+ bdrv_graph_wrlock(NULL);

bdrv_replace_child_noperm(s->child, NULL);

+ bdrv_graph_wrunlock();

if (bdrv_get_aio_context(bs) != s->old_child_ctx) {

bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abort);

@@ -XXX,XX +XXX,XX @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

* a problem, we already did this), but it will still poll until the parent

* is fully quiesced, so it will not be negatively affected either.

*/

+ bdrv_graph_wrlock(child_bs);

bdrv_parent_drained_begin_single(new_child);

bdrv_replace_child_noperm(new_child, child_bs);

+ bdrv_graph_wrunlock();

BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1);

*s = (BdrvAttachChildCommonState) {

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

BlockDriverState *child_bs = child->bs;

GLOBAL_STATE_CODE();

+ bdrv_graph_wrlock(NULL);

bdrv_replace_child_noperm(child, NULL);

+ bdrv_graph_wrunlock();

bdrv_child_free(child);

if (child_bs) {

--

2.41.0

Instead of taking the writer lock internally, require callers to already

hold it when calling bdrv_attach_child_common(). These callers will

typically already hold the graph lock once the locking work is

completed, which means that they can't call functions that take it

internally.

Note that the transaction callbacks still take the lock internally, so

tran_finalize() must be called without the lock held. This is because

bdrv_append() also calls bdrv_replace_node_noperm(), which currently

requires the transaction callbacks to be called unlocked. In the next

step, both of them can be switched to locked tran_finalize() calls

together.

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

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

Message-ID: <20230911094620.45040-11-kwolf@redhat.com>

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

---

block.c | 133 +++++++++++++++++++++++++++++++------------------

block/stream.c | 20 ++++++--

2 files changed, 100 insertions(+), 53 deletions(-)

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

index XXXXXXX..XXXXXXX 100644

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_attach_child_common_drv = {

* @child_bs can move to a different AioContext in this function. Callers must

* make sure that their AioContext locking is still correct after this.

*/

-static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

- const char *child_name,

- const BdrvChildClass *child_class,

- BdrvChildRole child_role,

- uint64_t perm, uint64_t shared_perm,

- void *opaque,

- Transaction *tran, Error **errp)

+static BdrvChild * GRAPH_WRLOCK

+bdrv_attach_child_common(BlockDriverState *child_bs,

+ const char *child_name,

+ const BdrvChildClass *child_class,

+ BdrvChildRole child_role,

+ uint64_t perm, uint64_t shared_perm,

+ void *opaque,

+ Transaction *tran, Error **errp)

{

BdrvChild *new_child;

AioContext *parent_ctx, *new_child_ctx;

@@ -XXX,XX +XXX,XX @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

* a problem, we already did this), but it will still poll until the parent

* is fully quiesced, so it will not be negatively affected either.

*/

- bdrv_graph_wrlock(child_bs);

bdrv_parent_drained_begin_single(new_child);

bdrv_replace_child_noperm(new_child, child_bs);

- bdrv_graph_wrunlock();

BdrvAttachChildCommonState *s = g_new(BdrvAttachChildCommonState, 1);

*s = (BdrvAttachChildCommonState) {

@@ -XXX,XX +XXX,XX @@ static BdrvChild *bdrv_attach_child_common(BlockDriverState *child_bs,

* @child_bs can move to a different AioContext in this function. Callers must

* make sure that their AioContext locking is still correct after this.

*/

-static BdrvChild *bdrv_attach_child_noperm(BlockDriverState *parent_bs,

- BlockDriverState *child_bs,

- const char *child_name,

- const BdrvChildClass *child_class,

- BdrvChildRole child_role,

- Transaction *tran,

- Error **errp)

+static BdrvChild * GRAPH_WRLOCK

+bdrv_attach_child_noperm(BlockDriverState *parent_bs,

+ BlockDriverState *child_bs,

+ const char *child_name,

+ const BdrvChildClass *child_class,

+ BdrvChildRole child_role,

+ Transaction *tran,

+ Error **errp)

{

uint64_t perm, shared_perm;

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

GLOBAL_STATE_CODE();

+ bdrv_graph_wrlock(child_bs);

+

child = bdrv_attach_child_common(child_bs, child_name, child_class,

child_role, perm, shared_perm, opaque,

tran, errp);

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

ret = bdrv_refresh_perms(child_bs, tran, errp);

out:

+ bdrv_graph_wrunlock();

tran_finalize(tran, ret);

bdrv_unref(child_bs);

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,

GLOBAL_STATE_CODE();

+ bdrv_graph_wrlock(child_bs);

+

child = bdrv_attach_child_noperm(parent_bs, child_bs, child_name,

child_class, child_role, tran, errp);

if (!child) {

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,

}

out:

+ bdrv_graph_wrunlock();

tran_finalize(tran, ret);

bdrv_unref(child_bs);

@@ -XXX,XX +XXX,XX @@ static BdrvChildRole bdrv_backing_role(BlockDriverState *bs)

* Sets the bs->backing or bs->file link of a BDS. A new reference is created;

* callers which don't need their own reference any more must call bdrv_unref().

*

+ * If the respective child is already present (i.e. we're detaching a node),

+ * that child node must be drained.

+ *

* Function doesn't update permissions, caller is responsible for this.

*

* The caller must hold the AioContext lock for @child_bs. Both @parent_bs and

* @child_bs can move to a different AioContext in this function. Callers must

* make sure that their AioContext locking is still correct after this.

*/

-static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,

- BlockDriverState *child_bs,

- bool is_backing,

- Transaction *tran, Error **errp)

+static int GRAPH_WRLOCK

+bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,

+ BlockDriverState *child_bs,

+ bool is_backing,

+ Transaction *tran, Error **errp)

{

bool update_inherits_from =

bdrv_inherits_from_recursive(child_bs, parent_bs);

@@ -XXX,XX +XXX,XX @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,

}

if (child) {

- bdrv_drained_begin(child->bs);

- bdrv_graph_wrlock(NULL);

-

+ assert(child->bs->quiesce_counter);

bdrv_unset_inherits_from(parent_bs, child, tran);

bdrv_remove_child(child, tran);

-

- bdrv_graph_wrunlock();

- bdrv_drained_end(child->bs);

}

if (!child_bs) {

@@ -XXX,XX +XXX,XX @@ static int bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,

}

out:

- bdrv_graph_rdlock_main_loop();

bdrv_refresh_limits(parent_bs, tran, NULL);

- bdrv_graph_rdunlock_main_loop();