Re: [PATCH 4/6] libxl: Use the block-tap script for LIBXL_DISK_BACKEND_TAP

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

On 07/07/2015 03:20 PM, Ian Campbell wrote:
> On Tue, 2015-07-07 at 14:41 +0100, George Dunlap wrote:
>> On 07/07/2015 01:29 PM, Wei Liu wrote:
>>> On Mon, Jul 06, 2015 at 11:51:41AM +0100, George Dunlap wrote:
>>>> The block-tap script can now do everything needed for libxl; no need
>>>> to link against the blktap library.
>>>>
>>>> To do this:
>>>>
>>>> * Set disk->script to "block-tap" and dev to "format:pdev_path" in
>>>>   device_disk_add for LIBXL_DISK_BACKEND_TAP
>>>>
>>>> * Remove libxl_blktap2.o and libxl_noblktap2.o and all code depending
>>>>   on them
>>>>
>>>> Signed-off-by: George Dunlap <george.dunlap@eu.citrix.com>
>>>> ---
>>>> CC: Ian Campbell <ian.campbell@citrix.com>
>>>> CC: Ian Jackson <ian.jackson@citrix.com>
>>>> CC: Wei Liu <wei.liu2@citrix.com>
>>>> CC: Dave Scott <Dave.Scott@eu.citrix.com>
>>>> CC: Jonathan Ludlam <Jonathan.Ludlam@eu.citrix.com>
>>>> CC: Wen Congyang <wency@cn.fujitsu.com>
>>>> CC: Yang Hongyang <yanghy@cn.fujitsu.com>
>>>> CC: Felipe Franciosi <felipe.franciosi@citrix.com>
>>>>
>>>> Note: This is broken for HVM domains at the moment because all block
>>>> scripts are broken for HVM domains.  That is a bug which should be a
>>>> blocker for the 4.6 release.  As such, I think there is justification
>>>
>>> This bug is partially fixed by this particular patch. I think making
>>> things partially work is better than having it not work at all.
>>>
>>>> for checking in this "feature" (enabling use of a 'system' blktap)
>>>> with the assumption that the FIXME will go away before the release.
>>>>
>>>
>>> I have the impression that to make QEMU work with block script (the
>>> FIXME) would require us to devise a way to get back the path of block
>>> device node. Am I right? How much work do you envisage is needed to make
>>> that FIXME go away?
>>
>> For block scripts where dom0 is the backend, I *think* we should in
>> theory be able change block-common to store a pathname to the device
>> node in xenstore and hand it to qemu.  Alternately, we could fish the
>> existing major:minor numbers out of xenstore and make our own node to
>> pass to qemu.
>>
>>> On the flip side we're not making anything worse than before so we can
>>> probably get by by writing that down in known issues section of release
>>> note.
>>
>> We do technically cause a regression at the moment.  Before this patch,
>> someone who has access to the blktap kernel module, and is using the
>> in-tree blktap, will be able to use a blktap backend with HVM guests.
>> Once this patch is accepted, blktap will work for PV guests, but not for
>> HVM guests -- at least until we fix the bug with local block scripts.
> 
> Unfortunately that does sound like something which needs to be fixed
> before we accept this patch, doesn't it?

Since the bug is in block scripts in general, and should (I think) be a
blocker, it can go in after the feature freeze.  This being a new
feature, it can't (or at least, that's how I was doing my calculations).
 So I didn't want to block the thing I can't work on during the feature
freeze for the thing I can.

On the other hand, there's always a risk that the fix will be somewhat
complicated, and we may decide that since nobody has been shouting about
it so far, we could just put the fix off until 4.6.1 -- or at least, if
we don't have any visible regressions, we could do that.  Checking this
in might burn our bridges somewhat.

I think that risk is small, so I'm still in favor of checking it in, but
I could certainly see the the argument against it.

 -George