| Message ID | 0d86a074935ca6c1a0645e5d4af492c7cac12821.1435758248.git.berto@igalia.com |
|---|---|
| State | New |
| Headers | show |
On 01.07.2015 16:21, Alberto Garcia wrote: > When a backing image is opened using bdrv_open_inherit(), it is added > to the parent image's list of children. However there's no way to > remove it from there. > > In particular, changing a BlockDriverState's backing image does not > add the new one to the list nor removes the old one. If the latter is > closed then the pointer in the list becomes invalid. This can be > reproduced easily using the block-stream command. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > Cc: Kevin Wolf <kwolf@redhat.com> > --- > block.c | 40 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 7e130cc..eaf3ad0 100644 > --- a/block.c > +++ b/block.c > @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > const BdrvChildRole *child_role, > BlockDriver *drv, Error **errp); > > +static void bdrv_attach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs, > + const BdrvChildRole *child_role); > + > +static void bdrv_detach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs); > + > static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > @@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > if (bs->backing_hd) { > assert(bs->backing_blocker); > bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > + bdrv_detach_child(bs, bs->backing_hd); > } else if (backing_hd) { > error_setg(&bs->backing_blocker, > "node is used as backing hd of '%s'", > @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > bs->backing_blocker = NULL; > goto out; > } > + > + bdrv_attach_child(bs, backing_hd, &child_backing); > + backing_hd->inherits_from = bs; > + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); Do we really want this, unconditionally? ... After looking through the code, I can't find a place where we wouldn't. It just seems strange to have it here. > + > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, > BlockDriverState *child_bs, > const BdrvChildRole *child_role) > { > - BdrvChild *child = g_new(BdrvChild, 1); > + BdrvChild *child; > + > + /* Don't attach the child if it's already attached */ > + QLIST_FOREACH(child, &parent_bs->children, next) { > + if (child->bs == child_bs) { > + return; > + } > + } Hm, it may have been attached with a different role, though... I guess that would be a bug, however. But if it's the same role, trying to re-attach it seems wrong, too. So where could this happen? Max > + > + child = g_new(BdrvChild, 1); > *child = (BdrvChild) { > .bs = child_bs, > .role = child_role, > @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, > QLIST_INSERT_HEAD(&parent_bs->children, child, next); > } > > +static void bdrv_detach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs) > +{ > + BdrvChild *child, *next_child; > + QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) { > + if (child->bs == child_bs) { > + if (child->bs->inherits_from == parent_bs) { > + child->bs->inherits_from = NULL; > + } > + QLIST_REMOVE(child, next); > + g_free(child); > + } > + } > +} > + > /* > * Opens a disk image (raw, qcow2, vmdk, ...) > * > @@ -2116,7 +2153,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) > /* The contents of 'tmp' will become bs_top, as we are > * swapping bs_new and bs_top contents. */ > bdrv_set_backing_hd(bs_top, bs_new); > - bdrv_attach_child(bs_top, bs_new, &child_backing); > } > > static void bdrv_delete(BlockDriverState *bs) >
On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mreitz@redhat.com> wrote: >> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) >> bs->backing_blocker = NULL; >> goto out; >> } >> + >> + bdrv_attach_child(bs, backing_hd, &child_backing); >> + backing_hd->inherits_from = bs; >> + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); > > Do we really want this, unconditionally? ... After looking through the > code, I can't find a place where we wouldn't. It just seems strange to > have it here. Yeah, I understand. In general I think that the API for handling bs->children is rather unclear and I wanted to avoid that callers need to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately. >> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, >> BlockDriverState *child_bs, >> const BdrvChildRole *child_role) >> { >> - BdrvChild *child = g_new(BdrvChild, 1); >> + BdrvChild *child; >> + >> + /* Don't attach the child if it's already attached */ >> + QLIST_FOREACH(child, &parent_bs->children, next) { >> + if (child->bs == child_bs) { >> + return; >> + } >> + } > > Hm, it may have been attached with a different role, though... I guess > that would be a bug, however. But if it's the same role, trying to > re-attach it seems wrong, too. So where could this happen? The reason I'm doing this is because of bdrv_open_backing_file(). That function attaches the backing file to the parent file twice: once in bdrv_open_inherit() and the second time in bdrv_set_backing_hd(). One alternative would be not to attach it in bdrv_set_backing_hd(), but since that function is used in many other places we would have to add new calls to bdrv_attach_child() everywhere. That's one example of the situation I mentioned earlier: it seems logical that bdrv_set_backing_hd() and bdrv_attach_child() go together, but none of the solutions that came to my mind feels 100% right. Berto
On 01.07.2015 21:41, Alberto Garcia wrote: > On Wed 01 Jul 2015 06:05:32 PM CEST, Max Reitz <mreitz@redhat.com> wrote: > >>> @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) >>> bs->backing_blocker = NULL; >>> goto out; >>> } >>> + >>> + bdrv_attach_child(bs, backing_hd, &child_backing); >>> + backing_hd->inherits_from = bs; >>> + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); >> >> Do we really want this, unconditionally? ... After looking through the >> code, I can't find a place where we wouldn't. It just seems strange to >> have it here. > > Yeah, I understand. In general I think that the API for handling > bs->children is rather unclear and I wanted to avoid that callers need > to call bdrv_set_backing_hd() and bdrv_attach/detach_child() separately. Oh, sorry, I was unclear. The bdrv_attach_child() is fine, but I find unconditionally inheriting the flags from the backed BDS strange. >>> @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, >>> BlockDriverState *child_bs, >>> const BdrvChildRole *child_role) >>> { >>> - BdrvChild *child = g_new(BdrvChild, 1); >>> + BdrvChild *child; >>> + >>> + /* Don't attach the child if it's already attached */ >>> + QLIST_FOREACH(child, &parent_bs->children, next) { >>> + if (child->bs == child_bs) { >>> + return; >>> + } >>> + } >> >> Hm, it may have been attached with a different role, though... I guess >> that would be a bug, however. But if it's the same role, trying to >> re-attach it seems wrong, too. So where could this happen? > > The reason I'm doing this is because of bdrv_open_backing_file(). That > function attaches the backing file to the parent file twice: once in > bdrv_open_inherit() and the second time in bdrv_set_backing_hd(). Okay, that's fine then. > One alternative would be not to attach it in bdrv_set_backing_hd(), but > since that function is used in many other places we would have to add > new calls to bdrv_attach_child() everywhere. > > That's one example of the situation I mentioned earlier: it seems > logical that bdrv_set_backing_hd() and bdrv_attach_child() go together, > but none of the solutions that came to my mind feels 100% right. I think putting it into bdrv_set_backing_hd() is fine. Still feeling a bit bad about overwriting the backing BDS's flags and making it inherit its flags from the backed BDS in bdrv_set_backing_hd(), but anyway: Reviewed-by: Max Reitz <mreitz@redhat.com> (I do think it is fine and can't think of any better solution)
Am 01.07.2015 um 16:21 hat Alberto Garcia geschrieben: > When a backing image is opened using bdrv_open_inherit(), it is added > to the parent image's list of children. However there's no way to > remove it from there. > > In particular, changing a BlockDriverState's backing image does not > add the new one to the list nor removes the old one. If the latter is > closed then the pointer in the list becomes invalid. This can be > reproduced easily using the block-stream command. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > Cc: Kevin Wolf <kwolf@redhat.com> I think I have a fix for this as part of a larger series I've been working on before I left for holidays. I intended to send it out before that, but I couldn't get it finished in time. http://repo.or.cz/w/qemu/kevin.git/shortlog/refs/heads/bdrv_swap Commit cde08581 'block: Fix backing file child when modifying graph' It seems cleaner to me than this patch, though I haven't tried yet to split the series so that it could be applied to 2.4. > block.c | 40 ++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 38 insertions(+), 2 deletions(-) > > diff --git a/block.c b/block.c > index 7e130cc..eaf3ad0 100644 > --- a/block.c > +++ b/block.c > @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, > const BdrvChildRole *child_role, > BlockDriver *drv, Error **errp); > > +static void bdrv_attach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs, > + const BdrvChildRole *child_role); > + > +static void bdrv_detach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs); > + > static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); > /* If non-zero, use only whitelisted block drivers */ > static int use_bdrv_whitelist; > @@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > if (bs->backing_hd) { > assert(bs->backing_blocker); > bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); > + bdrv_detach_child(bs, bs->backing_hd); > } else if (backing_hd) { > error_setg(&bs->backing_blocker, > "node is used as backing hd of '%s'", > @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) > bs->backing_blocker = NULL; > goto out; > } > + > + bdrv_attach_child(bs, backing_hd, &child_backing); > + backing_hd->inherits_from = bs; > + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); This looks wrong to me. Attaching a BDS as a backing file doesn't mean that the (potentially explicitly set) options and flags for that BDS should be changed. It's perfectly fine for children to not inherit options from their parent. > bs->open_flags &= ~BDRV_O_NO_BACKING; > pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); > pstrcpy(bs->backing_format, sizeof(bs->backing_format), > @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, > BlockDriverState *child_bs, > const BdrvChildRole *child_role) > { > - BdrvChild *child = g_new(BdrvChild, 1); > + BdrvChild *child; > + > + /* Don't attach the child if it's already attached */ > + QLIST_FOREACH(child, &parent_bs->children, next) { > + if (child->bs == child_bs) { > + return; > + } > + } In theory, it could be valid to attach the same BDS for two different roles of the same parent. > + child = g_new(BdrvChild, 1); > *child = (BdrvChild) { > .bs = child_bs, > .role = child_role, > @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, > QLIST_INSERT_HEAD(&parent_bs->children, child, next); > } > > +static void bdrv_detach_child(BlockDriverState *parent_bs, > + BlockDriverState *child_bs) > +{ > + BdrvChild *child, *next_child; > + QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) { > + if (child->bs == child_bs) { > + if (child->bs->inherits_from == parent_bs) { > + child->bs->inherits_from = NULL; > + } > + QLIST_REMOVE(child, next); > + g_free(child); > + } > + } > +} For the same reason, BlockDriverState *child_bs is a bad interface. My patches use BdrvChild *child instead. Kevin
diff --git a/block.c b/block.c index 7e130cc..eaf3ad0 100644 --- a/block.c +++ b/block.c @@ -88,6 +88,13 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename, const BdrvChildRole *child_role, BlockDriver *drv, Error **errp); +static void bdrv_attach_child(BlockDriverState *parent_bs, + BlockDriverState *child_bs, + const BdrvChildRole *child_role); + +static void bdrv_detach_child(BlockDriverState *parent_bs, + BlockDriverState *child_bs); + static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs); /* If non-zero, use only whitelisted block drivers */ static int use_bdrv_whitelist; @@ -1108,6 +1115,7 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) if (bs->backing_hd) { assert(bs->backing_blocker); bdrv_op_unblock_all(bs->backing_hd, bs->backing_blocker); + bdrv_detach_child(bs, bs->backing_hd); } else if (backing_hd) { error_setg(&bs->backing_blocker, "node is used as backing hd of '%s'", @@ -1120,6 +1128,11 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd) bs->backing_blocker = NULL; goto out; } + + bdrv_attach_child(bs, backing_hd, &child_backing); + backing_hd->inherits_from = bs; + backing_hd->open_flags = child_backing.inherit_flags(bs->open_flags); + bs->open_flags &= ~BDRV_O_NO_BACKING; pstrcpy(bs->backing_file, sizeof(bs->backing_file), backing_hd->filename); pstrcpy(bs->backing_format, sizeof(bs->backing_format), @@ -1332,7 +1345,16 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, const BdrvChildRole *child_role) { - BdrvChild *child = g_new(BdrvChild, 1); + BdrvChild *child; + + /* Don't attach the child if it's already attached */ + QLIST_FOREACH(child, &parent_bs->children, next) { + if (child->bs == child_bs) { + return; + } + } + + child = g_new(BdrvChild, 1); *child = (BdrvChild) { .bs = child_bs, .role = child_role, @@ -1341,6 +1363,21 @@ static void bdrv_attach_child(BlockDriverState *parent_bs, QLIST_INSERT_HEAD(&parent_bs->children, child, next); } +static void bdrv_detach_child(BlockDriverState *parent_bs, + BlockDriverState *child_bs) +{ + BdrvChild *child, *next_child; + QLIST_FOREACH_SAFE(child, &parent_bs->children, next, next_child) { + if (child->bs == child_bs) { + if (child->bs->inherits_from == parent_bs) { + child->bs->inherits_from = NULL; + } + QLIST_REMOVE(child, next); + g_free(child); + } + } +} + /* * Opens a disk image (raw, qcow2, vmdk, ...) * @@ -2116,7 +2153,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top) /* The contents of 'tmp' will become bs_top, as we are * swapping bs_new and bs_top contents. */ bdrv_set_backing_hd(bs_top, bs_new); - bdrv_attach_child(bs_top, bs_new, &child_backing); } static void bdrv_delete(BlockDriverState *bs)
When a backing image is opened using bdrv_open_inherit(), it is added to the parent image's list of children. However there's no way to remove it from there. In particular, changing a BlockDriverState's backing image does not add the new one to the list nor removes the old one. If the latter is closed then the pointer in the list becomes invalid. This can be reproduced easily using the block-stream command. Signed-off-by: Alberto Garcia <berto@igalia.com> Cc: Kevin Wolf <kwolf@redhat.com> --- block.c | 40 ++++++++++++++++++++++++++++++++++++++-- 1 file changed, 38 insertions(+), 2 deletions(-)