qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media


From: Max Reitz
Subject: Re: [Qemu-devel] [PATCH 00/50] blockdev: BlockBackend and media
Date: Tue, 27 Jan 2015 14:38:49 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0

On 2015-01-27 at 14:30, Eric Blake wrote:
On 01/26/2015 09:02 AM, Max Reitz wrote:
This series reworks a lot regarding BlockBackend and media. It is
essentially a v3 to the "blockdev: Add blockdev-change-medium with
read-only option" series (which is in fact a part of this series), but
of course does a lot more.

I'm not done reviewing yet, but making some comments here as I go.

This series depends on v3 (or any later version) of my series
'block: Remove "growable", add blk_new_open()'.
So I reviewed that first.


- Patches 1 and 2 make it possible to use blockdev-add without creating
   a BlockBackend. This is necessary because the QMP command introduced
   in patch 42 (blockdev-insert-medium) will insert a BDS tree (a medium)
   into a BlockBackend (a drive). Creating a BlockBackend for such a BDS
   tree would both be a hassle and a waste, so this makes it possible to
   omit creating a BB.
Basically, making blockdev-add be capable of creating a detached BDS
tree for later attachment to some device or as a branch in an even
larger BDS tree.  Seems reasonable enough; I still haven't made up my
mind if we need some form of introspection to know if this usage is
supported (but since later in the series you are adding new commands
that depend on this, that may be a good enough witness).

Well, one form of introspection is just assuming that this usage is supported and if it doesn't work (you're trying to execute blockdev-add without an ID and it fails), fall back to the mem-leaking variant (just use blockdev-add with a superfluous BlockBackend) and remember that support is missing.

- Patch 6 makes blk_is_inserted() return true only if there is a BDS
   tree; furthermore, blk_is_available() is added which additionally
   checks whether the guest device tray is closed.

- Patches 7 and 8 are some kind of follow-up to patch 6; they make
   bdrv_is_inserted() actually useful (unless I'm not mistaken; maybe I
   am and they make bdrv_is_inserted() actually useless).
Does it even make sense to have a quorum that is mixed between a CDROM
passthrough drive (where medium can be removed) and on-disk/network
locations (where medium is unchangeable)?

Hm, good question.

But your idea makes sense - a
tree is inserted if all of its parts are also inserted, and removing any
one medium anywhere in the BDS tree makes it pointless to use all other
parts of the tree that feed the same BB.  Do we need some sort of
operation blocker that prevents eject on a BDS that feeds a higher-level
node of a BDS tree?

I'd be completely fine with setting up an eject blocker for Quorum children, it does make sense to me.

Max

Or if you want to read that paragraph differently: I reviewed the
patches for coding bugs and found none, but I'm fuzzy enough on design
details that I'd appreciate a second review from an interested party to
make sure I'm not overlooking a design flaw in your change.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]