| Message ID | 20211104103849.46855-3-hreitz@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: Attempt on fixing 030-reported errors | expand |
04.11.2021 13:38, Hanna Reitz wrote: > The children list is specific to BDS parents. We should not modify it > in the general children modification code, but let BDS parents deal with > it in their .attach() and .detach() methods. > > This also has the advantage that a BdrvChild is removed from the > children list before its .bs pointer can become NULL. BDS parents > generally assume that their children's .bs pointer is never NULL, so > this is actually a bug fix. > > Signed-off-by: Hanna Reitz <hreitz@redhat.com> > --- > block.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/block.c b/block.c > index 580cb77a70..243ae206b5 100644 > --- a/block.c > +++ b/block.c > @@ -1387,6 +1387,8 @@ static void bdrv_child_cb_attach(BdrvChild *child) > { > BlockDriverState *bs = child->opaque; > > + QLIST_INSERT_HEAD(&bs->children, child, next); > + > if (child->role & BDRV_CHILD_COW) { > bdrv_backing_attach(child); > } > @@ -1403,6 +1405,8 @@ static void bdrv_child_cb_detach(BdrvChild *child) > } > > bdrv_unapply_subtree_drain(child, bs); > + > + QLIST_REMOVE(child, next); > } > > static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base, > @@ -2747,7 +2751,7 @@ static void bdrv_child_free(void *opaque) > static void bdrv_remove_empty_child(BdrvChild *child) > { > assert(!child->bs); > - QLIST_SAFE_REMOVE(child, next); > + assert(!child->next.le_prev); /* not in children list */ > bdrv_child_free(child); > } > > @@ -2913,7 +2917,6 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, > return ret; > } > > - QLIST_INSERT_HEAD(&parent_bs->children, *child, next); The following comment become stale. We should remove it too. With comment removed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > /* > * child is removed in bdrv_attach_child_common_abort(), so don't care to > * abort this change separately. > @@ -4851,7 +4854,6 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) > BdrvRemoveFilterOrCowChild *s = opaque; > BlockDriverState *parent_bs = s->child->opaque; > > - QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); > if (s->is_backing) { > parent_bs->backing = s->child; > } else { > @@ -4906,7 +4908,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, > }; > tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); > > - QLIST_SAFE_REMOVE(child, next); > if (s->is_backing) { > bs->backing = NULL; > } else { >
04.11.2021 13:38, Hanna Reitz wrote: > The children list is specific to BDS parents. We should not modify it > in the general children modification code, but let BDS parents deal with > it in their .attach() and .detach() methods. > > This also has the advantage that a BdrvChild is removed from the > children list before its .bs pointer can become NULL. BDS parents > generally assume that their children's .bs pointer is never NULL, so > this is actually a bug fix. > > Signed-off-by: Hanna Reitz<hreitz@redhat.com> Interesting that nor child_root neither child_job do similar things in .attach / .detach ... Should we do something with it?
On 10.11.21 13:46, Vladimir Sementsov-Ogievskiy wrote: > 04.11.2021 13:38, Hanna Reitz wrote: >> The children list is specific to BDS parents. We should not modify it >> in the general children modification code, but let BDS parents deal with >> it in their .attach() and .detach() methods. >> >> This also has the advantage that a BdrvChild is removed from the >> children list before its .bs pointer can become NULL. BDS parents >> generally assume that their children's .bs pointer is never NULL, so >> this is actually a bug fix. >> >> Signed-off-by: Hanna Reitz <hreitz@redhat.com> >> --- >> block.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/block.c b/block.c >> index 580cb77a70..243ae206b5 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1387,6 +1387,8 @@ static void bdrv_child_cb_attach(BdrvChild *child) >> { >> BlockDriverState *bs = child->opaque; >> + QLIST_INSERT_HEAD(&bs->children, child, next); >> + >> if (child->role & BDRV_CHILD_COW) { >> bdrv_backing_attach(child); >> } >> @@ -1403,6 +1405,8 @@ static void bdrv_child_cb_detach(BdrvChild *child) >> } >> bdrv_unapply_subtree_drain(child, bs); >> + >> + QLIST_REMOVE(child, next); >> } >> static int bdrv_child_cb_update_filename(BdrvChild *c, >> BlockDriverState *base, >> @@ -2747,7 +2751,7 @@ static void bdrv_child_free(void *opaque) >> static void bdrv_remove_empty_child(BdrvChild *child) >> { >> assert(!child->bs); >> - QLIST_SAFE_REMOVE(child, next); >> + assert(!child->next.le_prev); /* not in children list */ >> bdrv_child_free(child); >> } >> @@ -2913,7 +2917,6 @@ static int >> bdrv_attach_child_noperm(BlockDriverState *parent_bs, >> return ret; >> } >> - QLIST_INSERT_HEAD(&parent_bs->children, *child, next); > > The following comment become stale. We should remove it too. With > comment removed: Ah, right, thanks! > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
On 10.11.21 13:51, Vladimir Sementsov-Ogievskiy wrote: > 04.11.2021 13:38, Hanna Reitz wrote: >> The children list is specific to BDS parents. We should not modify it >> in the general children modification code, but let BDS parents deal with >> it in their .attach() and .detach() methods. >> >> This also has the advantage that a BdrvChild is removed from the >> children list before its .bs pointer can become NULL. BDS parents >> generally assume that their children's .bs pointer is never NULL, so >> this is actually a bug fix. >> >> Signed-off-by: Hanna Reitz<hreitz@redhat.com> > > > Interesting that nor child_root neither child_job do similar things in > .attach / .detach ... Should we do something with it? Well, it’s up to them, I thought. :) A BB only has a single child, so it doesn’t need a list. Jobs do have their own child list (BlockJob.nodes). I thought a bit about this when writing this series, and I figured perhaps they don’t need to care about that in .attach() and .detach(), because they don’t really expect nodes to be detached or attached anyway; child_job.stay_at_node is true, after all. Hanna
diff --git a/block.c b/block.c index 580cb77a70..243ae206b5 100644 --- a/block.c +++ b/block.c @@ -1387,6 +1387,8 @@ static void bdrv_child_cb_attach(BdrvChild *child) { BlockDriverState *bs = child->opaque; + QLIST_INSERT_HEAD(&bs->children, child, next); + if (child->role & BDRV_CHILD_COW) { bdrv_backing_attach(child); } @@ -1403,6 +1405,8 @@ static void bdrv_child_cb_detach(BdrvChild *child) } bdrv_unapply_subtree_drain(child, bs); + + QLIST_REMOVE(child, next); } static int bdrv_child_cb_update_filename(BdrvChild *c, BlockDriverState *base, @@ -2747,7 +2751,7 @@ static void bdrv_child_free(void *opaque) static void bdrv_remove_empty_child(BdrvChild *child) { assert(!child->bs); - QLIST_SAFE_REMOVE(child, next); + assert(!child->next.le_prev); /* not in children list */ bdrv_child_free(child); } @@ -2913,7 +2917,6 @@ static int bdrv_attach_child_noperm(BlockDriverState *parent_bs, return ret; } - QLIST_INSERT_HEAD(&parent_bs->children, *child, next); /* * child is removed in bdrv_attach_child_common_abort(), so don't care to * abort this change separately. @@ -4851,7 +4854,6 @@ static void bdrv_remove_filter_or_cow_child_abort(void *opaque) BdrvRemoveFilterOrCowChild *s = opaque; BlockDriverState *parent_bs = s->child->opaque; - QLIST_INSERT_HEAD(&parent_bs->children, s->child, next); if (s->is_backing) { parent_bs->backing = s->child; } else { @@ -4906,7 +4908,6 @@ static void bdrv_remove_file_or_backing_child(BlockDriverState *bs, }; tran_add(tran, &bdrv_remove_filter_or_cow_child_drv, s); - QLIST_SAFE_REMOVE(child, next); if (s->is_backing) { bs->backing = NULL; } else {
The children list is specific to BDS parents. We should not modify it in the general children modification code, but let BDS parents deal with it in their .attach() and .detach() methods. This also has the advantage that a BdrvChild is removed from the children list before its .bs pointer can become NULL. BDS parents generally assume that their children's .bs pointer is never NULL, so this is actually a bug fix. Signed-off-by: Hanna Reitz <hreitz@redhat.com> --- block.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)