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

[David Scott <dave.scott@eu.citrix.com>]

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