David Scott
2013-Apr-23 09:59 UTC
[PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
Roger Pau Monné
2013-Apr-23 14:08 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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", >
Ian Campbell
2013-Apr-24 11:56 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
Ian Campbell
2013-Apr-26 10:55 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
David Scott
2013-Apr-26 11:07 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
Ian Campbell
2013-Apr-26 11:17 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
David Scott
2013-Apr-26 12:17 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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 --------------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--
Ian Campbell
2013-Apr-26 13:11 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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
Ian Campbell
2013-Apr-26 14:16 UTC
Re: [PATCH v2] Only call stat() when adding a disk if we expect a device to exist.
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.