From: Kevin Wolf <kwolf@redhat.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: stefanha@redhat.com, Alberto Garcia <berto@igalia.com>,
qemu-devel@nongnu.org, qemu-block@nongnu.org, mreitz@redhat.com
Subject: Re: [Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list
Date: Thu, 23 Jul 2015 10:14:14 +0200 [thread overview]
Message-ID: <20150723081414.GB4314@noname.redhat.com> (raw)
In-Reply-To: <87bnf3id4x.fsf@blackfin.pond.sub.org>
Am 23.07.2015 um 08:47 hat Markus Armbruster geschrieben:
> Alberto Garcia <berto@igalia.com> writes:
>
> > I've been debugging a couple of problems related to the recently
> > merged bdrv_reopen() overhaul code.
> >
> > 1. bs->children is not updated correctly
> > ----------------------------------------
> > The problem is described in this e-mail:
> >
> > https://lists.gnu.org/archive/html/qemu-devel/2015-06/msg06813.html
> >
> > In short, changing an image's backing hd does not replace the pointer
> > in the bs->children list.
> >
> > The children list is a feature added recently in 6e93e7c41fdfdee30.
> > In addition to bs->backing_hd and bs->file it also includes other
> > driver-specific children for cases like Quorum.
>
> I was involved in discussions leading up to the children list, but I
> couldn't find the time to review actual patches, so I only have
> high-level remarks at this time.
>
> Obvious: the BDS graph is linked together with pointers.
>
> Two pointers are special: BDS members @backing_hd and @file. By
> convention:
>
> 1. COW drivers use @file for the delta and @backing_hd for the base.
>
> Example: "qcow2" uses them that way.
>
> 2. Non-COW drivers don't use @backing_hd (it remains null), and they may
> use @file.
>
> Example: "raw" uses @file to point to its underlying BDS.
> Example: "file" doesn't use @file (it remains null).
>
> 3. If a driver needs children that don't fit the above, it has suitable
> members in its private state.
>
> Example "blkverify" has a member @test_file.
>
> Due to 3., generic code cannot find all children. This is unfortunate.
>
> Possible solutions:
>
> A. Driver callbacks to help iterate over children.
>
> B. Change the data structure so that generic code can iterate over
> children.
>
> C. Augment the data structure so that generic code can iterate over
> children.
>
> Solution A seems relatively straightforward to do, but clumsy. B is a
> lot of code churn, and the result might be less clear, say bs->child[0]
> and bs->child[1] instead of bs->file and bs->backing_hd. C begs
> consistency questions.
>
> Our children list is C. How do we ensure consistency?
By doing B.
> One way to make ensuring it easier is another level of indirection: have
> the children list nodes point to the child pointer instead of the child.
> Then we need to mess with the children list only initially, and when
> child pointers get born or die.
My series does it the other way round: bs->backing_file becomes a
pointer to BdrvChild.
> Without that, we need to mess with it when child pointers change. We
> can require all changes to go through a helper function that updates the
> children list. Perhaps something like
>
> void bdrv_set_child_internal(BlockDriverState *bs, size_t offs,
> BlockDriverState *new_child)
> {
> BlockDriverState **child_ptr = (BlockDriverState **)((char *)bs + offs);
>
> if (*child_ptr) {
> remove *child_ptr from bs->children
> }
> *child_ptr = new_child
> if (new_child) {
> add *child_ptr to bs->children
> }
> }
>
> #define bdrv_set_child(bs, member, new_child) \
> bdrv_set_child_internal(bs, offsetof(bs, member), new_child)
> // build time assertion bs->member is of type BlockDriverState * omitted
>
> Caution: when the same child occurs multiple times, this may destroy any
> association of "actual child pointer" with "position in child list".
This looks much too error-prone to me. We have to avoid duplication.
> Aside: why is it a children list and not an array? Are members deleted
> or inserted that often?
Not often, but not all at the same time either. Implementation detail,
feel free to convert it to reallocating an array each time.
> > 2. bdrv_reopen_queue() includes the complete backing chain
> > ----------------------------------------------------------
> > Calling bdrv_reopen() on a particular BlockDriverState now adds its
> > whole backing chain to the queue (formerly I think it was just
> > bs->file).
>
> I'm not sure I understand. Can you give an example with before and
> after behavior?
Backing chain: base <- snap
Start with cache=none set for snap. base inherits the option. Reopen
with cache=writeback. Before the change, base remained at cache=none,
afterwards it inherits the new setting cache=writeback.
As we defined that reopen should adjust the inherited options of child
nodes accordingly, this was a bug fix.
Kevin
prev parent reply other threads:[~2015-07-23 8:14 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-07-01 14:21 [Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list Alberto Garcia
2015-07-01 14:21 ` [Qemu-devel] [PATCH 1/1] block: update BlockDriverState's children in bdrv_set_backing_hd() Alberto Garcia
2015-07-01 16:05 ` Max Reitz
2015-07-01 19:41 ` Alberto Garcia
2015-07-04 16:13 ` Max Reitz
2015-07-07 14:49 ` Kevin Wolf
2015-07-23 6:47 ` [Qemu-devel] [PATCH 0/1] A couple of problems with BlockDriverState's children list Markus Armbruster
2015-07-23 8:14 ` Kevin Wolf [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150723081414.GB4314@noname.redhat.com \
--to=kwolf@redhat.com \
--cc=armbru@redhat.com \
--cc=berto@igalia.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=stefanha@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).