Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
On 12/12/22 16:57, Kevin Wolf wrote: I looks to me like this is a problem with the test case rather than the change per se. It seems to be fixed with this patch that is already posted as part of the next series: [PATCH 09/18] test-bdrv-drain: Fix incorrrect drain assumptions https://lists.gnu.org/archive/html/qemu-block/2022-12/msg00165.html Cool, thanks. I'll retest once the series hits block-next. In the meanwhile you can ignore the first three patches of http://patchew.org/QEMU/20221212125920.248567-1-pbonz...@redhat.com. Paolo
Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
Am 09.12.2022 um 17:53 hat Paolo Bonzini geschrieben: > On 11/18/22 18:41, Kevin Wolf wrote: > > In order to make sure that bdrv_replace_child_noperm() doesn't have to > > poll any more, get rid of the bdrv_parent_drained_begin_single() call. > > > > This is possible now because we can require that the parent is already > > drained through the child in question when the function is called and we > > don't call the parent drain callbacks more than once. > > > > The additional drain calls needed in callers cause the test case to run > > its code in the drain handler too early (bdrv_attach_child() drains > > now), so modify it to only enable the code after the test setup has > > completed. > > > > Signed-off-by: Kevin Wolf > > I hate to bear bad news, but this breaks the Windows builds on github > (msys-32bit, msys-64bit) with an obscure but 100% reproducible > > 51/88 qemu:unit / test-bdrv-drain ERROR 1.30s (exit status > 3221225477 or signal 3221225349 SIGinvalid) > > The exit status is 0xC005 aka a Windows SIGSEGV. With some luck it > could be reproducible with Wine (but no gdb). I can reproduce it with mingw+wine (and actually gdb! Still a somewhat limited debugging environment, but not as bad as I imagined.) I looks to me like this is a problem with the test case rather than the change per se. It seems to be fixed with this patch that is already posted as part of the next series: [PATCH 09/18] test-bdrv-drain: Fix incorrrect drain assumptions https://lists.gnu.org/archive/html/qemu-block/2022-12/msg00165.html Kevin
Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
On 12/9/22 17:53, Paolo Bonzini wrote:
On 11/18/22 18:41, Kevin Wolf wrote:
In order to make sure that bdrv_replace_child_noperm() doesn't have to
poll any more, get rid of the bdrv_parent_drained_begin_single() call.
This is possible now because we can require that the parent is already
drained through the child in question when the function is called and we
don't call the parent drain callbacks more than once.
The additional drain calls needed in callers cause the test case to run
its code in the drain handler too early (bdrv_attach_child() drains
now), so modify it to only enable the code after the test setup has
completed.
Signed-off-by: Kevin Wolf
I hate to bear bad news, but this breaks the Windows builds on github
(msys-32bit, msys-64bit) with an obscure but 100% reproducible
51/88 qemu:unit / test-bdrv-drain ERROR 1.30s (exit status
3221225477 or signal 3221225349 SIGinvalid)
The exit status is 0xC005 aka a Windows SIGSEGV. With some luck it
could be reproducible with Wine (but no gdb).
I am testing dropping this patch and squashing
diff --git a/block.c b/block.c
index 1a2a8d9de907..bb9c85222168 100644
--- a/block.c
+++ b/block.c
@@ -2870,7 +2870,9 @@ static void bdrv_replace_child_noperm(BdrvChild
*child,
*/
new_bs_quiesce_counter = (new_bs ? new_bs->quiesce_counter : 0);
if (new_bs_quiesce_counter && !child->quiesced_parent) {
-bdrv_parent_drained_begin_single(child, true);
+bdrv_parent_drained_begin_single(child);
+AIO_WAIT_WHILE(bdrv_child_get_parent_aio_context(child),
+ bdrv_parent_drained_poll_single(c));
}
if (old_bs) {
in patch 15/15, which should at least allow me to keep a lot of the
cleanups I had on top of this series.
Paolo
Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
On 11/18/22 18:41, Kevin Wolf wrote: In order to make sure that bdrv_replace_child_noperm() doesn't have to poll any more, get rid of the bdrv_parent_drained_begin_single() call. This is possible now because we can require that the parent is already drained through the child in question when the function is called and we don't call the parent drain callbacks more than once. The additional drain calls needed in callers cause the test case to run its code in the drain handler too early (bdrv_attach_child() drains now), so modify it to only enable the code after the test setup has completed. Signed-off-by: Kevin Wolf I hate to bear bad news, but this breaks the Windows builds on github (msys-32bit, msys-64bit) with an obscure but 100% reproducible 51/88 qemu:unit / test-bdrv-drain ERROR 1.30s (exit status 3221225477 or signal 3221225349 SIGinvalid) The exit status is 0xC005 aka a Windows SIGSEGV. With some luck it could be reproducible with Wine (but no gdb). Paolo
Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
Am 25.11.2022 um 17:07 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 11/18/22 20:41, Kevin Wolf wrote: > > In order to make sure that bdrv_replace_child_noperm() doesn't have to > > poll any more, get rid of the bdrv_parent_drained_begin_single() call. > > > > This is possible now because we can require that the parent is already > > drained through the child in question when the function is called and we > > don't call the parent drain callbacks more than once. > > > > The additional drain calls needed in callers cause the test case to run > > its code in the drain handler too early (bdrv_attach_child() drains > > now), so modify it to only enable the code after the test setup has > > completed. > > > > Signed-off-by: Kevin Wolf > > It's still hard to keep this all in mind, so weak: > Reviewed-by: Vladimir Sementsov-Ogievskiy > > > --- > > include/block/block-io.h | 8 +++ > > block.c | 103 ++- > > block/io.c | 2 +- > > tests/unit/test-bdrv-drain.c | 10 > > 4 files changed, 108 insertions(+), 15 deletions(-) > > > > diff --git a/include/block/block-io.h b/include/block/block-io.h > > index 8f5e75756a..65e6d2569b 100644 > > --- a/include/block/block-io.h > > +++ b/include/block/block-io.h > > @@ -292,6 +292,14 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector > > *qiov, int64_t pos); > >*/ > > void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll); > > +/** > > + * bdrv_parent_drained_poll_single: > > + * > > + * Returns true if there is any pending activity to cease before @c can be > > + * called quiesced, false otherwise. > > + */ > > +bool bdrv_parent_drained_poll_single(BdrvChild *c); > > + > > /** > >* bdrv_parent_drained_end_single: > >* > > diff --git a/block.c b/block.c > > index d18512944d..3f12aba6ce 100644 > > --- a/block.c > > +++ b/block.c > > [..] > > > @@ -2863,11 +2905,9 @@ static void bdrv_replace_child_noperm(BdrvChild > > *child, > > } > > /* > > - * If the old child node was drained but the new one is not, allow > > - * requests to come in only after the new node has been attached. > > - * > > - * Update new_bs_quiesce_counter because > > bdrv_parent_drained_begin_single() > > - * polls, which could have changed the value. > > + * If the parent was drained through this BdrvChild previously, but > > new_bs > > + * is not drained, allow requests to come in only after the new node > > has > > + * been attached. > > As I understand,the main reason why we MUST do this automatic undrain, is the > contract with the user: > > User things that: > > 1. It's enough to drain node X to drain all its parents (thanks to recursion) > 2. User should undrain exactly same nodes that was drained by hand, > everything that was drained automatically would be automatically undrained. > > So here we break the connection between X and its parent, therefore recursion > will not help on final undrain. So, we should do the automation here. Yes, I think that's the idea behind our interface. > I have an idea how to (probably) make things even more simple. > > 1. drop .quiesced_parent > > 2. consider the Full graph, including non-bds parents, and support having > .quiesce_counter for non-bds parents. (probably need some structure to unify > bds and non-bds nodes of the Full graph - Generic ndoe, that's not the first > time we are saying about that) > > 3. drop any recursion and automatic drain/undrain > > 4. user is responsible to drain all the nodes and their parents as needed to > proceed with some block graph modification > > 5. user keeps the list of all nodes that was drained to undrain them in the > end > > 6. node may be drained only when all its parents are already drained (add an > assertion) > > And of course we need helpers for the user to do 4-6. That should work > similar to permissions update. Add a function to produce a topologically > sorted list of Generic nodes (starting from some node and include all its > parents and their parents and so on), and simple functions that to > drain/undrain of such list in a loop. I understand your idea and it looks nice at the first sight. However, on second thought, I'm not sure how easy and nice this would actually turn out: You will lose the invariant that if a node is drained, its parent will always be drained, too - it depends on the caller now. You also can't delete a node that is still drained, you need to keep it around for undraining. Same thing with graph modifications in a drained section: You won't automatically get a consistent state regarding drain in the new graph layout, so you would have to manually make sure that both old and new children are drained. It feels like this will lead to new complications that might not be any easier to manage than the old ones. Kevin
Re: [PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
On 11/18/22 20:41, Kevin Wolf wrote: In order to make sure that bdrv_replace_child_noperm() doesn't have to poll any more, get rid of the bdrv_parent_drained_begin_single() call. This is possible now because we can require that the parent is already drained through the child in question when the function is called and we don't call the parent drain callbacks more than once. The additional drain calls needed in callers cause the test case to run its code in the drain handler too early (bdrv_attach_child() drains now), so modify it to only enable the code after the test setup has completed. Signed-off-by: Kevin Wolf It's still hard to keep this all in mind, so weak: Reviewed-by: Vladimir Sementsov-Ogievskiy --- include/block/block-io.h | 8 +++ block.c | 103 ++- block/io.c | 2 +- tests/unit/test-bdrv-drain.c | 10 4 files changed, 108 insertions(+), 15 deletions(-) diff --git a/include/block/block-io.h b/include/block/block-io.h index 8f5e75756a..65e6d2569b 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -292,6 +292,14 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); */ void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll); +/** + * bdrv_parent_drained_poll_single: + * + * Returns true if there is any pending activity to cease before @c can be + * called quiesced, false otherwise. + */ +bool bdrv_parent_drained_poll_single(BdrvChild *c); + /** * bdrv_parent_drained_end_single: * diff --git a/block.c b/block.c index d18512944d..3f12aba6ce 100644 --- a/block.c +++ b/block.c [..] @@ -2863,11 +2905,9 @@ static void bdrv_replace_child_noperm(BdrvChild *child, } /* - * If the old child node was drained but the new one is not, allow - * requests to come in only after the new node has been attached. - * - * Update new_bs_quiesce_counter because bdrv_parent_drained_begin_single() - * polls, which could have changed the value. + * If the parent was drained through this BdrvChild previously, but new_bs + * is not drained, allow requests to come in only after the new node has + * been attached. As I understand,the main reason why we MUST do this automatic undrain, is the contract with the user: User things that: 1. It's enough to drain node X to drain all its parents (thanks to recursion) 2. User should undrain exactly same nodes that was drained by hand, everything that was drained automatically would be automatically undrained. So here we break the connection between X and its parent, therefore recursion will not help on final undrain. So, we should do the automation here. I have an idea how to (probably) make things even more simple. 1. drop .quiesced_parent 2. consider the Full graph, including non-bds parents, and support having .quiesce_counter for non-bds parents. (probably need some structure to unify bds and non-bds nodes of the Full graph - Generic ndoe, that's not the first time we are saying about that) 3. drop any recursion and automatic drain/undrain 4. user is responsible to drain all the nodes and their parents as needed to proceed with some block graph modification 5. user keeps the list of all nodes that was drained to undrain them in the end 6. node may be drained only when all its parents are already drained (add an assertion) And of course we need helpers for the user to do 4-6. That should work similar to permissions update. Add a function to produce a topologically sorted list of Generic nodes (starting from some node and include all its parents and their parents and so on), and simple functions that to drain/undrain of such list in a loop. -- Best regards, Vladimir
[PATCH v2 14/15] block: Don't poll in bdrv_replace_child_noperm()
In order to make sure that bdrv_replace_child_noperm() doesn't have to
poll any more, get rid of the bdrv_parent_drained_begin_single() call.
This is possible now because we can require that the parent is already
drained through the child in question when the function is called and we
don't call the parent drain callbacks more than once.
The additional drain calls needed in callers cause the test case to run
its code in the drain handler too early (bdrv_attach_child() drains
now), so modify it to only enable the code after the test setup has
completed.
Signed-off-by: Kevin Wolf
---
include/block/block-io.h | 8 +++
block.c | 103 ++-
block/io.c | 2 +-
tests/unit/test-bdrv-drain.c | 10
4 files changed, 108 insertions(+), 15 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index 8f5e75756a..65e6d2569b 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -292,6 +292,14 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector
*qiov, int64_t pos);
*/
void bdrv_parent_drained_begin_single(BdrvChild *c, bool poll);
+/**
+ * bdrv_parent_drained_poll_single:
+ *
+ * Returns true if there is any pending activity to cease before @c can be
+ * called quiesced, false otherwise.
+ */
+bool bdrv_parent_drained_poll_single(BdrvChild *c);
+
/**
* bdrv_parent_drained_end_single:
*
diff --git a/block.c b/block.c
index d18512944d..3f12aba6ce 100644
--- a/block.c
+++ b/block.c
@@ -2409,6 +2409,20 @@ static void bdrv_replace_child_abort(void *opaque)
GLOBAL_STATE_CODE();
/* old_bs reference is transparently moved from @s to @s->child */
+if (!s->child->bs) {
+/*
+ * The parents were undrained when removing old_bs from the child. New
+ * requests can't have been made, though, because the child was empty.
+ *
+ * TODO Make bdrv_replace_child_noperm() transactionable to avoid
+ * undraining the parent in the first place. Once this is done, having
+ * new_bs drained when calling bdrv_replace_child_tran() is not a
+ * requirement any more.
+ */
+bdrv_parent_drained_begin_single(s->child, false);
+assert(!bdrv_parent_drained_poll_single(s->child));
+}
+assert(s->child->quiesced_parent);
bdrv_replace_child_noperm(s->child, s->old_bs);
bdrv_unref(new_bs);
}
@@ -2424,12 +2438,19 @@ static TransactionActionDrv bdrv_replace_child_drv = {
*
* Note: real unref of old_bs is done only on commit.
*
+ * Both @child->bs and @new_bs (if non-NULL) must be drained. @new_bs must be
+ * kept drained until the transaction is completed.
+ *
* The function doesn't update permissions, caller is responsible for this.
*/
static void bdrv_replace_child_tran(BdrvChild *child, BlockDriverState *new_bs,
Transaction *tran)
{
BdrvReplaceChildState *s = g_new(BdrvReplaceChildState, 1);
+
+assert(child->quiesced_parent);
+assert(!new_bs || new_bs->quiesce_counter);
+
*s = (BdrvReplaceChildState) {
.child = child,
.old_bs = child->bs,
@@ -2821,6 +2842,14 @@ uint64_t bdrv_qapi_perm_to_blk_perm(BlockPermission
qapi_perm)
return permissions[qapi_perm];
}
+/*
+ * Replaces the node that a BdrvChild points to without updating permissions.
+ *
+ * If @new_bs is non-NULL, the parent of @child must already be drained through
+ * @child.
+ *
+ * This function does not poll.
+ */
static void bdrv_replace_child_noperm(BdrvChild *child,
BlockDriverState *new_bs)
{
@@ -2828,6 +2857,28 @@ static void bdrv_replace_child_noperm(BdrvChild *child,
int new_bs_quiesce_counter;
assert(!child->frozen);
+
+/*
+ * If we want to change the BdrvChild to point to a drained node as its new
+ * child->bs, we need to make sure that its new parent is drained, too. In
+ * other words, either child->quiesce_parent must already be true or we
must
+ * be able to set it and keep the parent's quiesce_counter consistent with
+ * that, but without polling or starting new requests (this function
+ * guarantees that it doesn't poll, and starting new requests would be
+ * against the invariants of drain sections).
+ *
+ * To keep things simple, we pick the first option (child->quiesce_parent
+ * must already be true). We also generalise the rule a bit to make it
+ * easier to verify in callers and more likely to be covered in test cases:
+ * The parent must be quiesced through this child even if new_bs isn't
+ * currently drained.
+ *
+ * The only exception is for callers that always pass new_bs == NULL. In
+ * this case, we obviously never need to consider the case of a drained
+ * new_bs, so we can keep the callers simpler by allowing them not to drain
+ * the parent.
+ */
+assert(!new_bs ||
