Re: [Qemu-devel] [PATCH v6 for 2.1 00/10] Modify block jobs to use node-names

[Kevin Wolf]

Am 19.06.2014 um 18:49 hat Eric Blake geschrieben:
> On 06/19/2014 10:26 AM, Jeff Cody wrote:
> >> b. Is it a good idea to perform op blocker checks on the root node?
> >>    It's inconsistent with resize, snapshot-sync, etc.  Permissions in
> >>    BDS graphs with multiple root nodes (e.g. guest device and NBD
> >>    run-time server) will be different depending on which root you
> >>    specify.
> > 
> > I don't think (b) is the ultimate solution.  It is used as a stop-gap
> > because op blockers in the current implementation is essentially
> > analogous to the in-use flag.  But is it good enough for 2.1?  If
> > *everything* checks the topmost node in 2.1, then I think we are OK in
> > all cases except where images files share a common BDS.

What's the problem again with just propagating all blockers down the
chain using bs->backing_hd and then checking just the affected nodes
rather than trying to figure out a root? It would probably be more
restrictive than necessary, but it looked like a safe first step.

> > The ability for internal BDSs to share a common base BDS makes some
> > block jobs unsafe currently, I believe.  A crude and ugly fix is to
> > only allow a single block-job to occur at any given time, but that
> > doesn't seem feasible, so let's ignore that.
> 
> Can we even create a common base BDS in qemu 2.1?  I know that the
> blockdev-add command was designed with this possibility in mind, but to
> my knowledge, we have not actually wired it up yet.

You can create such chains, I just tried it:

x86_64-softmmu/qemu-system-x86_64 \
    -drive if=none,id=backing,file=/tmp/backing.qcow2 \
    -drive file=/tmp/empty.qcow2,backing.file=backing \
    -drive file=/tmp/empty2.qcow2,backing.file=backing

Gives me the same disk twice, and I can independently change them as you
would expect.

> >> The answer seems to be that op blockers should be propagated to all
> >> child nodes and commands should test the node, not the drive/root node.
> >> That gives us the flexibility for per-node op blockers in the future and
> >> maintains compatibility with existing node-name users.
> >>
> > 
> > Long term thoughts:
> > 
> > So if I think of operations that are done on block devices from a
> > block job, and chuck them into categories, I think we have:
> > 
> > 1) Read of guest-visible data
> > 2) Write of guest-visible data
> > 3) Read of host-visible data (e.g. image file metadata)
> > 4) Write of host-visible data (e.g. image file metadata, such as
> > the backing-file)
> > 5) Block chain manipulations (e.g. movement of a BDS, change to r/w
> > instead of r/o, etc..)
> > 6) I/O attribute changes (e.g. throttling, etc..)
> > 
> > Does this make sense, and did I miss any (likely so)?  It seems like
> > we should issue blockers not based on specific commands (e.g.
> > BLOCK_OP_TYPE_COMMIT), but rather based on what specific category of
> > interaction on a BDS we want to prohibit / allow.
> 
> Yes, I like that train of thought - it's not the operation that we are
> blocking, but the access category(ies) required by that operation.

Yes, I argued something similar in our block meeting in Stuttgart, and I
still think it's a good idea in principle. The problem with it is that
the right categories aren't quite obvious.

For example, I think guest/host writes really means writes that change
the visible contents of this image. A shared backing file would require
that its contents never changes, but that's not really related to the
guest. A commit operation does a "guest" write on the base node because
its contents changes when you look only at the specific node.

Kevin

pgpLIA0k5jhJJ.pgp
Description: PGP signature