| Message ID | 20211124064418.3120601-10-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 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: > We want to be sure that the functions that write the child and > parent list of a bs are under BQL and drain. > > BQL prevents from concurrent writings from the GS API, while > drains protect from I/O. > > TODO: drains are missing in some functions using this assert. > Therefore a proper assertion will fail. Because adding drains > requires additional discussions, they will be added in future > series. > > Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> > --- > include/block/block_int-global-state.h | 10 +++++++++- > block.c | 4 ++++ > block/io.c | 11 +++++++++++ > 3 files changed, 24 insertions(+), 1 deletion(-) > > diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h > index a1b7d0579d..fa96e8b449 100644 > --- a/include/block/block_int-global-state.h > +++ b/include/block/block_int-global-state.h > @@ -312,4 +312,12 @@ void bdrv_remove_aio_context_notifier(BlockDriverState *bs, > */ > void bdrv_drain_all_end_quiesce(BlockDriverState *bs); > > -#endif /* BLOCK_INT_GLOBAL_STATE*/ This looks like it should be squashed into patch 7, sorry I missed this in v4... (Rest of this patch looks good to me, for the record – and while I’m at it, for patches I didn’t reply to so far, I planned to send an R-b later. But then there’s things like patches 2/3 looking good to me, but it turned out in my review for patch 4 that bdrv_lock_medium() is used in an I/O path, so I can’t really send an R-b now anymore...) Hanna > +/** > + * Make sure that the function is running under both drain and BQL. > + * The latter protects from concurrent writings > + * from the GS API, while the former prevents concurrent reads > + * from I/O. > + */ > +void assert_bdrv_graph_writable(BlockDriverState *bs); > + > +#endif /* BLOCK_INT_GLOBAL_STATE */ > diff --git a/block.c b/block.c > index 198ec636ff..522a273140 100644 > --- a/block.c > +++ b/block.c > @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > > + assert_bdrv_graph_writable(bs); > QLIST_INSERT_HEAD(&bs->children, child, next); > > if (child->role & BDRV_CHILD_COW) { > @@ -1435,6 +1436,7 @@ static void bdrv_child_cb_detach(BdrvChild *child) > > bdrv_unapply_subtree_drain(child, bs); > > + assert_bdrv_graph_writable(bs); > QLIST_REMOVE(child, next); > } > > @@ -2818,6 +2820,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > if (child->klass->detach) { > child->klass->detach(child); > } > + assert_bdrv_graph_writable(old_bs); > QLIST_REMOVE(child, next_parent); > } > > @@ -2827,6 +2830,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, > } > > if (new_bs) { > + assert_bdrv_graph_writable(new_bs); > QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); > > /* > diff --git a/block/io.c b/block/io.c > index cb095deeec..3be08cad29 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -734,6 +734,17 @@ void bdrv_drain_all(void) > bdrv_drain_all_end(); > } > > +void assert_bdrv_graph_writable(BlockDriverState *bs) > +{ > + /* > + * TODO: this function is incomplete. Because the users of this > + * assert lack the necessary drains, check only for BQL. > + * Once the necessary drains are added, > + * assert also for qatomic_read(&bs->quiesce_counter) > 0 > + */ > + assert(qemu_in_main_thread()); > +} > + > /** > * Remove an active request from the tracked requests list > *
On 10/12/2021 18:43, Hanna Reitz wrote: > On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: >> We want to be sure that the functions that write the child and >> parent list of a bs are under BQL and drain. >> >> BQL prevents from concurrent writings from the GS API, while >> drains protect from I/O. >> >> TODO: drains are missing in some functions using this assert. >> Therefore a proper assertion will fail. Because adding drains >> requires additional discussions, they will be added in future >> series. >> >> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >> --- >> include/block/block_int-global-state.h | 10 +++++++++- >> block.c | 4 ++++ >> block/io.c | 11 +++++++++++ >> 3 files changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/include/block/block_int-global-state.h >> b/include/block/block_int-global-state.h >> index a1b7d0579d..fa96e8b449 100644 >> --- a/include/block/block_int-global-state.h >> +++ b/include/block/block_int-global-state.h >> @@ -312,4 +312,12 @@ void >> bdrv_remove_aio_context_notifier(BlockDriverState *bs, >> */ >> void bdrv_drain_all_end_quiesce(BlockDriverState *bs); >> -#endif /* BLOCK_INT_GLOBAL_STATE*/ > > This looks like it should be squashed into patch 7, sorry I missed this > in v4... > > (Rest of this patch looks good to me, for the record – and while I’m at > it, for patches I didn’t reply to so far, I planned to send an R-b > later. But then there’s things like patches 2/3 looking good to me, but > it turned out in my review for patch 4 that bdrv_lock_medium() is used > in an I/O path, so I can’t really send an R-b now anymore...) > Sorry I don't understand this, what should be squashed into patch 7? The assertion? If so, why? Thank you, Emanuele
On 14.12.21 20:48, Emanuele Giuseppe Esposito wrote: > > > On 10/12/2021 18:43, Hanna Reitz wrote: >> On 24.11.21 07:43, Emanuele Giuseppe Esposito wrote: >>> We want to be sure that the functions that write the child and >>> parent list of a bs are under BQL and drain. >>> >>> BQL prevents from concurrent writings from the GS API, while >>> drains protect from I/O. >>> >>> TODO: drains are missing in some functions using this assert. >>> Therefore a proper assertion will fail. Because adding drains >>> requires additional discussions, they will be added in future >>> series. >>> >>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> >>> --- >>> include/block/block_int-global-state.h | 10 +++++++++- >>> block.c | 4 ++++ >>> block/io.c | 11 +++++++++++ >>> 3 files changed, 24 insertions(+), 1 deletion(-) >>> >>> diff --git a/include/block/block_int-global-state.h >>> b/include/block/block_int-global-state.h >>> index a1b7d0579d..fa96e8b449 100644 >>> --- a/include/block/block_int-global-state.h >>> +++ b/include/block/block_int-global-state.h >>> @@ -312,4 +312,12 @@ void >>> bdrv_remove_aio_context_notifier(BlockDriverState *bs, >>> */ >>> void bdrv_drain_all_end_quiesce(BlockDriverState *bs); >>> -#endif /* BLOCK_INT_GLOBAL_STATE*/ >> >> This looks like it should be squashed into patch 7, sorry I missed >> this in v4... >> >> (Rest of this patch looks good to me, for the record – and while I’m >> at it, for patches I didn’t reply to so far, I planned to send an R-b >> later. But then there’s things like patches 2/3 looking good to me, >> but it turned out in my review for patch 4 that bdrv_lock_medium() is >> used in an I/O path, so I can’t really send an R-b now anymore...) >> > Sorry I don't understand this, what should be squashed into patch 7? > The assertion? If so, why? No, the -#endif /* BLOCK_INT_GLOBAL_STATE*/ +#endif /* BLOCK_INT_GLOBAL_STATE */ part of the hunk. Hanna
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h index a1b7d0579d..fa96e8b449 100644 --- a/include/block/block_int-global-state.h +++ b/include/block/block_int-global-state.h @@ -312,4 +312,12 @@ 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 running under both drain and BQL. + * The latter protects from concurrent writings + * from the GS API, while the former prevents concurrent reads + * from I/O. + */ +void assert_bdrv_graph_writable(BlockDriverState *bs); + +#endif /* BLOCK_INT_GLOBAL_STATE */ diff --git a/block.c b/block.c index 198ec636ff..522a273140 100644 --- a/block.c +++ b/block.c @@ -1416,6 +1416,7 @@ static void bdrv_child_cb_attach(BdrvChild *child) { BlockDriverState *bs = child->opaque; + assert_bdrv_graph_writable(bs); QLIST_INSERT_HEAD(&bs->children, child, next); if (child->role & BDRV_CHILD_COW) { @@ -1435,6 +1436,7 @@ static void bdrv_child_cb_detach(BdrvChild *child) bdrv_unapply_subtree_drain(child, bs); + assert_bdrv_graph_writable(bs); QLIST_REMOVE(child, next); } @@ -2818,6 +2820,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, if (child->klass->detach) { child->klass->detach(child); } + assert_bdrv_graph_writable(old_bs); QLIST_REMOVE(child, next_parent); } @@ -2827,6 +2830,7 @@ static void bdrv_replace_child_noperm(BdrvChild **childp, } if (new_bs) { + assert_bdrv_graph_writable(new_bs); QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent); /* diff --git a/block/io.c b/block/io.c index cb095deeec..3be08cad29 100644 --- a/block/io.c +++ b/block/io.c @@ -734,6 +734,17 @@ void bdrv_drain_all(void) bdrv_drain_all_end(); } +void assert_bdrv_graph_writable(BlockDriverState *bs) +{ + /* + * TODO: this function is incomplete. Because the users of this + * assert lack the necessary drains, check only for BQL. + * Once the necessary drains are added, + * assert also for qatomic_read(&bs->quiesce_counter) > 0 + */ + assert(qemu_in_main_thread()); +} + /** * Remove an active request from the tracked requests list *
We want to be sure that the functions that write the child and parent list of a bs are under BQL and drain. BQL prevents from concurrent writings from the GS API, while drains protect from I/O. TODO: drains are missing in some functions using this assert. Therefore a proper assertion will fail. Because adding drains requires additional discussions, they will be added in future series. Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com> --- include/block/block_int-global-state.h | 10 +++++++++- block.c | 4 ++++ block/io.c | 11 +++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-)