Re: [Xen-devel] [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Fri, 2013-05-10 at 16:03 +0100, Dave Scott wrote:
> On 10/05/13 15:42, George Dunlap wrote:
> > On 10/05/13 15:31, Dave Scott wrote:
> >> On 10/05/13 15:19, George Dunlap wrote:
> >>> On 10/05/13 15:09, Dave Scott wrote:
> >>>> On 10/05/13 14:46, George Dunlap wrote:
> >>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> 
> >>>>> wrote:
> >>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> >>>>>>>> Since the intention of that commit was to allow for qdisk backends 
> >>>>>>>> with no
> >>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the 
> >>>>>>>> lowest
> >>>>>>>> impact fix appears to be to make that explicit. This should probably 
> >>>>>>>> be
> >>>>>>>> revisited to rationalize the probing.
> >>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> >>>>>>> already AFAIK
> >>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
> >>>>>>> remote blktap case.
> >>>>>> Right, sounds like I should have gone with my first instinct which was:
> >>>>>>
> >>>>>> 8<------------------------------------------------------------
> >>>>>>
> >>>>>>      From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 
> >>>>>> 2001
> >>>>>> From: Ian Campbell <ian.campbell@xxxxxxxxxx>
> >>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
> >>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends 
> >>>>>> (including unknown)
> >>>>>>
> >>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if 
> >>>>>> we
> >>>>>> expect a device to exist." changed things to only stat the file when 
> >>>>>> the phy
> >>>>>> backend was explicitly requested. This broke the case where we are 
> >>>>>> probing and
> >>>>>> would normally be able to decide on the phy option.
> >>>>> So at the moment qdisk backends aren't checked at all with this --
> >>>>> which means that if you give a path to a file that doesn't exist via,
> >>>>> for example, xl cd-insert, things fail in weird ways:
> >>>>>
> >>>>> 1. In qemu-traditional, the command silently completes; the effect is
> >>>>> that the disk currently in the drive is ejected
> >>>>>
> >>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
> >>>>> is ejected from the guest, but the xenstore entries are not updated
> >>>>> (still contain the old values)
> >>>>>
> >>>>> It seems like we should probably also at least check if disk_format is 
> >>>>> RAW.
> >>>>>
> >>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
> >>>>> listed as "raw" as well?  That doesn't seem right...
> >>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
> >>>> protocol to actually read and write the blocks. I imagine on the ceph
> >>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
> >>>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
> >>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
> >>> *is* just a raw file for these purposes; and we should still be able to
> >>> call stat() on it.
> >> There is no file :) All we have is a "URL"-like thing which looks like:
> >>
> >>      rbd:rbd/foo.img
> >>
> >> which qemu/tapdisk accesses via a C library called librados. The C
> >> library takes the "URL" and makes a network connection to a storage
> >> server and reads and writes blocks over TCP/IP. No filesystems are
> >> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.
> >
> > Right -- so I think this should be a separate type to RAW.  I agree that
> > making a new format for each new protocol isn't very scalable -- but
> > there should be a "URL" or "VIRTUAL" format to indicate that there is no
> > real file.
> 
> Something like that would be fine. I agree "format=raw" is a bit odd, 
> when there's no real "format" involved, only a network protocol.

Yes, something along those lines seems like a good idea.

It's not clear how libxl would know which backend to choose, but I
suppose we either mandate backendtype= or pick qemu as the obvious
default.

> > Right now RAW doesn't seem to be talking about the format, but about the
> > format string; i.e., "I pass this straight into qemu and it figures out
> > what it means."  That's not right.
> >
> > For 4.3 -- I guess the simplest thing to fix the actual bug (xl
> > cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before
> > calling libxl_cdrom_insert().
> 
> That works for me.

Yeah, sounds sensible/pragmatic enough.

Ian.


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