Re: [Xen-devel] [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.

[David Scott <dave.scott@xxxxxxxxxxxxx>]

On 26/04/13 12:17, Ian Campbell wrote:
> On Fri, 2013-04-26 at 12:07 +0100, Dave Scott wrote:
> > On 26/04/13 11:55, Ian Campbell wrote:
> > > On Wed, 2013-04-24 at 12:56 +0100, Ian Campbell wrote:
> > > > On Tue, 2013-04-23 at 15:08 +0100, Roger Pau Monne wrote:
> > > > > On 23/04/13 11:59, David Scott wrote:
> > > > > > 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/ubuntu1204.img' 
> > > > > > ]
> > > > > >
> > > > > > Signed-off-by: David Scott <dave.scott@xxxxxxxxxxxxx>
> > > > > > Acked-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> > > > > Acked-by: Roger Pau Monnà <roger.pau@xxxxxxxxxx>
> > > > Applied, thanks.
> > > This patch had the slightly unfortunate impact of causing libxl to not
> > > use blkback for actual block devices present in dom0, causing it to fall
> > > back to tap/qemu.
> > >
> > > I'm having a look now...
> > Thanks (and sorry!)
> >
> > Looking at the code again, is it because bypassing the stat() in the
> > UNKNOWN case where in fact it is a block device, leaves a.stab
> > uninitialised which then confuses
> >
> >           if (libxl__try_phy_backend(a->stab.st_mode))
> >               return backend;
> >
> > in disk_try_backend?
> That's my working theory at the minute.
>
> > Perhaps a better patch would do the stat() anyway and only worry about
> > the failure if (disk->backend = ... PHY && disk->backend_domid = ... etc)
> I'm not sure why we can't do the libxl__try_phy_backend at the same time
> as the stat, all it does is abstract the difference away between Linux
> and BSD duplicating a subset of the existing test after the stat...
That's a good point -- we could do only one (platform specific) set of 
S_IS{BLK,REG} checks if we merged them together.
How about a refactoring along the lines of the attached (built but not 
yet tested) patch? The changes are:
1. remove the cached stat results in disk_try_backend_args
2. in disk_try_backend(... PHY) we
   * fail if not RAW or empty (as before)
   * assume phy is ok if we have a hotplug script (as before)
   * if we're in LIBXL_TOOLSTACK_DOMID, stat() the disk and call 
libxl__try_backend
   * if we're not in LIBXL_TOOLSTACK_DOMID then assume phy is ok 
(basically we can't tell for sure)
2. in libxl__device_disk_set_backend we
   * keep the CDROM sanity check
   * don't do any stat()ing
   * immediately call disk_try_backend (probing if UNKNOWN as before)

Does that make sense? I'll do a bit of testing now.

Cheers,
Dave

Attachment: libxl-device-stat.patch
Description: Text Data

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