Re: blktap, qdisk, xl cd-eject, and xencommons

[George Dunlap <george.dunlap@eu.citrix.com>]

On 04/30/2013 04:09 PM, George Dunlap wrote:
> On 04/30/2013 03:12 PM, Ian Campbell wrote:
>> On Tue, 2013-04-30 at 13:27 +0100, George Dunlap wrote:
>>> On 04/30/2013 11:21 AM, Ian Campbell wrote:
>>>> On Tue, 2013-04-30 at 11:02 +0100, George Dunlap wrote:
>>>>> The second is problems with xl cd-insert and eject, initially reported
>>>>> by Fabio Fantoni, and then (accidentally) reproduced by me.  The
>>>>> problem turns out to be libxl using blktap for cdroms.  Basically,
>>>>> AFAICT, the whole cd-insert cd-eject thing completely doen't work if
>>>>> blktap is used to provide it; and it's not a simple fix.
>>>>
>>>> cd-insert/eject are suppose to operate on emulated CDROM, which blktap
>>>> simply isn't in a position to provide, even if it was capable of doing
>>>> so. So this is certainly wrong IMHO at some level.
>>>>
>>>> All emulated disks have a PV counterpart. Many (all?) PVHVM drivers
>>>> choose not to unplug the emulated CDROM and use it in preference to the
>>>> PV version (since this way they get proper media change events etc) but
>>>> I don't think they are required to behave like this.
>>>>
>>>> In principal it's not wrong to provide the PV face of a device from a
>>>> different backend to the emulated one (e.g. we do blkback+qdisk all the
>>>> time for disks), but in the interests of simplicity it seems like the
>>>> obvious thing to do is to unconditionally use qdisk for both faces of an
>>>> HVM guest's CDROM.
>>>
>>> Right -- so I guess from a release perspective the best thing to do
>>> would just be to have tools/libxl/libxl_device.c:disk_try_backend() fail
>>> for TAP if disk->is_cdrom is true?
>>
>> Without having consulted the code that Sounds Plausible, yes.
>>
>> Is the underling problem understood though? In principal PV=blktap
>> +EMU=qemu should work, is this just some confusion on libxl's side when
>> reading the xenstore entries? I we sure we can't trigger that confusion
>> for other uses of blktap -- e.g. disks.
>
> Well the whole thing is a bit of a tangled mess right now.
>
> For one thing, disk_try_backend() will return an error if disk->format
> is EMPTY.  (Which is why in 4.2 if you hit this path, it breaks
> everything -- libxl__device_disk_set_backend() sets it to qdev because
> TAP fails, so it tries to write new qdev stuff and fails halfway through.)
>
> Is there anyone who actually knows what's supposed to be going on here,
> that we can ask to help with this?
>
> I suppose it might have been a mistake that TAP fails for EMPTY -- PHY
> will take EMPTY, so maybe TAP can too?
>
> Thanos, do you have any idea about that?  It looks like for an empty
> cdrom, it just sets "params" equal to "" in xenstore
> (tools/libxl/libxl.c:libxl_cdrom_insert()) -- will that work for blktap,
> or cause a problem?
>
> And, can changing the the "params" change the actual disk in the current
> version of blktap?

And the other thing is, it looks like for qemu-xen, libxl_cdrom_insert() 
unconditionally calls libxl__qmp_insert_cdrom(), which passes the path 
of the file in to qemu anyway -- so is blktap even actually being used 
in that case?  Or is it really just qemu directly accessing the file anyway?

  -George