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

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

[David Scott]

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@eu.citrix.com>
Acked-by: Ian Campbell <ian.campbell@citrix.com>
---
 tools/libxl/libxl_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eeea9d9..eb60fd5 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -236,7 +236,9 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
             return ERROR_INVAL;
         }
         memset(&a.stab, 0, sizeof(a.stab));
-    } else if (!disk->script) {
+    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY &&
+               disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
+               !disk->script) {
         if (stat(disk->pdev_path, &a.stab)) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
                              "failed to stat: %s",
-- 
1.8.1.2

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

[Roger Pau Monné]

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@eu.citrix.com>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Acked-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
>  tools/libxl/libxl_device.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index eeea9d9..eb60fd5 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -236,7 +236,9 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
>              return ERROR_INVAL;
>          }
>          memset(&a.stab, 0, sizeof(a.stab));
> -    } else if (!disk->script) {
> +    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY &&
> +               disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
> +               !disk->script) {
>          if (stat(disk->pdev_path, &a.stab)) {
>              LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
>                               "failed to stat: %s",
> 

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

[Ian Campbell]

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@eu.citrix.com>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

Applied, thanks.



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

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

[Ian Campbell]

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@eu.citrix.com>
> > > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> > 
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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...

Ian.



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

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

[David Scott]

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@eu.citrix.com>
>>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> 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?

Perhaps a better patch would do the stat() anyway and only worry about 
the failure if (disk->backend = ... PHY && disk->backend_domid = ... etc)


Cheers,
Dave

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

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

[Ian Campbell]

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@eu.citrix.com>
> >>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
> >>>
> >>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> >>
> >> 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...

Ian.


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

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

[David Scott]

[-- Attachment #1: Type: text/plain, Size: 3020 bytes --]

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@eu.citrix.com>
>>>>>> Acked-by: Ian Campbell <ian.campbell@citrix.com>
>>>>>
>>>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>>>
>>>> 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 #2: libxl-device-stat.patch --]
[-- Type: text/x-patch, Size: 3275 bytes --]

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eb60fd5..47b5afc 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -143,7 +143,6 @@ retry_transaction:
 typedef struct {
     libxl__gc *gc;
     libxl_device_disk *disk;
-    struct stat stab;
 } disk_try_backend_args;
 
 static int disk_try_backend(disk_try_backend_args *a,
@@ -153,6 +152,7 @@ static int disk_try_backend(disk_try_backend_args *a,
     /* returns 0 (ie, DISK_BACKEND_UNKNOWN) on failure, or
      * backend on success */
     libxl_ctx *ctx = libxl__gc_owner(gc);
+    struct stat stab;
 
     switch (backend) {
     case LIBXL_DISK_BACKEND_PHY:
@@ -167,13 +167,21 @@ static int disk_try_backend(disk_try_backend_args *a,
             return backend;
         }
 
-        if (libxl__try_phy_backend(a->stab.st_mode))
-            return backend;
+        if (a->disk->backend_domid == LIBXL_TOOLSTACK_DOMID) {
+            if (stat(a->disk->pdev_path, &stab)) {
+                LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
+                                 "failed to stat: %s",
+                                 a->disk->vdev, a->disk->pdev_path);
+                return 0;
+            }
 
-        LIBXL__LOG(ctx, LIBXL__LOG_DEBUG, "Disk vdev=%s, backend phy"
-                   " unsuitable as phys path not a block device",
+            if (libxl__try_phy_backend(stab.st_mode))
+                return backend;
+        }
+
+        LOG(DEBUG, "Disk vdev=%s, uses driver domain, assuming phy backend",
                    a->disk->vdev);
-        return 0;
+        return backend;
 
     case LIBXL_DISK_BACKEND_TAP:
         if (a->disk->script) goto bad_script;
@@ -228,30 +236,11 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_device_disk *disk) {
                disk->vdev,
                libxl_disk_backend_to_string(disk->backend));
 
-    if (disk->format == LIBXL_DISK_FORMAT_EMPTY) {
-        if (!disk->is_cdrom) {
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
-                       " but not cdrom",
-                       disk->vdev);
-            return ERROR_INVAL;
-        }
-        memset(&a.stab, 0, sizeof(a.stab));
-    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY &&
-               disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
-               !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;
-        }
-        if (!S_ISBLK(a.stab.st_mode) &
-            !S_ISREG(a.stab.st_mode)) {
-            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
-                             "phys path is not a block dev or file: %s",
-                             disk->vdev, disk->pdev_path);
-            return ERROR_INVAL;
-        }
+    if (disk->format == LIBXL_DISK_FORMAT_EMPTY && !disk->is_cdrom) {
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s is empty"
+                   " but not cdrom",
+                   disk->vdev);
+        return ERROR_INVAL;
     }
 
     if (disk->backend != LIBXL_DISK_BACKEND_UNKNOWN) {

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

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

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

[Ian Campbell]

On Fri, 2013-04-26 at 13:17 +0100, Dave Scott wrote:

> 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)

Right, this is the case which concerned me. Roger probably has a better
handler than me but I think it is probably true to say that at least as
things stand phy was the only option in a driver domain. Maybe blktap1
(which libxl doesn't support) would have worked but blktap2 won't and
neither will qdisk.

I was looking at this as the most minimal fix:
@@ -236,9 +236,10 @@ int libxl__device_disk_set_backend(libxl__gc *gc, libxl_de
             return ERROR_INVAL;
         }
         memset(&a.stab, 0, sizeof(a.stab));
-    } else if (disk->backend == LIBXL_DISK_BACKEND_PHY &&
+    } else if ((disk->backend == LIBXL_DISK_BACKEND_PHY ||
+               disk->backend == LIBXL_DISK_BACKEND_UNKNOWN) &&
                disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
                !disk->script) {
         if (stat(disk->pdev_path, &a.stab)) {
             LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "Disk vdev=%s "
                              "failed to stat: %s",

but I'm not 100% sure of this. 

If it is right the arguable TAP should be included too, it's only qdisk
which can deal with things which aren't block or file (so the == PHY
should be a != QDISK).

Which begs the question whether your variant shouldn't also add a stat
in the tap case too?

> 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.

I think it might...

> 
> Cheers,
> Dave

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

[Ian Campbell]

On Fri, 2013-04-26 at 14:11 +0100, Ian Campbell wrote:
> If it is right the arguable TAP should be included too, it's only
> qdisk
> which can deal with things which aren't block or file (so the == PHY
> should be a != QDISK). 

I've just sent out "libxl: stat the path for all non-qdisk backends
(including unknown)" which I think is the minimal fix, but we should
perhaps consider your approach after that too.

Ian.