blktap, qdisk, xl cd-eject, and xencommons

blktap, qdisk, xl cd-eject, and xencommons

[George Dunlap]

Following on from the recent discussion about qdisk, there are two other 
issues related to blktap and qdisk that need to be sorted out.

There are two bugs on the Xen release "blocker" list related to qdisk.

The first is "remove hard-coded modprobe's from xencommons".

Background:
- libxl will use blktap by default if it can

- but libxl only knows it can use it if it's already loaded

- QDEV used to use O_DIRECT, which tickled a bug in PVOPS kernels that 
would cause it to crash

- So it used to be the case that if you booted on (for example) a Debian 
Squeeze kernel with Xen, and had a cd-rom (or anything using qdisk), 
your system would eventually crash

- Adding "modprobe blktap" to xencommons fixed this by causing libxl to 
use blktap instead

- But Jan (rightly) objected to having such a hard-coded thing, and 
accepted it for 4.2 only on the condition that it would be removed for 4.3

- However, the initial problem (qdev crashing on pvops kernels) has been 
fixed in a different way now: by removing O_DIRECT from qdev.  I have 
verified that the same configuration (Debian Squeeze) running with a 
qdev cdrom does not crash anymore.

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.

To address these issue for 4.3, I would like to propose:

1. Have libxl default to qdev for everything, except for things that it 
needs blktap for (like vhd, IIRC)

2. Simply remove the hard-coded modprobe from xencommons

3. Add code into libxl to do modprobe-ing as part of the "does this 
system support blktap" check.

But I think #3 can probably wait until 4.4.

Thoughts?

  -George

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

[Jan Beulich]

>>> On 30.04.13 at 12:02, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> To address these issue for 4.3, I would like to propose:
> 
> 1. Have libxl default to qdev for everything, except for things that it 
> needs blktap for (like vhd, IIRC)

Everything? I.e. even raw disks? I thought those go through
blkbk...

> 2. Simply remove the hard-coded modprobe from xencommons
> 
> 3. Add code into libxl to do modprobe-ing as part of the "does this 
> system support blktap" check.
> 
> But I think #3 can probably wait until 4.4.

Can it? How would 1 work for "things that it needs blktap for"
if it doesn't first make an attempt to load the driver? And if it
needs to load the driver, it can as well be done as part of the
"does this system support blktap" check.

Jan

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

[Ian Campbell]

On Tue, 2013-04-30 at 11:17 +0100, Jan Beulich wrote:
> >>> On 30.04.13 at 12:02, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > To address these issue for 4.3, I would like to propose:
> > 
> > 1. Have libxl default to qdev for everything, except for things that it 
> > needs blktap for (like vhd, IIRC)
> 
> Everything? I.e. even raw disks? I thought those go through
> blkbk...

Yes, and they should continue to do so IMHO. I think George really meant
"all structured disk types" or something.

Ian.

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

[George Dunlap]

On 04/30/2013 11:21 AM, Ian Campbell wrote:
> On Tue, 2013-04-30 at 11:17 +0100, Jan Beulich wrote:
>>>>> On 30.04.13 at 12:02, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>>> To address these issue for 4.3, I would like to propose:
>>>
>>> 1. Have libxl default to qdev for everything, except for things that it
>>> needs blktap for (like vhd, IIRC)
>>
>> Everything? I.e. even raw disks? I thought those go through
>> blkbk...
>
> Yes, and they should continue to do so IMHO. I think George really meant
> "all structured disk types" or something.

Yes, sorry -- I suppose what I really meant was to make the preference 
"blkback > qdev > blktap" -- i.e., if there's a choice between qdev and 
blktap, use qdev (whereas now it's the other way around, AIUI).

  -George

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

[Ian Campbell]

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.

Ian.

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

[George Dunlap]

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?

  -George

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

[Ian Campbell]

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.

Ian.

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

[George Dunlap]

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?

  -George

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

[George Dunlap]

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

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

[Ian Campbell]

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.

Of course quite how this interacts with the "PV half of an emulated
device" stuff I've no idea, conceptually it makes my head hurt.

> 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?
> 
>   -George

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

[George Dunlap]

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

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

[Ian Campbell]

On Tue, 2013-04-30 at 17:02 +0100, George Dunlap wrote:
> 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?

AFAIK the backend is only created when you "insert" a disk into the
CDROM.

> That's part of what confused me when I was looking at this -- I didn't 
> see either tapdisk or qdev processes.

There is no separate qdev process -- just the one qemu process providing
emulation and PV qdisk backends (if asked to)

>   Is it the case that the emulated 
> device always goes through QEMU?

Absolutely, nothing else knows how to emulate a CDROM drive.

> And so you wouldn't be allowed to use a VHD with blktap anyway, since
> qemu doesn't actually know how to use it?

qemu does know how to use VHD, I'm not sure how good its implementation
is.

Don't forget that there are actually two devices here. The emulated one
is *always* provided by qemu and the PV one which can be provided by
blkback, tapdisk or qdisk depending on $decision.

In principal there is nothing about the fact that qemu is providing the
emulated version which requires qdisk to be the PV half.

> 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

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

[Thanos Makatos]

> 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?

I'm afraid I'm not familiar with blktap/tapdisk serving CR-ROM images.

> 
> And, can changing the the "params" change the actual disk in the
> current version of blktap?
> 
>   -George

Ditto.

One thing I remember though, for VHD files "params" is not written to XenStore at all since it's libxl itself spawning the tapdisk, so I wouldn't expect a change in params to picked up neither tapdisk nor by blktap. But of course I'm speculating :|

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

[Wei Liu]

On Tue, Apr 30, 2013 at 11:02:38AM +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.
> 
> To address these issue for 4.3, I would like to propose:
> 
> 1. Have libxl default to qdev for everything, except for things that it 
> needs blktap for (like vhd, IIRC)
> 
> 2. Simply remove the hard-coded modprobe from xencommons
> 

Presumably you only mean "modprobe blktap ..."?

> 3. Add code into libxl to do modprobe-ing as part of the "does this 
> system support blktap" check.
> 
> But I think #3 can probably wait until 4.4.
> 

I don't quite understand 2, 3 and the statment "#3 can probably wait
until 4.4". If blktap is necessary for VHD, we can not simply remove the
modprobe while not adding infrastructure in libxl to automatically load
blktap.


Wei.

> Thoughts?
> 
>   -George

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

[George Dunlap]

On 04/30/2013 11:22 AM, Wei Liu wrote:
> On Tue, Apr 30, 2013 at 11:02:38AM +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.
>>
>> To address these issue for 4.3, I would like to propose:
>>
>> 1. Have libxl default to qdev for everything, except for things that it
>> needs blktap for (like vhd, IIRC)
>>
>> 2. Simply remove the hard-coded modprobe from xencommons
>>
>
> Presumably you only mean "modprobe blktap ..."?
>
>> 3. Add code into libxl to do modprobe-ing as part of the "does this
>> system support blktap" check.
>>
>> But I think #3 can probably wait until 4.4.
>>
>
> I don't quite understand 2, 3 and the statment "#3 can probably wait
> until 4.4". If blktap is necessary for VHD, we can not simply remove the
> modprobe while not adding infrastructure in libxl to automatically load
> blktap.

Well we didn't have that before 4.3, and it didn't seem to be a major 
problem; I think people just knew to make sure blktap got modprobed 
themselves.  So I wouldn't personally consider this a blocker -- but I'm 
still open to other ideas.

In any case, it would certainly be *better* if we can have libxl attempt 
a modprobe, so if we can get it in before the release, I think that 
would be a good thing.

  -George

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

[Wei Liu]

On Tue, Apr 30, 2013 at 11:56:35AM +0100, George Dunlap wrote:
[...]
> 
> Well we didn't have that before 4.3, and it didn't seem to be a major 
                                  4.2 maybe?
> problem; I think people just knew to make sure blktap got modprobed 
> themselves.  So I wouldn't personally consider this a blocker -- but I'm 
> still open to other ideas.
> 
> In any case, it would certainly be *better* if we can have libxl attempt 
> a modprobe, so if we can get it in before the release, I think that 
> would be a good thing.
> 

So now the libxl infrastructure is no longer a blocker, good. Then how
can we justify it if we need it to go in at this stage? We're
approaching RC1 next week, what is the suitable policy for this
infrastructure?


Wei.

>   -George

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

[George Dunlap]

On 04/30/2013 12:54 PM, Wei Liu wrote:
> On Tue, Apr 30, 2013 at 11:56:35AM +0100, George Dunlap wrote:
> [...]
>>
>> Well we didn't have that before 4.3, and it didn't seem to be a major
>                                    4.2 maybe?
>> problem; I think people just knew to make sure blktap got modprobed
>> themselves.  So I wouldn't personally consider this a blocker -- but I'm
>> still open to other ideas.
>>
>> In any case, it would certainly be *better* if we can have libxl attempt
>> a modprobe, so if we can get it in before the release, I think that
>> would be a good thing.
>>
>
> So now the libxl infrastructure is no longer a blocker, good. Then how
> can we justify it if we need it to go in at this stage? We're
> approaching RC1 next week, what is the suitable policy for this
> infrastructure?

Well most of those things are "guidelines" rather than rules. :-)  Not 
everything fits neatly into the packages.  In this case, this is a mild 
regression from 4.2 -- so it could be sort of considered a "bug fix".

OTOH, if Jan were willing, the lowest disturbance thing to do might be 
to leave the modprobe in, and promise to address the issue first thing 
in the 4.4.  (In that case I'd owe Jan a big apology, as it was I that 
promised to sort it out for 4.3.)

  -George

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

[Jan Beulich]

>>> On 30.04.13 at 14:16, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 04/30/2013 12:54 PM, Wei Liu wrote:
>> On Tue, Apr 30, 2013 at 11:56:35AM +0100, George Dunlap wrote:
>> [...]
>>>
>>> Well we didn't have that before 4.3, and it didn't seem to be a major
>>                                    4.2 maybe?
>>> problem; I think people just knew to make sure blktap got modprobed
>>> themselves.  So I wouldn't personally consider this a blocker -- but I'm
>>> still open to other ideas.
>>>
>>> In any case, it would certainly be *better* if we can have libxl attempt
>>> a modprobe, so if we can get it in before the release, I think that
>>> would be a good thing.
>>>
>>
>> So now the libxl infrastructure is no longer a blocker, good. Then how
>> can we justify it if we need it to go in at this stage? We're
>> approaching RC1 next week, what is the suitable policy for this
>> infrastructure?
> 
> Well most of those things are "guidelines" rather than rules. :-)  Not 
> everything fits neatly into the packages.  In this case, this is a mild 
> regression from 4.2 -- so it could be sort of considered a "bug fix".
> 
> OTOH, if Jan were willing, the lowest disturbance thing to do might be 
> to leave the modprobe in, and promise to address the issue first thing 
> in the 4.4.  (In that case I'd owe Jan a big apology, as it was I that 
> promised to sort it out for 4.3.)

If this turns too intrusive at this point I wouldn't heavily object,
but would raise the question of what if the same happens during
the 4.4 cycle again.

Jan

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

[George Dunlap]

On 30/04/13 13:38, Jan Beulich wrote:
>>>> On 30.04.13 at 14:16, George Dunlap <george.dunlap@eu.citrix.com> wrote:
>> On 04/30/2013 12:54 PM, Wei Liu wrote:
>>> On Tue, Apr 30, 2013 at 11:56:35AM +0100, George Dunlap wrote:
>>> [...]
>>>> Well we didn't have that before 4.3, and it didn't seem to be a major
>>>                                     4.2 maybe?
>>>> problem; I think people just knew to make sure blktap got modprobed
>>>> themselves.  So I wouldn't personally consider this a blocker -- but I'm
>>>> still open to other ideas.
>>>>
>>>> In any case, it would certainly be *better* if we can have libxl attempt
>>>> a modprobe, so if we can get it in before the release, I think that
>>>> would be a good thing.
>>>>
>>> So now the libxl infrastructure is no longer a blocker, good. Then how
>>> can we justify it if we need it to go in at this stage? We're
>>> approaching RC1 next week, what is the suitable policy for this
>>> infrastructure?
>> Well most of those things are "guidelines" rather than rules. :-)  Not
>> everything fits neatly into the packages.  In this case, this is a mild
>> regression from 4.2 -- so it could be sort of considered a "bug fix".
>>
>> OTOH, if Jan were willing, the lowest disturbance thing to do might be
>> to leave the modprobe in, and promise to address the issue first thing
>> in the 4.4.  (In that case I'd owe Jan a big apology, as it was I that
>> promised to sort it out for 4.3.)
> If this turns too intrusive at this point I wouldn't heavily object,
> but would raise the question of what if the same happens during
> the 4.4 cycle again.

So our options, AFAICT, are as follows:

1. Remove modprobe from xencommons, try to put a modprobe into libxl.

This introduces new, potentially risky behavior at the 11th hour.

2. Remove modprobe from xencommons,

This introduces a mild regression: people who are using blktap now have 
to make sure that blktap gets loaded.

3. Punt for 4.4.

#2 isn't terrible, I don't think, but overall I think #3 is the best 
option for users, although it means me having to say I'm sorry for 
failing to follow through on a commitment. :-)

To answer your question about the 4.4 cycle:
1. We have a much clearer idea about what needs to be done now
2. I have a much clearer idea about how long a fix might take, so I will 
work on it first thing in the cycle.

So unless you (or someone else) objects, I think I'm going to move the 
xencommons thing to a "for 4.4"

  -George

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

[Jan Beulich]

>>> On 01.05.13 at 18:30, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> So unless you (or someone else) objects, I think I'm going to move the 
> xencommons thing to a "for 4.4"

Go ahead then.

Jan