[PATCH] xencommons: Attempt to load blktap driver

[PATCH] xencommons: Attempt to load blktap driver

[George Dunlap]

Older kernels, such as those found in Debian Squeeze:
* Have bugs in handling of AIO into foreign pages
* Have blktap modules, which will cause qemu not to use AIO, but
which are not loaded on boot.

Attempt to load blktap in xencommons, to make sure modern qemu's which
use AIO will work properly on those kernels.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
--- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
+++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
@@ -59,6 +59,7 @@ do_start () {
 	modprobe evtchn 2>/dev/null
 	modprobe gntdev 2>/dev/null
 	modprobe xen-acpi-processor 2>/dev/null
+	modprobe blktap 2>/dev/null
 	mkdir -p /var/run/xen
 
 	if ! `xenstore-read -s / >/dev/null 2>&1`

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 15.05.12 at 17:49, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> Older kernels, such as those found in Debian Squeeze:
> * Have bugs in handling of AIO into foreign pages
> * Have blktap modules, which will cause qemu not to use AIO, but
> which are not loaded on boot.
> 
> Attempt to load blktap in xencommons, to make sure modern qemu's which
> use AIO will work properly on those kernels.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
> 
> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
> @@ -59,6 +59,7 @@ do_start () {
>  	modprobe evtchn 2>/dev/null
>  	modprobe gntdev 2>/dev/null
>  	modprobe xen-acpi-processor 2>/dev/null
> +	modprobe blktap 2>/dev/null

Can we stop manually loading all kinds of drivers here? I was
glad this went away with the switch to xencommons, and
now this is coming back. Drivers definitely needed in all cases
are acceptable imo, but backend drivers should be loaded as
backends get created by the tools (similarly frontend drivers
for the local attach case, though they should get auto-loaded
normally anyway).

Jan

>  	mkdir -p /var/run/xen
>  
>  	if ! `xenstore-read -s / >/dev/null 2>&1`
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH] xencommons: Attempt to load blktap driver

[George Dunlap]

On 15/05/12 17:16, Jan Beulich wrote:
>>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>> Older kernels, such as those found in Debian Squeeze:
>> * Have bugs in handling of AIO into foreign pages
>> * Have blktap modules, which will cause qemu not to use AIO, but
>> which are not loaded on boot.
>>
>> Attempt to load blktap in xencommons, to make sure modern qemu's which
>> use AIO will work properly on those kernels.
>>
>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>
>> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
>> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
>> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
>> @@ -59,6 +59,7 @@ do_start () {
>>   	modprobe evtchn 2>/dev/null
>>   	modprobe gntdev 2>/dev/null
>>   	modprobe xen-acpi-processor 2>/dev/null
>> +	modprobe blktap 2>/dev/null
> Can we stop manually loading all kinds of drivers here? I was
> glad this went away with the switch to xencommons, and
> now this is coming back. Drivers definitely needed in all cases
> are acceptable imo, but backend drivers should be loaded as
> backends get created by the tools (similarly frontend drivers
> for the local attach case, though they should get auto-loaded
> normally anyway).
I tend to agree with you; I did it this way because that's what was 
suggested to me.  But I don't at the moment know enough about the 
backend creation stuff in xl / qemu to DTRT here.

If you want to volunteer to do a patch that DTRT, I think it makes sense 
to hold off.  But if not, I suggest we accept this patch, and I'll come 
back and try to write a proper one before the 4.2 release.  I think it's 
really important we do something before 4.2, as it causes pretty serious 
problems on systems which are affected (almost always a host crash, 
possibly with some disk corruption).

  -George

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 15.05.12 at 18:21, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 15/05/12 17:16, Jan Beulich wrote:
>>>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>>> Older kernels, such as those found in Debian Squeeze:
>>> * Have bugs in handling of AIO into foreign pages
>>> * Have blktap modules, which will cause qemu not to use AIO, but
>>> which are not loaded on boot.
>>>
>>> Attempt to load blktap in xencommons, to make sure modern qemu's which
>>> use AIO will work properly on those kernels.
>>>
>>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>>
>>> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
>>> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
>>> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
>>> @@ -59,6 +59,7 @@ do_start () {
>>>   	modprobe evtchn 2>/dev/null
>>>   	modprobe gntdev 2>/dev/null
>>>   	modprobe xen-acpi-processor 2>/dev/null
>>> +	modprobe blktap 2>/dev/null
>> Can we stop manually loading all kinds of drivers here? I was
>> glad this went away with the switch to xencommons, and
>> now this is coming back. Drivers definitely needed in all cases
>> are acceptable imo, but backend drivers should be loaded as
>> backends get created by the tools (similarly frontend drivers
>> for the local attach case, though they should get auto-loaded
>> normally anyway).
> I tend to agree with you; I did it this way because that's what was 
> suggested to me.  But I don't at the moment know enough about the 
> backend creation stuff in xl / qemu to DTRT here.
> 
> If you want to volunteer to do a patch that DTRT, I think it makes sense 
> to hold off.

No, I won't.

>  But if not, I suggest we accept this patch, and I'll come 
> back and try to write a proper one before the 4.2 release.  I think it's 
> really important we do something before 4.2, as it causes pretty serious 
> problems on systems which are affected (almost always a host crash, 
> possibly with some disk corruption).

A host crash because of a driver not loaded? That would suggest
bugs elsewhere...

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[George Dunlap]

On 15/05/12 17:36, Jan Beulich wrote:
>>>> On 15.05.12 at 18:21, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>> On 15/05/12 17:16, Jan Beulich wrote:
>>>>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>   wrote:
>>>> Older kernels, such as those found in Debian Squeeze:
>>>> * Have bugs in handling of AIO into foreign pages
>>>> * Have blktap modules, which will cause qemu not to use AIO, but
>>>> which are not loaded on boot.
>>>>
>>>> Attempt to load blktap in xencommons, to make sure modern qemu's which
>>>> use AIO will work properly on those kernels.
>>>>
>>>> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>>>>
>>>> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
>>>> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
>>>> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
>>>> @@ -59,6 +59,7 @@ do_start () {
>>>>    	modprobe evtchn 2>/dev/null
>>>>    	modprobe gntdev 2>/dev/null
>>>>    	modprobe xen-acpi-processor 2>/dev/null
>>>> +	modprobe blktap 2>/dev/null
>>> Can we stop manually loading all kinds of drivers here? I was
>>> glad this went away with the switch to xencommons, and
>>> now this is coming back. Drivers definitely needed in all cases
>>> are acceptable imo, but backend drivers should be loaded as
>>> backends get created by the tools (similarly frontend drivers
>>> for the local attach case, though they should get auto-loaded
>>> normally anyway).
>> I tend to agree with you; I did it this way because that's what was
>> suggested to me.  But I don't at the moment know enough about the
>> backend creation stuff in xl / qemu to DTRT here.
>>
>> If you want to volunteer to do a patch that DTRT, I think it makes sense
>> to hold off.
> No, I won't.
>
>>   But if not, I suggest we accept this patch, and I'll come
>> back and try to write a proper one before the 4.2 release.  I think it's
>> really important we do something before 4.2, as it causes pretty serious
>> problems on systems which are affected (almost always a host crash,
>> possibly with some disk corruption).
> A host crash because of a driver not loaded? That would suggest
> bugs elsewhere...
Yes -- a bug in the AIO implementation for foreign pages, as the 
description states.

  -George

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 15.05.12 at 18:40, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> On 15/05/12 17:36, Jan Beulich wrote:
>>>>> On 15.05.12 at 18:21, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>>>   But if not, I suggest we accept this patch, and I'll come
>>> back and try to write a proper one before the 4.2 release.  I think it's
>>> really important we do something before 4.2, as it causes pretty serious
>>> problems on systems which are affected (almost always a host crash,
>>> possibly with some disk corruption).
>> A host crash because of a driver not loaded? That would suggest
>> bugs elsewhere...
> Yes -- a bug in the AIO implementation for foreign pages, as the 
> description states.

But if there's no backend driver, there shouldn't be any I/O at all,
and hence no crash? Or are you saying that in the absence of the
driver another (qdisk?) backend gets selected (which doesn't match
my own observations, at least not as far as local attach to Dom0
is concerned, and I would hope [expect] that backend selection
doesn't depend on which domain the frontend lives in)?

Finally, loading "blktap" here is the right thing for pvops, but
wrong for legacy/forward ported kernels - blktap2 would be
the right module name there afaict. Perhaps, if this really is to
go in (temporarily), loading an alias (devname: or xen-backend:,
though the latter apears to be missing from the pvops driver)
would be better here?

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Wed, 2012-05-16 at 08:47 +0100, Jan Beulich wrote:
> >>> On 15.05.12 at 18:40, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> > On 15/05/12 17:36, Jan Beulich wrote:
> >>>>> On 15.05.12 at 18:21, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
> >>>   But if not, I suggest we accept this patch, and I'll come
> >>> back and try to write a proper one before the 4.2 release.  I think it's
> >>> really important we do something before 4.2, as it causes pretty serious
> >>> problems on systems which are affected (almost always a host crash,
> >>> possibly with some disk corruption).
> >> A host crash because of a driver not loaded? That would suggest
> >> bugs elsewhere...
> > Yes -- a bug in the AIO implementation for foreign pages, as the 
> > description states.
> 
> But if there's no backend driver, there shouldn't be any I/O at all,
> and hence no crash? Or are you saying that in the absence of the
> driver another (qdisk?) backend gets selected (which doesn't match
> my own observations, at least not as far as local attach to Dom0
> is concerned, and I would hope [expect] that backend selection
> doesn't depend on which domain the frontend lives in)?

backend selection depends on what drivers are available in the backend
domain, not the frontend domain.

libxl will try blktap(2) first and fallback to qdisk if blktap is not
present. The issue George is seeing is that qdisk can sometimes exercise
AIO bugs which cause host crashes. The fix is to try harder to use
blktap when it is really available by ensuring it gets loaded.

> Finally, loading "blktap" here is the right thing for pvops, but
> wrong for legacy/forward ported kernels - blktap2 would be
> the right module name there afaict. Perhaps, if this really is to
> go in (temporarily), loading an alias (devname: or xen-backend:,
> though the latter apears to be missing from the pvops driver)
> would be better here?

There is no blktap driver at all in mainline pvops, which is part of the
problem and the reason for falling back to qdisk. Which pvops driver do
you mean? To confuse matters there is a DKMS thing floating around which
works against modern kernels too.

Perhaps the right answer is (roughly) "modprobe blktap2 || modprobe
blktap"?

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 16.05.12 at 10:25, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-05-16 at 08:47 +0100, Jan Beulich wrote:
>> Finally, loading "blktap" here is the right thing for pvops, but
>> wrong for legacy/forward ported kernels - blktap2 would be
>> the right module name there afaict. Perhaps, if this really is to
>> go in (temporarily), loading an alias (devname: or xen-backend:,
>> though the latter apears to be missing from the pvops driver)
>> would be better here?
> 
> There is no blktap driver at all in mainline pvops, which is part of the
> problem and the reason for falling back to qdisk. Which pvops driver do
> you mean?

The one in Jeremy's tree (on branch xen/next-2.6.32), which is
easily forward ported to current kernels.

> Perhaps the right answer is (roughly) "modprobe blktap2 || modprobe
> blktap"?

modprobe devname:xen/blktap-2/control

or

modprobe xen-backend:tap2

(but I now realize the former is a 2.6.35 addition [or really a
counterpart of devname: aliases added elsewhere in that
release], and the latter one done in our patches for 3.1, where
other xen-backend: aliases also got added).

So yes, looks like the only half-way compatible way would be
what you suggest (perhaps extended to redirect stderr to
/dev/null).

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Wed, 2012-05-16 at 08:47 +0100, Jan Beulich wrote:
> Finally, loading "blktap" here is the right thing for pvops, but
> wrong for legacy/forward ported kernels - blktap2 would be
> the right module name there afaict. Perhaps, if this really is to
> go in (temporarily), loading an alias (devname: or xen-backend:,
> though the latter apears to be missing from the pvops driver)
> would be better here?

George's patch doesn't seem to apply any more (hardly surprising).

>From what you say I think we want to modprobe blktap if blktap2 didn't
exist.

blktap2 isn't actually a xenbus backend driver (since it uses blkback to
do the guest facing bit) so I don't think a xen-backend: alias is
available. I can't see any other aliases defined in the code in either
the 2.6.18-xen tree, the SLES 2.6.32.12-0.7.1 kernel (which is the
latest I happen to have to hand) or a mainline kernel. If there is
something else we should be trying please let me know.

So I intend to commit the following:

8<--------------------------------------------------

xencommons: Attempt to load blktap2 driver

Older kernels, such as those found in Debian Squeeze:
* Have bugs in handling of AIO into foreign pages
* Have blktap modules, which will cause qemu not to use AIO, but
  which are not loaded on boot.

Attempt to load blktap in xencommons, to make sure modern qemu's which
use AIO will work properly on those kernels.

Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Prefer to load blktap2 if it exists. This is the name of the driver in
classic-Xen ports, while in mainline kernels the driver is called just
blktap.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff -r 1126b3079bef -r 451724678dd4 tools/hotplug/Linux/init.d/xencommons
--- a/tools/hotplug/Linux/init.d/xencommons	Fri Aug 24 12:38:18 2012 +0100
+++ b/tools/hotplug/Linux/init.d/xencommons	Fri Aug 31 09:32:37 2012 +0100
@@ -68,6 +68,7 @@ do_start () {
 	modprobe usbbk 2>/dev/null
 	modprobe pciback 2>/dev/null
 	modprobe xen-acpi-processor 2>/dev/null
+	modprobe blktap2 2>/dev/null || modprobe blktap 2>/dev/null
 	mkdir -p /var/run/xen
 
 	if ! `xenstore-read -s / >/dev/null 2>&1`

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 31.08.12 at 10:40, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> From what you say I think we want to modprobe blktap if blktap2 didn't
> exist.
> 
> blktap2 isn't actually a xenbus backend driver (since it uses blkback to
> do the guest facing bit) so I don't think a xen-backend: alias is
> available. I can't see any other aliases defined in the code in either
> the 2.6.18-xen tree, the SLES 2.6.32.12-0.7.1 kernel (which is the
> latest I happen to have to hand) or a mainline kernel. If there is
> something else we should be trying please let me know.

There's a "devname:xen/blktap-2/control" alias in our SLE11 SP2
and newer openSUSE ones (as of 2.6.35). Whether that's fully
appropriate to be there and/or to be used as a modprobe
argument I'm not sure though.

The bad thing about the "blktap" name is that that's also the
name of the blktap1 driver in the 2.6.18 tree and its forward
ports, but I don't think there's anything we can reasonably do
about that. So I'm fine with the change you suggest from that
perspective (whether to use the module alias pointed out ).

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Fri, 2012-08-31 at 10:03 +0100, Jan Beulich wrote:
> >>> On 31.08.12 at 10:40, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > From what you say I think we want to modprobe blktap if blktap2 didn't
> > exist.
> > 
> > blktap2 isn't actually a xenbus backend driver (since it uses blkback to
> > do the guest facing bit) so I don't think a xen-backend: alias is
> > available. I can't see any other aliases defined in the code in either
> > the 2.6.18-xen tree, the SLES 2.6.32.12-0.7.1 kernel (which is the
> > latest I happen to have to hand) or a mainline kernel. If there is
> > something else we should be trying please let me know.
> 
> There's a "devname:xen/blktap-2/control" alias in our SLE11 SP2
> and newer openSUSE ones (as of 2.6.35). Whether that's fully
> appropriate to be there and/or to be used as a modprobe
> argument I'm not sure though.
> 
> The bad thing about the "blktap" name is that that's also the
> name of the blktap1 driver in the 2.6.18 tree and its forward
> ports, but I don't think there's anything we can reasonably do
> about that.

I thought about that. Most kernels which have blktap1 nowadays also have
blktap2 so the number of systems where you would actually end up with
only blktap1 loaded is pretty small. It's also AFAIK reasonably harmless
other than the memory usage etc.

In retrospect renaming blktap2->blktap ni pvops was a stupid idea (I can
say that since it was my idea...)

>  So I'm fine with the change you suggest from that
> perspective (whether to use the module alias pointed out ).

Can I take that as an
Acked-by: Jan Beulich <JBeulich@suse.com>
?

I think I'll skip the alias for now.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 31.08.12 at 11:33, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2012-08-31 at 10:03 +0100, Jan Beulich wrote:
>> >>> On 31.08.12 at 10:40, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > From what you say I think we want to modprobe blktap if blktap2 didn't
>> > exist.
>> > 
>> > blktap2 isn't actually a xenbus backend driver (since it uses blkback to
>> > do the guest facing bit) so I don't think a xen-backend: alias is
>> > available. I can't see any other aliases defined in the code in either
>> > the 2.6.18-xen tree, the SLES 2.6.32.12-0.7.1 kernel (which is the
>> > latest I happen to have to hand) or a mainline kernel. If there is
>> > something else we should be trying please let me know.
>> 
>> There's a "devname:xen/blktap-2/control" alias in our SLE11 SP2
>> and newer openSUSE ones (as of 2.6.35). Whether that's fully
>> appropriate to be there and/or to be used as a modprobe
>> argument I'm not sure though.
>> 
>> The bad thing about the "blktap" name is that that's also the
>> name of the blktap1 driver in the 2.6.18 tree and its forward
>> ports, but I don't think there's anything we can reasonably do
>> about that.
> 
> I thought about that. Most kernels which have blktap1 nowadays also have
> blktap2 so the number of systems where you would actually end up with
> only blktap1 loaded is pretty small. It's also AFAIK reasonably harmless
> other than the memory usage etc.
> 
> In retrospect renaming blktap2->blktap ni pvops was a stupid idea (I can
> say that since it was my idea...)
> 
>>  So I'm fine with the change you suggest from that
>> perspective (whether to use the module alias pointed out ).
> 
> Can I take that as an
> Acked-by: Jan Beulich <JBeulich@suse.com>
> ?

Yes, feel free to do so.

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Fri, 2012-08-31 at 10:49 +0100, Jan Beulich wrote:
> > Can I take that as an
> > Acked-by: Jan Beulich <JBeulich@suse.com>
> > ?
> 
> Yes, feel free to do so.

Thanks, applied.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Tue, 2012-05-15 at 17:21 +0100, George Dunlap wrote:
> On 15/05/12 17:16, Jan Beulich wrote:
> >>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
> >> Older kernels, such as those found in Debian Squeeze:
> >> * Have bugs in handling of AIO into foreign pages
> >> * Have blktap modules, which will cause qemu not to use AIO, but
> >> which are not loaded on boot.
> >>
> >> Attempt to load blktap in xencommons, to make sure modern qemu's which
> >> use AIO will work properly on those kernels.
> >>
> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
> >>
> >> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
> >> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
> >> @@ -59,6 +59,7 @@ do_start () {
> >>   	modprobe evtchn 2>/dev/null
> >>   	modprobe gntdev 2>/dev/null
> >>   	modprobe xen-acpi-processor 2>/dev/null
> >> +	modprobe blktap 2>/dev/null
> > Can we stop manually loading all kinds of drivers here? I was
> > glad this went away with the switch to xencommons, and
> > now this is coming back. Drivers definitely needed in all cases
> > are acceptable imo, but backend drivers should be loaded as
> > backends get created by the tools (similarly frontend drivers
> > for the local attach case, though they should get auto-loaded
> > normally anyway).
> I tend to agree with you; I did it this way because that's what was 
> suggested to me.  But I don't at the moment know enough about the 
> backend creation stuff in xl / qemu to DTRT here.

The issue preventing autoloading here is that blktap is effectively
optional and libxl tries to do a best effort search for a usable disk
backend. If blktap is loaded the libxl will choose it, otherwise it will
fallback to qdisk. The problem is that if blktap is available but not
loaded then that is something which libxl cannot detect, I'm not sure
how we could go about fixing that.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 16.05.12 at 10:21, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Tue, 2012-05-15 at 17:21 +0100, George Dunlap wrote:
>> On 15/05/12 17:16, Jan Beulich wrote:
>> >>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>> >> Older kernels, such as those found in Debian Squeeze:
>> >> * Have bugs in handling of AIO into foreign pages
>> >> * Have blktap modules, which will cause qemu not to use AIO, but
>> >> which are not loaded on boot.
>> >>
>> >> Attempt to load blktap in xencommons, to make sure modern qemu's which
>> >> use AIO will work properly on those kernels.
>> >>
>> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>> >>
>> >> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
>> >> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
>> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
>> >> @@ -59,6 +59,7 @@ do_start () {
>> >>   	modprobe evtchn 2>/dev/null
>> >>   	modprobe gntdev 2>/dev/null
>> >>   	modprobe xen-acpi-processor 2>/dev/null
>> >> +	modprobe blktap 2>/dev/null
>> > Can we stop manually loading all kinds of drivers here? I was
>> > glad this went away with the switch to xencommons, and
>> > now this is coming back. Drivers definitely needed in all cases
>> > are acceptable imo, but backend drivers should be loaded as
>> > backends get created by the tools (similarly frontend drivers
>> > for the local attach case, though they should get auto-loaded
>> > normally anyway).
>> I tend to agree with you; I did it this way because that's what was 
>> suggested to me.  But I don't at the moment know enough about the 
>> backend creation stuff in xl / qemu to DTRT here.
> 
> The issue preventing autoloading here is that blktap is effectively
> optional and libxl tries to do a best effort search for a usable disk
> backend. If blktap is loaded the libxl will choose it, otherwise it will
> fallback to qdisk. The problem is that if blktap is available but not
> loaded then that is something which libxl cannot detect, I'm not sure
> how we could go about fixing that.

Why not simply run a (series of) modprobe(s) from there? Or is
that precluded by not being OS-neutral?

The same would obviously apply to other backends (netback most
notably).

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Wed, 2012-05-16 at 09:58 +0100, Jan Beulich wrote:
> >>> On 16.05.12 at 10:21, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Tue, 2012-05-15 at 17:21 +0100, George Dunlap wrote:
> >> On 15/05/12 17:16, Jan Beulich wrote:
> >> >>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
> >> >> Older kernels, such as those found in Debian Squeeze:
> >> >> * Have bugs in handling of AIO into foreign pages
> >> >> * Have blktap modules, which will cause qemu not to use AIO, but
> >> >> which are not loaded on boot.
> >> >>
> >> >> Attempt to load blktap in xencommons, to make sure modern qemu's which
> >> >> use AIO will work properly on those kernels.
> >> >>
> >> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
> >> >>
> >> >> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
> >> >> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
> >> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
> >> >> @@ -59,6 +59,7 @@ do_start () {
> >> >>   	modprobe evtchn 2>/dev/null
> >> >>   	modprobe gntdev 2>/dev/null
> >> >>   	modprobe xen-acpi-processor 2>/dev/null
> >> >> +	modprobe blktap 2>/dev/null
> >> > Can we stop manually loading all kinds of drivers here? I was
> >> > glad this went away with the switch to xencommons, and
> >> > now this is coming back. Drivers definitely needed in all cases
> >> > are acceptable imo, but backend drivers should be loaded as
> >> > backends get created by the tools (similarly frontend drivers
> >> > for the local attach case, though they should get auto-loaded
> >> > normally anyway).
> >> I tend to agree with you; I did it this way because that's what was 
> >> suggested to me.  But I don't at the moment know enough about the 
> >> backend creation stuff in xl / qemu to DTRT here.
> > 
> > The issue preventing autoloading here is that blktap is effectively
> > optional and libxl tries to do a best effort search for a usable disk
> > backend. If blktap is loaded the libxl will choose it, otherwise it will
> > fallback to qdisk. The problem is that if blktap is available but not
> > loaded then that is something which libxl cannot detect, I'm not sure
> > how we could go about fixing that.
> 
> Why not simply run a (series of) modprobe(s) from there? Or is
> that precluded by not being OS-neutral?

An interesting idea.

The blktap detection code is necessarily OS-specific. I previously
discounted it because of the possibility of a race between the
completion of modprobe and the driver actually registering enough for
detection to succeed. Maybe I was too pessimistic or someone has a
bright idea?

> The same would obviously apply to other backends (netback most
> notably).

We use netback unconditionally (there are no alternatives, at least not
ones we want to use (does qnet exist? not sure)) so I think the normal
driver autoloading ought to work there -- at least on kernels where it
exists, it was a relatively recent addition IIRC.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Jan Beulich]

>>> On 16.05.12 at 11:16, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-05-16 at 09:58 +0100, Jan Beulich wrote:
>> >>> On 16.05.12 at 10:21, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> > On Tue, 2012-05-15 at 17:21 +0100, George Dunlap wrote:
>> >> On 15/05/12 17:16, Jan Beulich wrote:
>> >> >>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
>> >> >> Older kernels, such as those found in Debian Squeeze:
>> >> >> * Have bugs in handling of AIO into foreign pages
>> >> >> * Have blktap modules, which will cause qemu not to use AIO, but
>> >> >> which are not loaded on boot.
>> >> >>
>> >> >> Attempt to load blktap in xencommons, to make sure modern qemu's which
>> >> >> use AIO will work properly on those kernels.
>> >> >>
>> >> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
>> >> >>
>> >> >> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
>> >> >> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
>> >> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
>> >> >> @@ -59,6 +59,7 @@ do_start () {
>> >> >>   	modprobe evtchn 2>/dev/null
>> >> >>   	modprobe gntdev 2>/dev/null
>> >> >>   	modprobe xen-acpi-processor 2>/dev/null
>> >> >> +	modprobe blktap 2>/dev/null
>> >> > Can we stop manually loading all kinds of drivers here? I was
>> >> > glad this went away with the switch to xencommons, and
>> >> > now this is coming back. Drivers definitely needed in all cases
>> >> > are acceptable imo, but backend drivers should be loaded as
>> >> > backends get created by the tools (similarly frontend drivers
>> >> > for the local attach case, though they should get auto-loaded
>> >> > normally anyway).
>> >> I tend to agree with you; I did it this way because that's what was 
>> >> suggested to me.  But I don't at the moment know enough about the 
>> >> backend creation stuff in xl / qemu to DTRT here.
>> > 
>> > The issue preventing autoloading here is that blktap is effectively
>> > optional and libxl tries to do a best effort search for a usable disk
>> > backend. If blktap is loaded the libxl will choose it, otherwise it will
>> > fallback to qdisk. The problem is that if blktap is available but not
>> > loaded then that is something which libxl cannot detect, I'm not sure
>> > how we could go about fixing that.
>> 
>> Why not simply run a (series of) modprobe(s) from there? Or is
>> that precluded by not being OS-neutral?
> 
> An interesting idea.
> 
> The blktap detection code is necessarily OS-specific. I previously
> discounted it because of the possibility of a race between the
> completion of modprobe and the driver actually registering enough for
> detection to succeed. Maybe I was too pessimistic or someone has a
> bright idea?

modprobe only exits when the driver's init function completed,
at which point the driver can be expected to be in a usable state.

>> The same would obviously apply to other backends (netback most
>> notably).
> 
> We use netback unconditionally (there are no alternatives, at least not
> ones we want to use (does qnet exist? not sure)) so I think the normal
> driver autoloading ought to work there -- at least on kernels where it
> exists, it was a relatively recent addition IIRC.

It was a recent addition, and I wonder whether calling this
auto-loading really is the right term (unless I'm mis-understanding,
you're referring to the xen-backend: module aliases) - what would
trigger their loading? My current understanding of this it that it only
allows loading the drivers without referring to a hard-coded driver
module names.

Jan

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Wed, 2012-05-16 at 10:51 +0100, Jan Beulich wrote:
> >>> On 16.05.12 at 11:16, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2012-05-16 at 09:58 +0100, Jan Beulich wrote:
> >> >>> On 16.05.12 at 10:21, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >> > On Tue, 2012-05-15 at 17:21 +0100, George Dunlap wrote:
> >> >> On 15/05/12 17:16, Jan Beulich wrote:
> >> >> >>>> On 15.05.12 at 17:49, George Dunlap<george.dunlap@eu.citrix.com>  wrote:
> >> >> >> Older kernels, such as those found in Debian Squeeze:
> >> >> >> * Have bugs in handling of AIO into foreign pages
> >> >> >> * Have blktap modules, which will cause qemu not to use AIO, but
> >> >> >> which are not loaded on boot.
> >> >> >>
> >> >> >> Attempt to load blktap in xencommons, to make sure modern qemu's which
> >> >> >> use AIO will work properly on those kernels.
> >> >> >>
> >> >> >> Signed-off-by: George Dunlap<george.dunlap@eu.citrix.com>
> >> >> >>
> >> >> >> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
> >> >> >> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
> >> >> >> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
> >> >> >> @@ -59,6 +59,7 @@ do_start () {
> >> >> >>   	modprobe evtchn 2>/dev/null
> >> >> >>   	modprobe gntdev 2>/dev/null
> >> >> >>   	modprobe xen-acpi-processor 2>/dev/null
> >> >> >> +	modprobe blktap 2>/dev/null
> >> >> > Can we stop manually loading all kinds of drivers here? I was
> >> >> > glad this went away with the switch to xencommons, and
> >> >> > now this is coming back. Drivers definitely needed in all cases
> >> >> > are acceptable imo, but backend drivers should be loaded as
> >> >> > backends get created by the tools (similarly frontend drivers
> >> >> > for the local attach case, though they should get auto-loaded
> >> >> > normally anyway).
> >> >> I tend to agree with you; I did it this way because that's what was 
> >> >> suggested to me.  But I don't at the moment know enough about the 
> >> >> backend creation stuff in xl / qemu to DTRT here.
> >> > 
> >> > The issue preventing autoloading here is that blktap is effectively
> >> > optional and libxl tries to do a best effort search for a usable disk
> >> > backend. If blktap is loaded the libxl will choose it, otherwise it will
> >> > fallback to qdisk. The problem is that if blktap is available but not
> >> > loaded then that is something which libxl cannot detect, I'm not sure
> >> > how we could go about fixing that.
> >> 
> >> Why not simply run a (series of) modprobe(s) from there? Or is
> >> that precluded by not being OS-neutral?
> > 
> > An interesting idea.
> > 
> > The blktap detection code is necessarily OS-specific. I previously
> > discounted it because of the possibility of a race between the
> > completion of modprobe and the driver actually registering enough for
> > detection to succeed. Maybe I was too pessimistic or someone has a
> > bright idea?
> 
> modprobe only exits when the driver's init function completed,
> at which point the driver can be expected to be in a usable state.

OK, so maybe that works then.

> >> The same would obviously apply to other backends (netback most
> >> notably).
> > 
> > We use netback unconditionally (there are no alternatives, at least not
> > ones we want to use (does qnet exist? not sure)) so I think the normal
> > driver autoloading ought to work there -- at least on kernels where it
> > exists, it was a relatively recent addition IIRC.
> 
> It was a recent addition, and I wonder whether calling this
> auto-loading really is the right term (unless I'm mis-understanding,
> you're referring to the xen-backend: module aliases) - what would
> trigger their loading? My current understanding of this it that it only
> allows loading the drivers without referring to a hard-coded driver
> module names.

My understanding was that creating a xenstore backend path would cause
the core generic xenstore backend code in the kernel to go "ah, I need a
driver for foo" (where foo is the backend name) and if one is not
already registered it will do the "modprobe foo" userspace event, which
the modaliases then cause to pickup the right kernel module.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[George Dunlap]

On 16/05/12 10:57, Ian Campbell wrote:
>>> The blktap detection code is necessarily OS-specific. I previously
>>> discounted it because of the possibility of a race between the
>>> completion of modprobe and the driver actually registering enough for
>>> detection to succeed. Maybe I was too pessimistic or someone has a
>>> bright idea?
>> modprobe only exits when the driver's init function completed,
>> at which point the driver can be expected to be in a usable state.
> OK, so maybe that works then.
So what's the plan?  Options seem to be:
1. Put this on the blocker list, so it definitely gets done before release
2. Accept the patch that started this thread (or a version of it), and 
put "do it right" on the "nice-to-have" list.  I can do it if I get a 
chance.
3. Someone can volunteer who can prioritize it.

  -George

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Wed, 2012-05-16 at 11:49 +0100, George Dunlap wrote:
> On 16/05/12 10:57, Ian Campbell wrote:
> >>> The blktap detection code is necessarily OS-specific. I previously
> >>> discounted it because of the possibility of a race between the
> >>> completion of modprobe and the driver actually registering enough for
> >>> detection to succeed. Maybe I was too pessimistic or someone has a
> >>> bright idea?
> >> modprobe only exits when the driver's init function completed,
> >> at which point the driver can be expected to be in a usable state.
> > OK, so maybe that works then.
> So what's the plan?  Options seem to be:
> 1. Put this on the blocker list, so it definitely gets done before release
> 2. Accept the patch that started this thread (or a version of it), and 
> put "do it right" on the "nice-to-have" list.  I can do it if I get a 
> chance.
> 3. Someone can volunteer who can prioritize it.

My preference, in decreasing order would be 2, 3, 1.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xencommons: Attempt to load blktap driver"):
> On Wed, 2012-05-16 at 11:49 +0100, George Dunlap wrote:
> > On 16/05/12 10:57, Ian Campbell wrote:
> > >>> The blktap detection code is necessarily OS-specific. I previously
> > >>> discounted it because of the possibility of a race between the
> > >>> completion of modprobe and the driver actually registering enough for
> > >>> detection to succeed. Maybe I was too pessimistic or someone has a
> > >>> bright idea?
> > >> modprobe only exits when the driver's init function completed,
> > >> at which point the driver can be expected to be in a usable state.
> > > OK, so maybe that works then.
> > So what's the plan?  Options seem to be:
> > 1. Put this on the blocker list, so it definitely gets done before release
> > 2. Accept the patch that started this thread (or a version of it), and 
> > put "do it right" on the "nice-to-have" list.  I can do it if I get a 
> > chance.
> > 3. Someone can volunteer who can prioritize it.
> 
> My preference, in decreasing order would be 2, 3, 1.

AFAICT this is still outstanding.  Have we made a decision ?

I'm inclined to throw all of these best-effort modprobes into 4.2 and
try to sort out something saner (if possible) in 4.3.

Ian: can you make sure this is on the 4.2 TODO ?

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Fri, 2012-06-08 at 16:35 +0100, Ian Jackson wrote:
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xencommons: Attempt to load blktap driver"):
> > On Wed, 2012-05-16 at 11:49 +0100, George Dunlap wrote:
> > > On 16/05/12 10:57, Ian Campbell wrote:
> > > >>> The blktap detection code is necessarily OS-specific. I previously
> > > >>> discounted it because of the possibility of a race between the
> > > >>> completion of modprobe and the driver actually registering enough for
> > > >>> detection to succeed. Maybe I was too pessimistic or someone has a
> > > >>> bright idea?
> > > >> modprobe only exits when the driver's init function completed,
> > > >> at which point the driver can be expected to be in a usable state.
> > > > OK, so maybe that works then.
> > > So what's the plan?  Options seem to be:
> > > 1. Put this on the blocker list, so it definitely gets done before release
> > > 2. Accept the patch that started this thread (or a version of it), and 
> > > put "do it right" on the "nice-to-have" list.  I can do it if I get a 
> > > chance.
> > > 3. Someone can volunteer who can prioritize it.
> > 
> > My preference, in decreasing order would be 2, 3, 1.
> 
> AFAICT this is still outstanding.  Have we made a decision ?
> 
> I'm inclined to throw all of these best-effort modprobes into 4.2 and
> try to sort out something saner (if possible) in 4.3.
> 
> Ian: can you make sure this is on the 4.2 TODO ?

I've added, to tools nice to have:

    * Load blktap driver from xencommons initscript if available, thread at:
      <db614e92faf743e20b3f.1337096977@kodo2>. To be fixed more
      properly in 4.3

I'll post an update of the TODO list after I've processed my
post-vacation email backlog.

Ian.

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Campbell]

On Tue, 2012-05-15 at 16:49 +0100, George Dunlap wrote:
> Older kernels, such as those found in Debian Squeeze:
> * Have bugs in handling of AIO into foreign pages
> * Have blktap modules, which will cause qemu not to use AIO, but
> which are not loaded on boot.
> 
> Attempt to load blktap in xencommons, to make sure modern qemu's which
> use AIO will work properly on those kernels.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Rereading the thread which this patch spawned I think the conclusion was
that we should take this patch (or a version of it) for 4.2. I think the
"or a version of it" refers to the selection of the precise set of names
which we need to try to get the best chance of loading something useful.
But I'm not sure what the conclusion was in that regard.

Ian.

> 
> diff -r 99244350516a -r db614e92faf7 tools/hotplug/Linux/init.d/xencommons
> --- a/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:48:49 2012 +0100
> +++ b/tools/hotplug/Linux/init.d/xencommons	Tue May 15 16:49:32 2012 +0100
> @@ -59,6 +59,7 @@ do_start () {
>  	modprobe evtchn 2>/dev/null
>  	modprobe gntdev 2>/dev/null
>  	modprobe xen-acpi-processor 2>/dev/null
> +	modprobe blktap 2>/dev/null
>  	mkdir -p /var/run/xen
>  
>  	if ! `xenstore-read -s / >/dev/null 2>&1`
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] xencommons: Attempt to load blktap driver

[Ian Jackson]

George Dunlap writes ("[Xen-devel] [PATCH] xencommons: Attempt to load blktap driver"):
> Older kernels, such as those found in Debian Squeeze:
> * Have bugs in handling of AIO into foreign pages
> * Have blktap modules, which will cause qemu not to use AIO, but
> which are not loaded on boot.
> 
> Attempt to load blktap in xencommons, to make sure modern qemu's which
> use AIO will work properly on those kernels.
> 
> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>

Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>

For 4.2.

Ian.