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

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

On 04/30/2013 04:17 PM, Ian Campbell wrote:
> On Tue, 2013-04-30 at 16:09 +0100, 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?
>
> I think PHY+EMPTY is the case where you have passed a physical
> CDROM /dev/cdrom type thing.
>
> EMPTY exists for the empty emulated CDROM case.
>
> In general PV devices cannot be "empty" since they don't handle the
> concept of media change (except as noted the special case of a physical
> cdrom and blkback). I expect it would probably be a mistake for TAP or
> QDISK to handle EMPTY. "media change" for PV devices == unplug=>plug.

So how does the PV side of things work then?  Is the backend only 
actually created when the front-end tries to connect to it?

That's part of what confused me when I was looking at this -- I didn't 
see either tapdisk or qdev processes.  Is it the case that the emulated 
device always goes through QEMU?  And so you wouldn't be allowed to use 
a VHD with blktap anyway, since qemu doesn't actually know how to use it?

Maybe we'll just have to wait for Roger to weigh in; but overall for 
this release I'm just inclined to disallow TAP for cdroms, and put 
sorting it out on the to-do for 4.4.

  -George