Re: [Qemu-block] 'drive-mirror' vs. 'blockdev-mirror' semantics

[Kashyap Chamarthy]

On Wed, Aug 17, 2016 at 11:22:55AM +0200, Kevin Wolf wrote:
> Am 16.08.2016 um 23:28 hat Kashyap Chamarthy geschrieben:
> > Hi,
> > 
> > From the QAPI schema documentation of 'blockdev-mirror':
> > 
> >   # @target: the id or node-name of the block device to mirror to. This
> >              mustn't be attached to guest.
> > 
> > Why must the target, prepared by 'blockdev-add', be not attached to the
> > guest?  (Side question: is this via 'device_add' HMP comand, which
> > handles the guest-visible "frontend" aspect?)  John mentioned on IRC
> > that this is may be something to do with Linux AIO contexts.
> 
> I think the real requirement is "must not be attached to a user that
> expects the device not to change under its feet". Guest devices (and
> more so guest OSes) happen to expect that their contents doesn't change
> between two reads except if they explicitly wrote to it.

Yes, that wording makes it lot clearer.  Thanks.
 
> > From the DriveMirror structure documentation (and from my own tests of
> > 'drive-mirror' to different targets -- local file, NBD over
> > UnixSocketAddress, NBD over InetSocketAddress) I don't see such a
> > limitation.
> 
> drive-mirror creates a new block device (i.e. opens the image), so it's
> obvious that no guest device could possibly be attached to it yet.

Ah, noted.

[...]

> > .---------------------------------------------------------------------.
> > |     drive-mirror                  |  blockdev-mirror                |
> > ----------------------------------------------------------------------
> > | 'target' will be created if not   | 'target' needs to be explicitly |
> > | specified; or an existing target  | created by `blockdev-add`, which|
> > |  can be used if it exists         | assigns a name to the to-be     |
> > |                                   | created node                    |
> 
> Careful with the word "create" here: drive-mirror always creates a new
> block device in qemu, the part that is optional is creating the image
> file.

Kevin (thank you) clarified on IRC a bit further: "It's kind of like it has a
built-in 'blockdev-add'.  So it always opens an image file instead of
using an already opened one -- except that it's a built-in
'blockdev-add' that doesn't accept any options besides the filename, so
it is rather limited.  This is the reason why 'blockdev-mirror' was
introduced" [commit: df92562].

For my own reference, here's the related code that Kevin is referring to
(from qmp_drive_mirror() in blockdev.c)

[...]
3593     /* Mirroring takes care of copy-on-write using the source's backing
3594      * file.
3595      */
3596     target_bs = bdrv_open(arg->target, NULL, options,
3597                           flags | BDRV_O_NO_BACKING, errp);
3598     if (!target_bs) {
3599         goto out;
3600     }
[...]

> > -----------------------------------------------------------------------
> > | 'target' device can be            | The block-core.json file tells  |
> > | attached to the guest             | us that  the 'target' "mustn't  |
> > |                                   | be attached to the guest"       |
> 
> For drive-mirror, 'target' is a file name, so it can't be a block device
> that is already attached. Later attaching it didn't work because it
> didn't have an ID.
> 
> It does, however, have a node-name and with the recent patches that
> allow to use node names to create guest devices... Oops. Don't do that,
> or you'll get what you deserve. :-)

Understood.  :-)

> One more reason to introduce the new op blockers really soon.
> 
> > -----------------------------------------------------------------------
> > | Can specify an (optional)         | No 'node-name' parameter,       |
> > | 'node-name' parameter ("the new   | because `blockdev-add` sets     |
> > | block driver state node name in   | up the 'node-name'[*]           |
> > | the graph") for the target        |                                 |
> > -----------------------------------------------------------------------
> > 
> > [*] On the topic of 'node-name' parameter, I now see that Kevin has
> >     applied his series "[PATCH v5 00/11] block: Accept node-name in all
> >     node level QMP commands" will address this, with the below specific
> >     patch.  [/me noticed that this series is now applied to the
> >     'block-next' Git branch]:
> 
> This is not about the node-name for the target (which is already set by
> blockdev-add as you correctly wrote), but about the source.

Ah, right, thanks for correcting.  Your change[1] to block-core.json
tell that:

    [...]
    -# @device: the name of the device whose writes should be mirrored.
    +# @device: The device name or node-name of a root node whose writes should 
be
    +#          mirrored.
    [...]

[Note to self: Don't write emails late at night, without
double-checking.]

[1] https://lists.nongnu.org/archive/html/qemu-devel/2016-08/msg00540.html

-- 
/kashyap