Re: [Qemu-devel] [PATCH 4/4] qmp: add BLOCK_MEDIUM_EJECT event

[Luiz Capitulino]

On Thu, 16 Feb 2012 19:30:13 +0100
Markus Armbruster <address@hidden> wrote:

> >                     We're probably taking shortcuts so that this
> > function really sees a closed -> closed transition when we really have
> > closed -> open -> closed.
> 
> Let's figure out how this stuff really works.
> 
> 1. Guest load/eject
> 
> Guest commands load or eject.  Device model updates its state of virtual
> tray (e.g. IDEState member tray_open) unless locked, then calls
> bdrv_eject().
> 
> bdrv_eject() is a no-op except for pass-through backends such as
> host_cdrom.
> 
> Note: device models call bdrv_eject() whether the state changed or not.
> The only use for that I can see is syncing a wayward physical tray to
> the virtual one.  Shouldn't be necessary with Paolo's recent work,
> should it?
> 
> Luiz's patch adds event emission to bdrv_eject().  Makes sense, except
> when it's called even though the tray didn't move.  Patch adds a
> parameter to suppress the event then.  I'd prefer to suppress the call
> entirely then, if that's possible.

I can do that. I can't also think of any problem that could arise from
that, except that we will be deviating slightly on how this works in
non-virtual life when using passthrough (ie. all ejects reach the driver and
possibly the drive).

Again, either way is fine with me. I just tried to preserve what the current
code does.

> 2. Monitor commands
> 
> 2a. eject
> 
> run bdrv_close() if eject is permitted right now (not in use by
> long-running block operations such as block migration and image
> streaming, and force or not locked).
> 
> bdrv_close(bs) calls bdrv_dev_change_media_cb(bs, false).
> 
> bdrv_dev_change_media_cb() tells the device model the media went away.
> Device model responds by opening its virtual tray, unless it's already
> open.
> 
> Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
> emits an "open" event when the tray is closed before telling the device
> model. 

Both events are called after dev_ops->change_media_cb() is called, so this
is not true (unless I got you wrong here).

> We assume the device model always opens the tray then, so this
> emits the event only when the tray actually moves.  A bit subtle, but
> works.

Yes, having QMP events in the block layer this way ends up being subtle. That's
why I wanted to have only GUEST_MEDIUM_EJECT, this would allow us to cut
a lot on the subtle factor...

Now, one way of improving this would be to add an interface to the block layer
which allows for other subsystems to register for block layer events. Then we
should trigger those events in well specified block layer "actions", like
"tray moved" or "a block I/O error happened".

> Wart: bdrv_close(bs) is a no-op if bs isn't open.  "bs not open"
> represents "no media".  Therefore, eject without media does nothing.  In
> particular, the virtual tray doesn't move, and no event is emitted.
> Works.
> 
> 2b. change
> 
> First do 2a. eject.  If we had media, the virtual tray is now open.
> Else, it could be open or closed, because of the wart.
> 
> Then open a new block backend.  bdrv_dev_change_media_cb(bs, true) gets
> called either right away by bdrv_open(), or later by bdrv_set_key().
> 
> bdrv_dev_change_media_cb() tells the device model new media appeared.
> Device model responds by closing its virtual tray, unless it's already
> closed[*].
> 
> Luiz's patch adds event emission to bdrv_dev_change_media_cb().  It
> emits an "open" event when the tray is closed before telling the device
> model, and a "closed" event unconditionally.  As above, subtle, but
> works,
> 
> 3. Physical load/eject with pass-through device
> 
> The host_cdrom backend's tray handling is rudimentary.  But let me
> sketch how a real implementation could work.
> 
> 3a. User closes tray, or opens unlocked tray
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.
> 
> 3b. User presses button while tray is closed and locked
> 
> Backend gets notified, calls into block layer.  Block layer notifies
> device model.  Device model notifies guest OS.  Guest OS may command
> tray open.  Goto 1.
> 
> We should be able to emit suitable events in the block layer.
> 
> > Depending on how hard it would be to fix I would suggest that either you
> > fix the internal model, or that we check that the externally visible
> > behaviour is the same as if we did it right and postpone the fix for the
> > internal model.
> 
> Your suggestion makes sense to me.
> 
> I figure Luiz's patch works.  But maybe it could be simplified some by
> replacing bdrv_dev_change_media_cb() by a "open/close tray" callback
> that returns whether it moved.  bdrv_dev_change_media_cb() would then
> simply open the tray, emit event if it moved, close the tray, emit event
> if it moved.

I can work on this refactoring, but I'd like to defer it for now.

> 
> I believe that should also do fine for my case 3.
> 
> 
> [*] Can happen only in the warty case.  Or when the guest closes the
> tray before us.  I'm not sure that works.  The device model sees media
> (because bs is open) even though we don't have the key, yet.  No idea
> what happens when it reads or writes it.
>