| Message ID | 20211005143215.29500-9-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block layer: split block APIs in global state and I/O | expand |
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 <eesposit@redhat.com> > --- > 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 <pbonzini@redhat.com>
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 <eesposit@redhat.com> > --- > 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 */ >
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
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
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 */
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 <eesposit@redhat.com> --- block.c | 5 +++++ block/io.c | 5 +++++ include/block/block_int-global-state.h | 8 +++++++- 3 files changed, 17 insertions(+), 1 deletion(-)