Re: [Xen-devel] [RFC] support more qdisk types

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

Ian,

Some surprising behaviour of the libxlu disk spec parser is below...

On Mon, 2016-02-08 at 17:54 -0700, Jim Fehlig wrote:
> On 02/03/2016 07:53 PM, Jim Fehlig wrote:
> > On 02/03/2016 02:56 AM, Ian Campbell wrote:
> > > On Tue, 2016-02-02 at 15:06 -0700, Jim Fehlig wrote:
> > > > > And extending
> > > > > the structure seems to be the right thing to do.
> > > > So what do you think of the approach in the RFC patch? It adds
> > > > discrete knobs in
> > > > the disk config and extends the disk structure similarly. Before I
> > > > can make
> > > > progress on this we need to agree on extending the config and
> > > > libxl_device_disk
> > > > structure.
> > > My main concern is that this approach requires us to update libxl for
> > > each
> > > new possible backend type.
> > Yes, understood.
> > 
> > > The intention of the target= in the disk spec is that it consumes the
> > > rest
> > > of the line so it can be used to encode pretty much anything. Is it
> > > not
> > > possible (modulo bugs) to pass all the necessary information to qdisk
> > > in
> > > this form? I thought Dave S had made it possible to use qdisk in this
> > > way
> > > back in:
> > > 
> > > ÂÂÂÂcommit a8a1f236a2964506a22d1779648d8e1c8668cb1a
> > > ÂÂÂÂAuthor: David Scott <ÂÂÂÂdave.scott@xxxxxxxxxxxxxÂÂÂÂ>
> > > ÂÂÂÂDate:ÂÂÂTue Apr 23 10:59:26 2013 +0100
> > > 
> > > ÂÂÂÂÂÂÂÂlibxl: Only call stat() when adding a disk if we expect a
> > > device to exist.
> > > ÂÂÂÂÂÂÂÂ
> > > ÂÂÂÂÂÂÂÂWe consider calling stat() a helpful error check in the
> > > following
> > > ÂÂÂÂÂÂÂÂcircumstances only:
> > > ÂÂÂÂÂÂÂÂÂ1. the disk backend type must be PHYsical
> > > ÂÂÂÂÂÂÂÂÂ2. the disk backend domain must be the same as the running
> > > libxl
> > > ÂÂÂÂÂÂÂÂÂÂÂÂcode (ie LIBXL_TOOLSTACK_DOMID)
> > > ÂÂÂÂÂÂÂÂÂ3. there must not be a hotplug script because this would
> > > imply that
> > > ÂÂÂÂÂÂÂÂÂÂÂÂthe device won't be created until after the hotplug
> > > script has
> > > ÂÂÂÂÂÂÂÂÂÂÂÂrun.
> > > ÂÂÂÂÂÂÂÂ
> > > ÂÂÂÂÂÂÂÂWith this fix, it is possible to use qemu's built-in block
> > > drivers
> > > ÂÂÂÂÂÂÂÂsuch as ceph/rbd, with a xl config disk spec like this:
> > > ÂÂÂÂÂÂÂÂ
> > > ÂÂÂÂÂÂÂÂdisk=[
> > > 'backendtype=qdisk,format=raw,vdev=hda,access=rw,target=rbd:rbd/ubunt
> > > u1204.img' ]
> > I thought I tried disk config along those lines with no success. But
> > I'll
> > certainly take a closer look at using target= to encode the config
> > needed by
> > these qdisk block drivers. Thanks for the pointer.
> 
> I think my previous problem was related to quoting in a disk spec containing
> several ceph monitors. According to $qemu-src/hw/block/rbd.c, ':' is used to
> delimit option=value tuples. I was escaping the colon between a ceph monitor
> server and port with a single '\'. E.g.
> 
> disk = [ "vdev=xvdb, backendtype=qdisk,
> target=rbd:libvirt-pool/new-libvirt-image:auth_supported=none:mon_host=192.168.0.100\:6789;192.168.0.101\:6789;192.168.0.102\:6789"
> ]
> 
> What I didn't realize was that libxl omitted the entire colon when writing the
> string to xenstore

It's a bit surprising to me that any escaping is required at all in this
context (apart perhaps from escaping any embedded quotes), but it certainly
seems like there must be a bug somewhere if "\:" becomes ""! At the very
least it should become ":", no?

AIUI you really do need the literal "\:" to make it through to the other
end, so it's possible that irrespective of the above oddity what you do
need is "\\:".

> xenstore-read /local/domain/0/backend/qdisk/55/51728/params
> aio:rbd:libvirt-pool/new-libvirt-image:auth_supported=none:mon_host=192.168.0.1006789;192.168.0.1016789;192.168.0.1026789
> 
> which caused the rbd block driver to barf. Using double backslash worked. E.g.
> 
> disk = [ "vdev=xvdb, backendtype=qdisk,
> target=rbd:libvirt-pool/new-libvirt-image:auth_supported=none:mon_host=192.168.0.100\\:6789;192.168.0.101\\:6789;192.168.0.102\\:6789"
> ]
> 
> with corresponding xenstore entry
> 
> aio:rbd:libvirt-pool/new-libvirt-image:auth_supported=none:mon_host=192.168.0.100\:6789;192.168.0.101\:6789;192.168.0.102\:6789
> 
> Note that specifying server:port in nbd doesn't require quoting the colon. The
> following config works just fine
> 
> disk = [ "vdev=xvdb, backendtype=qdisk, target=nbd:192.168.0.150:5555"
> 
> In the end, maybe all that's needed is a few more examples in
> xl-disk-configuration. Perhaps a paragraph under "Special Syntax" of 'target'
> doc, along with example config like the above?

Irrespective of any bug above once we know what is supposed to work and
have fixed it so it does actually work then I agree more examples should be
all which is needed.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel