| Message ID | 20220118162738.1366281-8-eesposit@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Removal of Aiocontext lock through drains: protect bdrv_replace_child_noperm. | expand |
On 1/18/22 17:27, Emanuele Giuseppe Esposito wrote: > Same as the locked version, but use BDRV_POLL_UNLOCKED ... We are going to add drains to all graph modifications, and they are generally performed without the AioContext lock taken. Paolo > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > block/io.c | 50 +++++++++++++++++++++++++++++----------- > include/block/block-io.h | 2 ++ > 2 files changed, 39 insertions(+), 13 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 5123b7b713..9d5167f64a 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -244,6 +244,7 @@ typedef struct { > bool begin; > bool recursive; > bool poll; > + bool unlock; > BdrvChild *parent; > bool ignore_bds_parents; > int *drained_end_counter; > @@ -334,7 +335,7 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive, > > static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, > BdrvChild *parent, bool ignore_bds_parents, > - bool poll); > + bool poll, bool unlock); > static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, > BdrvChild *parent, bool ignore_bds_parents, > int *drained_end_counter); > @@ -352,7 +353,8 @@ static void bdrv_co_drain_bh_cb(void *opaque) > if (data->begin) { > assert(!data->drained_end_counter); > bdrv_do_drained_begin(bs, data->recursive, data->parent, > - data->ignore_bds_parents, data->poll); > + data->ignore_bds_parents, data->poll, > + data->unlock); > } else { > assert(!data->poll); > bdrv_do_drained_end(bs, data->recursive, data->parent, > @@ -374,6 +376,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > BdrvChild *parent, > bool ignore_bds_parents, > bool poll, > + bool unlock, > int *drained_end_counter) > { > BdrvCoDrainData data; > @@ -394,6 +397,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, > .parent = parent, > .ignore_bds_parents = ignore_bds_parents, > .poll = poll, > + .unlock = unlock, > .drained_end_counter = drained_end_counter, > }; > > @@ -441,13 +445,13 @@ static void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, > > static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, > BdrvChild *parent, bool ignore_bds_parents, > - bool poll) > + bool poll, bool unlock) > { > BdrvChild *child, *next; > > if (qemu_in_coroutine()) { > bdrv_co_yield_to_drain(bs, true, recursive, parent, ignore_bds_parents, > - poll, NULL); > + poll, unlock, NULL); > return; > } > > @@ -458,7 +462,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, > bs->recursive_quiesce_counter++; > QLIST_FOREACH_SAFE(child, &bs->children, next, next) { > bdrv_do_drained_begin(child->bs, true, child, ignore_bds_parents, > - false); > + false, false); > } > } > > @@ -473,23 +477,35 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, > */ > if (poll) { > assert(!ignore_bds_parents); > - BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent)); > + if (unlock) { > + BDRV_POLL_WHILE_UNLOCKED(bs, > + bdrv_drain_poll_top_level(bs, recursive, > + parent)); > + } else { > + BDRV_POLL_WHILE(bs, > + bdrv_drain_poll_top_level(bs, recursive, parent)); > + } > } > } > > void bdrv_drained_begin(BlockDriverState *bs) > { > - bdrv_do_drained_begin(bs, false, NULL, false, true); > + bdrv_do_drained_begin(bs, false, NULL, false, true, false); > } > > void bdrv_subtree_drained_begin(BlockDriverState *bs) > { > - bdrv_do_drained_begin(bs, true, NULL, false, true); > + bdrv_do_drained_begin(bs, true, NULL, false, true, false); > +} > + > +void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs) > +{ > + bdrv_do_drained_begin(bs, true, NULL, false, true, true); > } > > void bdrv_drained_begin_no_poll(BlockDriverState *bs) > { > - bdrv_do_drained_begin(bs, false, NULL, false, false); > + bdrv_do_drained_begin(bs, false, NULL, false, false, false); > } > > /** > @@ -517,7 +533,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, > > if (qemu_in_coroutine()) { > bdrv_co_yield_to_drain(bs, false, recursive, parent, ignore_bds_parents, > - false, drained_end_counter); > + false, false, drained_end_counter); > return; > } > assert(bs->quiesce_counter > 0); > @@ -561,12 +577,19 @@ void bdrv_subtree_drained_end(BlockDriverState *bs) > BDRV_POLL_WHILE(bs, qatomic_read(&drained_end_counter) > 0); > } > > +void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs) > +{ > + int drained_end_counter = 0; > + bdrv_do_drained_end(bs, true, NULL, false, &drained_end_counter); > + BDRV_POLL_WHILE_UNLOCKED(bs, qatomic_read(&drained_end_counter) > 0); > +} > + > void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent) > { > int i; > > for (i = 0; i < new_parent->recursive_quiesce_counter; i++) { > - bdrv_do_drained_begin(child->bs, true, child, false, true); > + bdrv_do_drained_begin(child->bs, true, child, false, true, false); > } > } > > @@ -651,7 +674,8 @@ void bdrv_drain_all_begin(void) > assert(qemu_in_main_thread()); > > if (qemu_in_coroutine()) { > - bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, NULL); > + bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, false, > + NULL); > return; > } > > @@ -676,7 +700,7 @@ void bdrv_drain_all_begin(void) > AioContext *aio_context = bdrv_get_aio_context(bs); > > aio_context_acquire(aio_context); > - bdrv_do_drained_begin(bs, false, NULL, true, false); > + bdrv_do_drained_begin(bs, false, NULL, true, false, false); > aio_context_release(aio_context); > } > > diff --git a/include/block/block-io.h b/include/block/block-io.h > index 34a991649c..a329ae24c1 100644 > --- a/include/block/block-io.h > +++ b/include/block/block-io.h > @@ -253,6 +253,7 @@ void bdrv_drained_begin_no_poll(BlockDriverState *bs); > * exclusive access to all child nodes as well. > */ > void bdrv_subtree_drained_begin(BlockDriverState *bs); > +void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs); > > /** > * bdrv_drained_end: > @@ -285,6 +286,7 @@ void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter); > * End a quiescent section started by bdrv_subtree_drained_begin(). > */ > void bdrv_subtree_drained_end(BlockDriverState *bs); > +void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs); > > bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, > uint32_t granularity, Error **errp);
On Tue, Jan 18, 2022 at 11:27:33AM -0500, Emanuele Giuseppe Esposito wrote: > diff --git a/block/io.c b/block/io.c > index 5123b7b713..9d5167f64a 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -244,6 +244,7 @@ typedef struct { > bool begin; > bool recursive; > bool poll; > + bool unlock; > BdrvChild *parent; > bool ignore_bds_parents; > int *drained_end_counter; ... > @@ -473,23 +477,35 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, > */ > if (poll) { > assert(!ignore_bds_parents); > - BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent)); > + if (unlock) { "Unlock" is a verb that suggests we'll perform an unlock operation. Please call it "unlocked" instead. > + BDRV_POLL_WHILE_UNLOCKED(bs, > + bdrv_drain_poll_top_level(bs, recursive, > + parent)); > + } else { > + BDRV_POLL_WHILE(bs, > + bdrv_drain_poll_top_level(bs, recursive, parent)); > + } > } > }
diff --git a/block/io.c b/block/io.c index 5123b7b713..9d5167f64a 100644 --- a/block/io.c +++ b/block/io.c @@ -244,6 +244,7 @@ typedef struct { bool begin; bool recursive; bool poll; + bool unlock; BdrvChild *parent; bool ignore_bds_parents; int *drained_end_counter; @@ -334,7 +335,7 @@ static bool bdrv_drain_poll_top_level(BlockDriverState *bs, bool recursive, static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, BdrvChild *parent, bool ignore_bds_parents, - bool poll); + bool poll, bool unlock); static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, BdrvChild *parent, bool ignore_bds_parents, int *drained_end_counter); @@ -352,7 +353,8 @@ static void bdrv_co_drain_bh_cb(void *opaque) if (data->begin) { assert(!data->drained_end_counter); bdrv_do_drained_begin(bs, data->recursive, data->parent, - data->ignore_bds_parents, data->poll); + data->ignore_bds_parents, data->poll, + data->unlock); } else { assert(!data->poll); bdrv_do_drained_end(bs, data->recursive, data->parent, @@ -374,6 +376,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, BdrvChild *parent, bool ignore_bds_parents, bool poll, + bool unlock, int *drained_end_counter) { BdrvCoDrainData data; @@ -394,6 +397,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs, .parent = parent, .ignore_bds_parents = ignore_bds_parents, .poll = poll, + .unlock = unlock, .drained_end_counter = drained_end_counter, }; @@ -441,13 +445,13 @@ static void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, BdrvChild *parent, bool ignore_bds_parents, - bool poll) + bool poll, bool unlock) { BdrvChild *child, *next; if (qemu_in_coroutine()) { bdrv_co_yield_to_drain(bs, true, recursive, parent, ignore_bds_parents, - poll, NULL); + poll, unlock, NULL); return; } @@ -458,7 +462,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, bs->recursive_quiesce_counter++; QLIST_FOREACH_SAFE(child, &bs->children, next, next) { bdrv_do_drained_begin(child->bs, true, child, ignore_bds_parents, - false); + false, false); } } @@ -473,23 +477,35 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive, */ if (poll) { assert(!ignore_bds_parents); - BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, recursive, parent)); + if (unlock) { + BDRV_POLL_WHILE_UNLOCKED(bs, + bdrv_drain_poll_top_level(bs, recursive, + parent)); + } else { + BDRV_POLL_WHILE(bs, + bdrv_drain_poll_top_level(bs, recursive, parent)); + } } } void bdrv_drained_begin(BlockDriverState *bs) { - bdrv_do_drained_begin(bs, false, NULL, false, true); + bdrv_do_drained_begin(bs, false, NULL, false, true, false); } void bdrv_subtree_drained_begin(BlockDriverState *bs) { - bdrv_do_drained_begin(bs, true, NULL, false, true); + bdrv_do_drained_begin(bs, true, NULL, false, true, false); +} + +void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs) +{ + bdrv_do_drained_begin(bs, true, NULL, false, true, true); } void bdrv_drained_begin_no_poll(BlockDriverState *bs) { - bdrv_do_drained_begin(bs, false, NULL, false, false); + bdrv_do_drained_begin(bs, false, NULL, false, false, false); } /** @@ -517,7 +533,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, bool recursive, if (qemu_in_coroutine()) { bdrv_co_yield_to_drain(bs, false, recursive, parent, ignore_bds_parents, - false, drained_end_counter); + false, false, drained_end_counter); return; } assert(bs->quiesce_counter > 0); @@ -561,12 +577,19 @@ void bdrv_subtree_drained_end(BlockDriverState *bs) BDRV_POLL_WHILE(bs, qatomic_read(&drained_end_counter) > 0); } +void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs) +{ + int drained_end_counter = 0; + bdrv_do_drained_end(bs, true, NULL, false, &drained_end_counter); + BDRV_POLL_WHILE_UNLOCKED(bs, qatomic_read(&drained_end_counter) > 0); +} + void bdrv_apply_subtree_drain(BdrvChild *child, BlockDriverState *new_parent) { int i; for (i = 0; i < new_parent->recursive_quiesce_counter; i++) { - bdrv_do_drained_begin(child->bs, true, child, false, true); + bdrv_do_drained_begin(child->bs, true, child, false, true, false); } } @@ -651,7 +674,8 @@ void bdrv_drain_all_begin(void) assert(qemu_in_main_thread()); if (qemu_in_coroutine()) { - bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, NULL); + bdrv_co_yield_to_drain(NULL, true, false, NULL, true, true, false, + NULL); return; } @@ -676,7 +700,7 @@ void bdrv_drain_all_begin(void) AioContext *aio_context = bdrv_get_aio_context(bs); aio_context_acquire(aio_context); - bdrv_do_drained_begin(bs, false, NULL, true, false); + bdrv_do_drained_begin(bs, false, NULL, true, false, false); aio_context_release(aio_context); } diff --git a/include/block/block-io.h b/include/block/block-io.h index 34a991649c..a329ae24c1 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -253,6 +253,7 @@ void bdrv_drained_begin_no_poll(BlockDriverState *bs); * exclusive access to all child nodes as well. */ void bdrv_subtree_drained_begin(BlockDriverState *bs); +void bdrv_subtree_drained_begin_unlocked(BlockDriverState *bs); /** * bdrv_drained_end: @@ -285,6 +286,7 @@ void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter); * End a quiescent section started by bdrv_subtree_drained_begin(). */ void bdrv_subtree_drained_end(BlockDriverState *bs); +void bdrv_subtree_drained_end_unlocked(BlockDriverState *bs); bool bdrv_can_store_new_dirty_bitmap(BlockDriverState *bs, const char *name, uint32_t granularity, Error **errp);
Same as the locked version, but use BDRV_POLL_UNLOCKED Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- block/io.c | 50 +++++++++++++++++++++++++++++----------- include/block/block-io.h | 2 ++ 2 files changed, 39 insertions(+), 13 deletions(-)