On 10.11.21 14:38, Vladimir Sementsov-Ogievskiy wrote:
04.11.2021 13:38, Hanna Reitz wrote:
bdrv_attach_child_common_abort() restores the parent's AioContext. To
do so, the child (which was supposed to be attached, but is now detached
again by this abort handler) is added to the ignore list for the
AioContext changing functions.
However, since we modify a BDS's children list in the BdrvChildClass's
.attach and .detach handlers, the child is already effectively detached
from the parent by this point. We do not need to put it into the ignore
list.
Use this opportunity to clean up the empty line structure: Keep setting
the ignore list, invoking the AioContext function, and freeing the
ignore list in blocks separated by empty lines.
Signed-off-by: Hanna Reitz <hre...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com>
What comes in my mind, is that now bdrv_replace_child_noperm() is more
strong in detaching.. But may be not enough strong: we still have link
from the child to parent (child->opaque).. Maybe more correct for
BdrvChild object to have no relation with parent after detaching. But
than we'll need some GenericParent object (as child->class is mostly
about parent, not about child and even not about the edge).. Now
GenericParent is "included" into BdrvChild, which represents both
GenericParent and Edge objects..
Yes, I thought about this in exactly this function here
(bdrv_attach_child_common_abort()). I was wondering whether I could
save a pointer to the BdrvChildClass, then just let
bdrv_replace_child_noperm() free the child, and then invoke the
BdrvChildClass methods when the child is already gone. As you say, it’s
really just about the parent, and child->opaque is effectively just a
pointer to the parent object, so all of the methods that only use
child->opaque and nothing else from the BdrvChild object can be invoked
even after the child is gone.
But doing something about that (like changing some methods like the
AioContext methods to only take the opaque pointer) was too invasive for
me for this series, so in case of this function I decided to just
begrudgingly keep the BdrvChild object around, including its
back-connection to the parent (via child->opaque), and free it at the
end of the function.
Besides that back-connection, there’s also to consider that some block
drivers can keep pointers to special children in their BDS
“subclasses”. For example, qcow2 keeps s->data_file. That’s a problem
just like bs->backing or bs->file, except that it’s much more unlikely
to cause problems in practice, and that it would probably be even more
invasive to fix...
Hanna
---
block.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/block.c b/block.c
index b95f8dcf8f..6d230ad3d1 100644
--- a/block.c
+++ b/block.c
@@ -2774,14 +2774,16 @@ static void
bdrv_attach_child_common_abort(void *opaque)
}
if (bdrv_child_get_parent_aio_context(child) !=
s->old_parent_ctx) {
- GSList *ignore = g_slist_prepend(NULL, child);
+ GSList *ignore;
+ /* No need to ignore `child`, because it has been detached
already */
+ ignore = NULL;
child->klass->can_set_aio_ctx(child, s->old_parent_ctx,
&ignore,
&error_abort);
g_slist_free(ignore);
- ignore = g_slist_prepend(NULL, child);
- child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
+ ignore = NULL;
+ child->klass->set_aio_ctx(child, s->old_parent_ctx, &ignore);
g_slist_free(ignore);
}
