libxl_device.c, stat() and remote disks

libxl_device.c, stat() and remote disks

[David Scott]

Hi,

I've got a virtual disk stored on a ceph cluster and am trying to
use qemu's built-in support for the ceph RBD protocol.

However I notice in libxl_device.c:libxl__device_disk_set_backend
there's an attempt to stat() the VM's disk, which fails because the
disk is remote: there's never any file or device on the local system:

     } else if (!disk->script) {
         if (stat(disk->pdev_path, &a.stab)) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
                              "failed to stat: %s",
                              disk->vdev, disk->pdev_path);
             return ERROR_INVAL;
         }

[At this point I should confess that I did my testing with xen-4.2.1
rather than -unstable, but I didn't spot any relevant changes to 
libxl_device.c]

My attempts to 'xl create' the VM fail as expected with:

   # xl create ubuntu/ceph-ubuntu1204.cfg
   Parsing config from ubuntu/ceph-ubuntu1204.cfg
   libxl: error: libxl_device.c:243:libxl__device_disk_set_backend: Disk 
vdev=hda failed to stat: rbd:rbd/ubuntu1204.img: No such file or directory

I tried to work around the error by adding a dummy hotplug script, but
this is incompatible with the qdisk backend:

     case LIBXL_DISK_BACKEND_QDISK:
         if (a->disk->script) goto bad_script;
         return backend;

However if I work around the stat() by creating a fake file...

   # mkdir "rbd:rbd"
   # touch rbd\:rbd/ubuntu1204.img

... then it now works:

   # xl create ubuntu/ceph-ubuntu1204.cfg
   Parsing config from ubuntu/ceph-ubuntu1204.cfg
   xc: info: VIRTUAL MEMORY ARRANGEMENT:
     Loader:        0000000000100000->000000000019bb24
     TOTAL:         0000000000000000->000000003f800000
     ENTRY ADDRESS: 0000000000100000
   xc: info: PHYSICAL MEMORY ALLOCATION:
     4KB PAGES: 0x0000000000000200
     2MB PAGES: 0x00000000000001fb
     1GB PAGES: 0x0000000000000000
   Daemon running with PID 5895

The resulting qemu has the right command-line argument:

   -drive 
file=rbd:rbd/ubuntu1204.img,if=ide,index=0,media=disk,format=raw,cache=writeback

The qdisk backend also seems operational: (although the "aio:" prefix is
slightly concerning)

   # xenstore-ls /local/domain/0/backend/qdisk/35/768
   frontend = "/local/domain/35/device/vbd/768"
   params = "aio:rbd:rbd/ubuntu1204.img"
   frontend-id = "35"
   online = "1"
   removable = "0"
   bootable = "1"
   state = "4"
   dev = "hda"
   type = "qdisk"
   mode = "w"
   device-type = "disk"
   feature-barrier = "1"
   info = "0"
   sector-size = "512"
   sectors = "33554432"
   hotplug-status = "connected"

My PV on HVM linux VM seems happy: it boots up and successfully mounts 
its root filesystem on /dev/xvda1.

Is it safe to remove the stat() from libxl_device.c?

For reference my xl config file looks like this:

   name="ubuntu1204"
   builder='hvm'
   boot='dc'
   vcpus=1
   memory=1024
   disk=[ 
'backendtype=qdisk,format=raw,vdev=hda,access=rw,target=rbd:rbd/ubuntu1204.img' 
]
   #vif=[ "bridge=br0" ]
   device_model_version='qemu-xen'

Cheers,
Dave

Re: libxl_device.c, stat() and remote disks

[Ian Campbell]

On Mon, 2013-04-22 at 15:07 +0100, David Scott wrote:
> Is it safe to remove the stat() from libxl_device.c? 

I expect it only makes sense when backendtype=phy and perhaps the
disk->script check has served as an imperfect surrogate for that until
now?

Does changing
     } else if (!disk->script) {
into
     } else if (disk->backendtype == ...PHY && !disk->script) {

Work for you also?

Ian.

Re: libxl_device.c, stat() and remote disks

[David Scott]

On 22/04/13 15:36, Ian Campbell wrote:
> On Mon, 2013-04-22 at 15:07 +0100, David Scott wrote:
>> Is it safe to remove the stat() from libxl_device.c?
>
> I expect it only makes sense when backendtype=phy and perhaps the
> disk->script check has served as an imperfect surrogate for that until
> now?
>
> Does changing
>       } else if (!disk->script) {
> into
>       } else if (disk->backendtype == ...PHY && !disk->script) {
>
> Work for you also?

Yes, it works nicely against 4.2.1! The exact patch I applied was:

--- tools/libxl/libxl_device.c.orig     2013-04-22 14:52:54.745001092 +0000
+++ tools/libxl/libxl_device.c  2013-04-22 14:54:11.566001097 +0000
@@ -236,7 +236,7 @@
              return ERROR_INVAL;
          }
          memset(&a.stab, 0, sizeof(a.stab));
-    } else if (!disk->script) {
+    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY && !disk->script) {
          if (stat(disk->pdev_path, &a.stab)) {
              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
                               "failed to stat: %s",

Would you like me to retest this against -unstable? Also would you like 
me to 'git format-patch/send-email' or is it too trivial to bother?

Thanks,
Dave

Re: libxl_device.c, stat() and remote disks

[Ian Campbell]

On Mon, 2013-04-22 at 17:54 +0100, Dave Scott wrote:
> On 22/04/13 15:36, Ian Campbell wrote:
> > On Mon, 2013-04-22 at 15:07 +0100, David Scott wrote:
> >> Is it safe to remove the stat() from libxl_device.c?
> >
> > I expect it only makes sense when backendtype=phy and perhaps the
> > disk->script check has served as an imperfect surrogate for that until
> > now?
> >
> > Does changing
> >       } else if (!disk->script) {
> > into
> >       } else if (disk->backendtype == ...PHY && !disk->script) {
> >
> > Work for you also?
> 
> Yes, it works nicely against 4.2.1! The exact patch I applied was:
> 
> --- tools/libxl/libxl_device.c.orig     2013-04-22 14:52:54.745001092 +0000
> +++ tools/libxl/libxl_device.c  2013-04-22 14:54:11.566001097 +0000
> @@ -236,7 +236,7 @@
>               return ERROR_INVAL;
>           }
>           memset(&a.stab, 0, sizeof(a.stab));
> -    } else if (!disk->script) {
> +    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY && !disk->script) {
>           if (stat(disk->pdev_path, &a.stab)) {
>               LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
>                                "failed to stat: %s",
> 
> Would you like me to retest this against -unstable? Also would you like 
> me to 'git format-patch/send-email' or is it too trivial to bother?

A changelog and a S-o-b should be sufficient, thanks.

However, I think this code is probably broken for driver domains as
well. I think it probably needs a disk->backend_domid check in addition
to what's there -- if you wouldn't mind folding that into the patch that
would be awesome.

There's so many exclusions now I'm wondering if the stat is actually
useful, oh well. I suppose it is still active in some of the more common
cases.

Ian,