Re: [Qemu-devel] [PATCH 0/3] v4 Decouple block device removal from device removal

[Ryan Harper <ryanh@us.ibm.com>]

* Markus Armbruster <armbru@redhat.com> [2010-10-29 11:11]:
> Ryan Harper <ryanh@us.ibm.com> writes:
> 
> > * Markus Armbruster <armbru@redhat.com> [2010-10-29 09:13]:
> >> [Note cc: Michael]
> >> 
> >> Ryan Harper <ryanh@us.ibm.com> writes:
> >> 
> >> 
> >> If I understand your patch correctly, the difference between your
> >> drive_unplug and my blockdev_del is as follows:
> >> 
> >> * drive_unplug forcefully severs the connection between the host part of
> >>   the block device and its BlockDriverState.  A shell of the host part
> >>   remains, to be cleaned up later.  You need forceful disconnect
> >>   operation to be able to revoke access to an image whether the guest
> >>   cooperates or not.  Fair enough.
> >> 
> >> * blockdev_del deletes a host part.  My current version fails when the
> >>   host part is in use.  I patterned that after netdev_del, which used to
> >>   work that way, until commit 2ffcb18d:
> >> 
> >>     Make netdev_del delete the netdev even when it's in use
> >>     
> >>     To hot-unplug guest and host part of a network device, you do:
> >>     
> >>         device_del NIC-ID
> >>         netdev_del NETDEV-ID
> >>     
> >>     For PCI devices, device_del merely tells ACPI to unplug the device.
> >>     The device goes away for real only after the guest processed the ACPI
> >>     unplug event.
> >>     
> >>     You have to wait until then (e.g. by polling info pci) before you can
> >>     unplug the netdev.  Not good.
> >>     
> >>     Fix by removing the "in use" check from do_netdev_del().  Deleting a
> >>     netdev while it's in use is safe; packets simply get routed to the bit
> >>     bucket.
> >> 
> >>   Isn't this the very same problem that's behind your drive_unplug?
> >
> > Yes it is.
> >
> >> 
> >> I'd like to have some consistency among net, block and char device
> >> commands, i.e. a common set of operations that work the same for all of
> >> them.  Can we agree on such a set?
> >
> > Yeah; the current trouble (or at least what I perceive to be trouble) is
> > that in the case where the guest responds to device_del induced ACPI
> > removal event; the current qdev code already does the host-side device
> > tear down.  Not sure if it is OK to do a blockdev_del() immediately
> > after the device_del.  What happens when we do:
> >
> > device_del
> > ACPI to guest
> > blockdev_del /* removes host-side device */
> 
> Fails in my tree, because the blockdev's still in use.  See below.
> 
> > guest responds to ACPI
> > qdev calls pci device removal code
> > qemu attempts to destroy the associated host-side block
> >
> > That may just work today; and if not, it shouldn't be hard to fix up the
> > code to check for NULLs
> 
> I hate the automatic deletion of host part along with the guest part.
> device_del should undo device_add.  {block,net,char}dev_{add,del} should
> be similarly paired.

Agreed.
> 
> In my blockdev branch, I keep the automatic delete only for backwards
> compatibility: if you create the drive with drive_add, it gets
> auto-deleted, but if you use blockdev_add, it stays around.

But what to do about the case where we're doing drive_add and then a
device_del()  That's the urgent situation that needs to be resolved.

> 
> >> Even if your drive_unplug shouldn't fit in that set, we might want it as
> >> a stop-gap.  Depends on how urgent the need for it is.  Yet another
> >> special-purpose command to be deprecated later.
> >
> > The fix is urgent; but I'm willing to spin a couple patches if it helps
> > get this into better shape.
> 
> Can we agree on a common solution for block and net?  That's why I cc'ed
> Michael.

I didn't see a good way to have block behave the same as net; though I
do agree that it would be good to have this be common, long term.

> 
> Currently, we have two different ways:
> 
> * The netdev way: "del" always succeeds
> 
>   How can it succeed if the host part is in use?
> 
>   If all device models are prepared to deal with a missing host part, we
>   can delete it right away.
> 
>   Else, we need to replace it with a suitable zombie, which is
>   auto-deleted when it goes out of use.  Such zombies are not be visible
>   elsewhere, in particular, the ID becomes available immediately.
> 
> * The unplug way: "del" fails while in use, "unplug" always succeeds
> 
>   Feels a bit cleaner to me.  But changing netdev_del might not be
>   acceptable.
> 
> Either way works for me as an user interface.  But I'd rather not have
> both.
> 
> Next, we need to consider how to integrate this with the automatic
> deletion of drives on qdev destruction.  That's too late for unplug, we
> want that right in device_del.  I'd leave the stupid automatic delete
> where it is now, in qdev destruction.  The C API need unplug and delete
> separate for that.
> 
> 
> Regardless of the way we choose, we need to think very clearly on how
> exactly device models should behave when their host part is missing or a
> zombie, and how that behavior appears in the guest.
> 
> For net, making it look exactly like a yanked out network cable would
> make sense to me.
> 
> What about block?

It seems to me that for block it's like cdrom with no disk, floppy with
no media, hard disk that's gone bad.  I think we we throw EIO back; it's
handled gracefully enough.  This is what happens when you do a
drive_unplug with my patch; the application using the device gets IO
errors.  That's expected if a drive were to suddently fail (which is
what this looks like).  And certainly there is some responsibility
at the mgmt console to ensure you're not unplugging a drive that you are
currently using.




-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
ryanh@us.ibm.com