From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Scott Subject: Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist. Date: Fri, 26 Apr 2013 13:17:09 +0100 Message-ID: <517A7045.9010107@eu.citrix.com> References: <1366711166-28296-1-git-send-email-dave.scott@eu.citrix.com> <517695F1.4000206@citrix.com> <1366804610.20256.300.camel@zakaz.uk.xensource.com> <1366973715.3142.66.camel@zakaz.uk.xensource.com> <517A5FDD.6090309@eu.citrix.com> <1366975060.3142.68.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------010609040007010702030608" Return-path: In-Reply-To: <1366975060.3142.68.camel@zakaz.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: "xen-devel@lists.xen.org" , Roger Pau Monne List-Id: xen-devel@lists.xenproject.org --------------010609040007010702030608 Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 8bit 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 >>>>>> Acked-by: Ian Campbell >>>>> >>>>> Acked-by: Roger Pau Monné >>>> >>>> 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 --------------010609040007010702030608 Content-Type: text/x-patch; name="libxl-device-stat.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="libxl-device-stat.patch" 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) { --------------010609040007010702030608 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --------------010609040007010702030608--