Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> 
> Stefano - Thanks, this is great information that I couldn't find before!  
> Rest in-line.
> 
> > This patch made me realize that we really need a specification for the
> > disk format syntax in the config file and a better xl parser to match it.
> > The syntax should be clear and at the same time backward compatible as
> > much as we can.
> > The followings are disk configurations supported in libxl and what they
> > mean:
> > 
> > - phy:/path/to/device
> > blkback is used as block backend, if blkback is missing from your dom0
> > kernel, this configuration will fail.
> So, we add a check in libxl validate disk code to find whether or not blkback 
> is present rather than go all the way and fail?
> 

Yeah, you can check for the presence of:

/sys/modules/{blkbk,xen_blkback}

the modules are called differently in pvops compared to traditional
xenolinux kernels.


> > A block device must be specified because blkback cannot handle files by
> > itself.
> > The format must be raw.
> > 
> We probably need to extend the validation to confirm what is passed is a 
> block device and not a file and I will add that.

Right.

> > - file:/path/to/file
> > tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
> > instead.
> > Both a file or a block device can be specified.
> > The format is raw.
> > 
> > - tap:/path/to/file
> > same as file:
> > 
> > - tap:qcow:/path/to/file
> > qemu is used as block backend because tapdisk2 has broken qcow support.
> Maybe we should explicitly fail this case and let the user know that tapdisk2 
> is broken with qcow so they change the prefix in the config file.  
> Alternately, we could simply turn the error into a warning and keep going 
> with qemu.
> 

I think there is nothing wrong in using the qemu disk backend,
especially when the new qemu will be available with much better aio
support.
I also think that we shouldn't bother the users with implementation
details on the disk backend we use, the toolstack should just set it up
behind the scene.
So in the qcow and qcow2 cases I wouldn't print anything at all.
However in the vhd case, considering that I have never tested qemu's vhd
support, I don't know how that would actually work. So for the vhd
format it might be OK to just print an error an exit if tapdisk2 is not
present.


> > Both a file or a block device can be specified.
> > The format is vhd.
> > 
> > 
> > The previous cases cover all the supported disk formats, but for
> > backward compatibility we also support the following disk configurations:
> > 
> > - tap:aio:, tap:aio:qcow:, tap:aio:qcow2:, tap:aio:vhd:
> > considering that aio is just an implementation detail, it should not be
> > part of a user exposed interface, in the xl/libxenlight context aio is
> > miningless. aio: should just be ignored by the xl config parser.
> >
> This would require a bit of refactoring in the parsing code to simply skip 
> past aio.  Since we are not accepting interface changes for 4.1, this 
> refactoring would have to wait till post 4.1?
>  

I think this fix can be considered critical so we'll accept it for 4.1.


> > - tap:tapdisk:
> > exactly as for aio:, tapdisk: should just be ignored.
> > 
> I don't think our disk parsing code even considers this option at the moment. 
>  So, we are probably failing when this option is specified.  We would need to 
> tweak the parsing code to parse and ignore this option for backward 
> compatibility.
> 

Right.


> > 
> > Following these notes, we need:
> > 
> > - a document describing this in full details.
> Would a txt file do?  Where would it go - under tools/examples where other 
> config options are described (albeit in a config file and not a separate 
> file)?

Yep. A text file under docs will be perfect.


 
> > - A fix for the usage of aio in libxl: aio is currently considered a
> > type and should be renamed PHYSTYPE_RAW.
> > 
> Of course.  This is the interface change I mentioned above that IanJ might 
> not take before 4.1.

We'll take it because it is a critical fix. Xl has to be compatibile
with xend.

> 
> > - A fix for libxl_device_disk_add so that QCOW and QCOW2 are
> > handled by qemu.
> >
> Is you patch below sufficient for that?  Or, is there more that I need done 
> that I am missing?
>  

I think the patch below should be sufficient, once the disk parsing is
fixed (in particular "aio:" is removed).

> > All these fixes don't have to be separate patches, some of them might
> > be collapsed in a sigle patch.
> > 
> > The last fix should be something like this:
> >
> 
> Last question -  Are we looking to merge these patches for 4.1 or is this for 
> post 4.1?
> 

the target is 4.1

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel