Re: [RFC PATCH v2 08/25] block: introduce assert_bdrv_graph_writable
On Thu, Oct 07, 2021 at 03:47:42PM +0200, Emanuele Giuseppe Esposito wrote: > > > On 07/10/2021 14:02, Paolo Bonzini wrote: > > > --- a/block/io.c > > > +++ b/block/io.c > > > @@ -739,6 +739,11 @@ void bdrv_drain_all(void) > > > bdrv_drain_all_end(); > > > } > > > +void assert_bdrv_graph_writable(BlockDriverState *bs) > > > +{ > > > + g_assert(qatomic_read(&bs->quiesce_counter) > 0 || > > > qemu_in_main_thread()); > > > +} > > > > Hmm, wait - I think this should be an "&&", not an OR? > > > > You are right, && makes more sense. But as you said, using an AND will make > the assertion fail in multiple places, because the necessary drains are > missing. So I am going to remove the check for drains and leave it as todo. > I will handle this case in another patch. BTW when an assertion expression tests unrelated things it helps to use separate assert() calls instead of &&. That way it's obvious which sub-expression failed from the error message and it's not necessary to inspect the coredump. In other words: assert(a && b) -> Assertion `a && b` failed. This doesn't tell me whether it was a or b that was false. The assertion failure is easier to diagnose if you split it: assert(a); -> Assertion `a` failed. assert(b); -> Assertion `b` failed. Stefan signature.asc Description: PGP signature
Re: [RFC PATCH v2 08/25] block: introduce assert_bdrv_graph_writable
On 07/10/2021 14:02, Paolo Bonzini wrote: --- a/block/io.c +++ b/block/io.c @@ -739,6 +739,11 @@ void bdrv_drain_all(void) bdrv_drain_all_end(); } +void assert_bdrv_graph_writable(BlockDriverState *bs) +{ + g_assert(qatomic_read(&bs->quiesce_counter) > 0 || qemu_in_main_thread()); +} Hmm, wait - I think this should be an "&&", not an OR? You are right, && makes more sense. But as you said, using an AND will make the assertion fail in multiple places, because the necessary drains are missing. So I am going to remove the check for drains and leave it as todo. I will handle this case in another patch. Thank you, Emanuele
Re: [RFC PATCH v2 08/25] block: introduce assert_bdrv_graph_writable
On 05/10/21 16:31, Emanuele Giuseppe Esposito wrote: We want to be sure that the functions that write the child and parent list of a bs are either under BQL or drain. If this guarantee holds, then we can read the list also in the I/O APIs. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 5 + block/io.c | 5 + include/block/block_int-global-state.h | 8 +++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index b912f517a4..7d1eb847a4 100644 --- a/block.c +++ b/block.c @@ -2711,12 +2711,14 @@ static void bdrv_replace_child_noperm(BdrvChild *child, if (child->klass->detach) { child->klass->detach(child); } +assert_bdrv_graph_writable(old_bs); QLIST_REMOVE(child, next_parent); } child->bs = new_bs; if (new_bs) { +assert_bdrv_graph_writable(new_bs); QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* @@ -2917,6 +2919,7 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return ret; } +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, *child, next); /* * child is removed in bdrv_attach_child_common_abort(), so don't care to @@ -3117,6 +3120,7 @@ static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child, void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child) { g_assert(qemu_in_main_thread()); +assert_bdrv_graph_writable(parent); if (child == NULL) { return; } @@ -4878,6 +4882,7 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) BdrvRemoveFilterOrCowChild *s = opaque; BlockDriverState *parent_bs = s->child->opaque; +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); if (s->is_backing) { parent_bs->backing = s->child; diff --git a/block/io.c b/block/io.c index 21dcc5d962..d184183b07 100644 --- a/block/io.c +++ b/block/io.c @@ -739,6 +739,11 @@ void bdrv_drain_all(void) bdrv_drain_all_end(); } +void assert_bdrv_graph_writable(BlockDriverState *bs) +{ +g_assert(qatomic_read(&bs->quiesce_counter) > 0 || qemu_in_main_thread()); +} Hmm, wait - I think this should be an "&&", not an OR? Paolo + /** * Remove an active request from the tracked requests list * diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h index aad549cb85..a53ab146fc 100644 --- a/include/block/block_int-global-state.h +++ b/include/block/block_int-global-state.h @@ -343,4 +343,10 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, */ void bdrv_drain_all_end_quiesce(BlockDriverState *bs); -#endif /* BLOCK_INT_GLOBAL_STATE*/ +/** + * Make sure that the function is either running under + * drain, or under BQL. + */ +void assert_bdrv_graph_writable(BlockDriverState *bs); + +#endif /* BLOCK_INT_GLOBAL_STATE */
Re: [RFC PATCH v2 08/25] block: introduce assert_bdrv_graph_writable
On 05/10/21 16:31, Emanuele Giuseppe Esposito wrote: We want to be sure that the functions that write the child and parent list of a bs are either under BQL or drain. If this guarantee holds, then we can read the list also in the I/O APIs. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 5 + block/io.c | 5 + include/block/block_int-global-state.h | 8 +++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index b912f517a4..7d1eb847a4 100644 --- a/block.c +++ b/block.c @@ -2711,12 +2711,14 @@ static void bdrv_replace_child_noperm(BdrvChild *child, if (child->klass->detach) { child->klass->detach(child); } +assert_bdrv_graph_writable(old_bs); QLIST_REMOVE(child, next_parent); } child->bs = new_bs; if (new_bs) { +assert_bdrv_graph_writable(new_bs); QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* @@ -2917,6 +2919,7 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return ret; } +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, *child, next); /* * child is removed in bdrv_attach_child_common_abort(), so don't care to @@ -3117,6 +3120,7 @@ static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child, void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child) { g_assert(qemu_in_main_thread()); +assert_bdrv_graph_writable(parent); if (child == NULL) { return; } @@ -4878,6 +4882,7 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) BdrvRemoveFilterOrCowChild *s = opaque; BlockDriverState *parent_bs = s->child->opaque; +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); if (s->is_backing) { parent_bs->backing = s->child; diff --git a/block/io.c b/block/io.c index 21dcc5d962..d184183b07 100644 --- a/block/io.c +++ b/block/io.c @@ -739,6 +739,11 @@ void bdrv_drain_all(void) bdrv_drain_all_end(); } +void assert_bdrv_graph_writable(BlockDriverState *bs) +{ +g_assert(qatomic_read(&bs->quiesce_counter) > 0 || qemu_in_main_thread()); +} + /** * Remove an active request from the tracked requests list * diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h index aad549cb85..a53ab146fc 100644 --- a/include/block/block_int-global-state.h +++ b/include/block/block_int-global-state.h @@ -343,4 +343,10 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, */ void bdrv_drain_all_end_quiesce(BlockDriverState *bs); -#endif /* BLOCK_INT_GLOBAL_STATE*/ +/** + * Make sure that the function is either running under + * drain, or under BQL. + */ +void assert_bdrv_graph_writable(BlockDriverState *bs); + +#endif /* BLOCK_INT_GLOBAL_STATE */ Reviewed-by: Paolo Bonzini
[RFC PATCH v2 08/25] block: introduce assert_bdrv_graph_writable
We want to be sure that the functions that write the child and parent list of a bs are either under BQL or drain. If this guarantee holds, then we can read the list also in the I/O APIs. Signed-off-by: Emanuele Giuseppe Esposito --- block.c| 5 + block/io.c | 5 + include/block/block_int-global-state.h | 8 +++- 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/block.c b/block.c index b912f517a4..7d1eb847a4 100644 --- a/block.c +++ b/block.c @@ -2711,12 +2711,14 @@ static void bdrv_replace_child_noperm(BdrvChild *child, if (child->klass->detach) { child->klass->detach(child); } +assert_bdrv_graph_writable(old_bs); QLIST_REMOVE(child, next_parent); } child->bs = new_bs; if (new_bs) { +assert_bdrv_graph_writable(new_bs); QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* @@ -2917,6 +2919,7 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return ret; } +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, *child, next); /* * child is removed in bdrv_attach_child_common_abort(), so don't care to @@ -3117,6 +3120,7 @@ static void bdrv_unset_inherits_from(BlockDriverState *root, BdrvChild *child, void bdrv_unref_child(BlockDriverState *parent, BdrvChild *child) { g_assert(qemu_in_main_thread()); +assert_bdrv_graph_writable(parent); if (child == NULL) { return; } @@ -4878,6 +4882,7 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) BdrvRemoveFilterOrCowChild *s = opaque; BlockDriverState *parent_bs = s->child->opaque; +assert_bdrv_graph_writable(parent_bs); QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); if (s->is_backing) { parent_bs->backing = s->child; diff --git a/block/io.c b/block/io.c index 21dcc5d962..d184183b07 100644 --- a/block/io.c +++ b/block/io.c @@ -739,6 +739,11 @@ void bdrv_drain_all(void) bdrv_drain_all_end(); } +void assert_bdrv_graph_writable(BlockDriverState *bs) +{ +g_assert(qatomic_read(&bs->quiesce_counter) > 0 || qemu_in_main_thread()); +} + /** * Remove an active request from the tracked requests list * diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h index aad549cb85..a53ab146fc 100644 --- a/include/block/block_int-global-state.h +++ b/include/block/block_int-global-state.h @@ -343,4 +343,10 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, */ void bdrv_drain_all_end_quiesce(BlockDriverState *bs); -#endif /* BLOCK_INT_GLOBAL_STATE*/ +/** + * Make sure that the function is either running under + * drain, or under BQL. + */ +void assert_bdrv_graph_writable(BlockDriverState *bs); + +#endif /* BLOCK_INT_GLOBAL_STATE */ -- 2.27.0