[PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
expect a device to exist." changed things to only stat the file when the phy
backend was explicitly requested. This broke the case where we are probing and
would normally be able to decide on the phy option.

Since the intention of that commit was to allow for qdisk backends with no
explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
impact fix appears to be to make that explicit. This should probably be
revisited to rationalize the probing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: dave.scott@citrix.com
Cc: george.dunlap@citrix.com
Cc: roger.pau@citrix.com
Cc: ian.jackson@citrix.com
---
 tools/libxl/libxl_device.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eb60fd5..b01d109 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -236,7 +236,7 @@ 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->backend == LIBXL_DISK_BACKEND_PHY &&
+    } else if (disk->backend != LIBXL_DISK_BACKEND_QDISK &&
                disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
                !disk->script) {
         if (stat(disk->pdev_path, &a.stab)) {
-- 
1.7.2.5

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Sylvain Munaut]

> Since the intention of that commit was to allow for qdisk backends with no
> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> impact fix appears to be to make that explicit. This should probably be
> revisited to rationalize the probing.

What about the remote disk case of blktap ?  blktap2.5 supports NBD
already AFAIK
and I'm pretty sure I'll hit that same stat issue soon for another
remote blktap case.


Cheers,

   Sylvain

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> > Since the intention of that commit was to allow for qdisk backends with no
> > explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> > impact fix appears to be to make that explicit. This should probably be
> > revisited to rationalize the probing.
> 
> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> already AFAIK
> and I'm pretty sure I'll hit that same stat issue soon for another
> remote blktap case.

Right, sounds like I should have gone with my first instinct which was:

8<------------------------------------------------------------

>From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Fri, 26 Apr 2013 12:41:43 +0100
Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
expect a device to exist." changed things to only stat the file when the phy
backend was explicitly requested. This broke the case where we are probing and
would normally be able to decide on the phy option.

Since the intention of that commit was to allow for backends with no explicit
file in dom0 (i.e. network remote backend such as ceph) the lowest impact fix
appears to be to make that explicit. It turns out that tap disk can also
potentially handle such paths.

The only backend which requires a local file/device is PHY but we need to
handle UNKNOWN too in order for subsequent probing to work. Note that it is
not possible to autoprobe the backend if the path is not a local object, so we
don't need to worry about autoprobing ceph etc.

This should probably be revisited to rationalize the probing.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: dave.scott@citrix.com
Cc: george.dunlap@citrix.com
Cc: roger.pau@citrix.com
Cc: ian.jackson@citrix.com
---
 tools/libxl/libxl_device.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index eb60fd5..a826ece 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -236,7 +236,8 @@ 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->backend == LIBXL_DISK_BACKEND_PHY &&
+    } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
+                disk->backend == LIBXL_DISK_BACKEND_PHY) &&
                disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
                !disk->script) {
         if (stat(disk->pdev_path, &a.stab)) {
-- 
1.7.2.5

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Roger Pau Monné]

On 26/04/13 16:29, Ian Campbell wrote:
> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>> Since the intention of that commit was to allow for qdisk backends with no
>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>> impact fix appears to be to make that explicit. This should probably be
>>> revisited to rationalize the probing.
>>
>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>> already AFAIK
>> and I'm pretty sure I'll hit that same stat issue soon for another
>> remote blktap case.
> 
> Right, sounds like I should have gone with my first instinct which was:
> 
> 8<------------------------------------------------------------
> 
> From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Fri, 26 Apr 2013 12:41:43 +0100
> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
> 
> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> expect a device to exist." changed things to only stat the file when the phy
> backend was explicitly requested. This broke the case where we are probing and
> would normally be able to decide on the phy option.
> 
> Since the intention of that commit was to allow for backends with no explicit
> file in dom0 (i.e. network remote backend such as ceph) the lowest impact fix
> appears to be to make that explicit. It turns out that tap disk can also
> potentially handle such paths.
> 
> The only backend which requires a local file/device is PHY but we need to
> handle UNKNOWN too in order for subsequent probing to work. Note that it is
> not possible to autoprobe the backend if the path is not a local object, so we
> don't need to worry about autoprobing ceph etc.
> 
> This should probably be revisited to rationalize the probing.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

> Cc: dave.scott@citrix.com
> Cc: george.dunlap@citrix.com
> Cc: roger.pau@citrix.com
> Cc: ian.jackson@citrix.com
> ---
>  tools/libxl/libxl_device.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index eb60fd5..a826ece 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -236,7 +236,8 @@ 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->backend == LIBXL_DISK_BACKEND_PHY &&
> +    } else if ((disk->backend == LIBXL_DISK_BACKEND_UNKNOWN ||
> +                disk->backend == LIBXL_DISK_BACKEND_PHY) &&
>                 disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
>                 !disk->script) {
>          if (stat(disk->pdev_path, &a.stab)) {
> 


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

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On 26/04/13 15:39, Roger Pau Monne wrote:
> On 26/04/13 16:29, Ian Campbell wrote:
>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>> impact fix appears to be to make that explicit. This should probably be
>>>> revisited to rationalize the probing.
>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>> already AFAIK
>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>> remote blktap case.
>> Right, sounds like I should have gone with my first instinct which was:
>>
>> 8<------------------------------------------------------------
>>
>>  From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>> From: Ian Campbell <ian.campbell@citrix.com>
>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>
>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>> expect a device to exist." changed things to only stat the file when the phy
>> backend was explicitly requested. This broke the case where we are probing and
>> would normally be able to decide on the phy option.
>>
>> Since the intention of that commit was to allow for backends with no explicit
>> file in dom0 (i.e. network remote backend such as ceph) the lowest impact fix
>> appears to be to make that explicit. It turns out that tap disk can also
>> potentially handle such paths.
>>
>> The only backend which requires a local file/device is PHY but we need to
>> handle UNKNOWN too in order for subsequent probing to work. Note that it is
>> not possible to autoprobe the backend if the path is not a local object, so we
>> don't need to worry about autoprobing ceph etc.
>>
>> This should probably be revisited to rationalize the probing.
>>
>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>

This is a fix, right?  Assuming it is:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

  -George

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

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-04-26 at 15:42 +0100, George Dunlap wrote:

> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This is a fix, right?

Yes, osstest has been failing, bisection in
<E1UVggG-0008W0-B5@woking.cam.xci-test.com> fingered the commit which
this fixes

Ian.



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

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-04-26 at 15:42 +0100, George Dunlap wrote:
> On 26/04/13 15:39, Roger Pau Monne wrote:
> > On 26/04/13 16:29, Ian Campbell wrote:
> >> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> >>>> Since the intention of that commit was to allow for qdisk backends with no
> >>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> >>>> impact fix appears to be to make that explicit. This should probably be
> >>>> revisited to rationalize the probing.
> >>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> >>> already AFAIK
> >>> and I'm pretty sure I'll hit that same stat issue soon for another
> >>> remote blktap case.
> >> Right, sounds like I should have gone with my first instinct which was:
> >>
> >> 8<------------------------------------------------------------
> >>
> >>  From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> >> From: Ian Campbell <ian.campbell@citrix.com>
> >> Date: Fri, 26 Apr 2013 12:41:43 +0100
> >> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
> >>
> >> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> >> expect a device to exist." changed things to only stat the file when the phy
> >> backend was explicitly requested. This broke the case where we are probing and
> >> would normally be able to decide on the phy option.
> >>
> >> Since the intention of that commit was to allow for backends with no explicit
> >> file in dom0 (i.e. network remote backend such as ceph) the lowest impact fix
> >> appears to be to make that explicit. It turns out that tap disk can also
> >> potentially handle such paths.
> >>
> >> The only backend which requires a local file/device is PHY but we need to
> >> handle UNKNOWN too in order for subsequent probing to work. Note that it is
> >> not possible to autoprobe the backend if the path is not a local object, so we
> >> don't need to worry about autoprobing ceph etc.
> >>
> >> This should probably be revisited to rationalize the probing.
> >>
> >> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> This is a fix, right?  Assuming it is:
> 
> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Applied, thanks.



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

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[David Scott]

On 26/04/13 16:33, Ian Campbell wrote:
> On Fri, 2013-04-26 at 15:42 +0100, George Dunlap wrote:
>> On 26/04/13 15:39, Roger Pau Monne wrote:
>>> On 26/04/13 16:29, Ian Campbell wrote:
>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>> revisited to rationalize the probing.
>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>> already AFAIK
>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>> remote blktap case.
>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>
>>>> 8<------------------------------------------------------------
>>>>
>>>>   From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>
>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>> expect a device to exist." changed things to only stat the file when the phy
>>>> backend was explicitly requested. This broke the case where we are probing and
>>>> would normally be able to decide on the phy option.
>>>>
>>>> Since the intention of that commit was to allow for backends with no explicit
>>>> file in dom0 (i.e. network remote backend such as ceph) the lowest impact fix
>>>> appears to be to make that explicit. It turns out that tap disk can also
>>>> potentially handle such paths.
>>>>
>>>> The only backend which requires a local file/device is PHY but we need to
>>>> handle UNKNOWN too in order for subsequent probing to work. Note that it is
>>>> not possible to autoprobe the backend if the path is not a local object, so we
>>>> don't need to worry about autoprobing ceph etc.
>>>>
>>>> This should probably be revisited to rationalize the probing.
>>>>
>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
>>
>> This is a fix, right?  Assuming it is:
>>
>> Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> Applied, thanks.

Thanks for fixing my bug :-)

I've just rebuilt everything and qdisk/RBD still works for me.

Cheers,
Dave



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

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>> > Since the intention of that commit was to allow for qdisk backends with no
>> > explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>> > impact fix appears to be to make that explicit. This should probably be
>> > revisited to rationalize the probing.
>>
>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>> already AFAIK
>> and I'm pretty sure I'll hit that same stat issue soon for another
>> remote blktap case.
>
> Right, sounds like I should have gone with my first instinct which was:
>
> 8<------------------------------------------------------------
>
> From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> From: Ian Campbell <ian.campbell@citrix.com>
> Date: Fri, 26 Apr 2013 12:41:43 +0100
> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>
> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> expect a device to exist." changed things to only stat the file when the phy
> backend was explicitly requested. This broke the case where we are probing and
> would normally be able to decide on the phy option.

So at the moment qdisk backends aren't checked at all with this --
which means that if you give a path to a file that doesn't exist via,
for example, xl cd-insert, things fail in weird ways:

1. In qemu-traditional, the command silently completes; the effect is
that the disk currently in the drive is ejected

2. in qemu-upstream, qmp returns an error which is reported.  The disk
is ejected from the guest, but the xenstore entries are not updated
(still contain the old values)

It seems like we should probably also at least check if disk_format is RAW.

OTOH, I don't seen an option for disk_format to be ceph; is it just
listed as "raw" as well?  That doesn't seem right...

 -George

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-05-10 at 14:46 +0100, George Dunlap wrote:
> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> >> > Since the intention of that commit was to allow for qdisk backends with no
> >> > explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> >> > impact fix appears to be to make that explicit. This should probably be
> >> > revisited to rationalize the probing.
> >>
> >> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> >> already AFAIK
> >> and I'm pretty sure I'll hit that same stat issue soon for another
> >> remote blktap case.
> >
> > Right, sounds like I should have gone with my first instinct which was:
> >
> > 8<------------------------------------------------------------
> >
> > From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> > From: Ian Campbell <ian.campbell@citrix.com>
> > Date: Fri, 26 Apr 2013 12:41:43 +0100
> > Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
> >
> > The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> > expect a device to exist." changed things to only stat the file when the phy
> > backend was explicitly requested. This broke the case where we are probing and
> > would normally be able to decide on the phy option.
> 
> So at the moment qdisk backends aren't checked at all with this --
> which means that if you give a path to a file that doesn't exist via,
> for example, xl cd-insert, things fail in weird ways:
> 
> 1. In qemu-traditional, the command silently completes; the effect is
> that the disk currently in the drive is ejected
> 
> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
> is ejected from the guest, but the xenstore entries are not updated
> (still contain the old values)
> 
> It seems like we should probably also at least check if disk_format is RAW.

A kit if these issues will come back with disk driver domains I think.

> OTOH, I don't seen an option for disk_format to be ceph; is it just
> listed as "raw" as well?  That doesn't seem right...

That might well be the correct answer, but not for 4.3 I suspect?

Ian.

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On 10/05/13 14:49, Ian Campbell wrote:
> On Fri, 2013-05-10 at 14:46 +0100, George Dunlap wrote:
>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>> revisited to rationalize the probing.
>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>> already AFAIK
>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>> remote blktap case.
>>> Right, sounds like I should have gone with my first instinct which was:
>>>
>>> 8<------------------------------------------------------------
>>>
>>>  From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@citrix.com>
>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>
>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>> expect a device to exist." changed things to only stat the file when the phy
>>> backend was explicitly requested. This broke the case where we are probing and
>>> would normally be able to decide on the phy option.
>> So at the moment qdisk backends aren't checked at all with this --
>> which means that if you give a path to a file that doesn't exist via,
>> for example, xl cd-insert, things fail in weird ways:
>>
>> 1. In qemu-traditional, the command silently completes; the effect is
>> that the disk currently in the drive is ejected
>>
>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>> is ejected from the guest, but the xenstore entries are not updated
>> (still contain the old values)
>>
>> It seems like we should probably also at least check if disk_format is RAW.
> A kit if these issues will come back with disk driver domains I think.
>
>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>> listed as "raw" as well?  That doesn't seem right...
> That might well be the correct answer, but not for 4.3 I suspect?

I can't resolve this into an unambiguous meaning that makes sense. :-)  
Are you saying that ceph *should* be "raw" long-term, but that it 
shoudln't be for 4.3?  Or that right now ceph *is* raw, but we should 
change it for 4.3?  Or that we should change it long-term but leave it 
for 4.3?

  -George

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-05-10 at 14:55 +0100, George Dunlap wrote:
> On 10/05/13 14:49, Ian Campbell wrote:
> > On Fri, 2013-05-10 at 14:46 +0100, George Dunlap wrote:
> >> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> >>>>> Since the intention of that commit was to allow for qdisk backends with no
> >>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> >>>>> impact fix appears to be to make that explicit. This should probably be
> >>>>> revisited to rationalize the probing.
> >>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> >>>> already AFAIK
> >>>> and I'm pretty sure I'll hit that same stat issue soon for another
> >>>> remote blktap case.
> >>> Right, sounds like I should have gone with my first instinct which was:
> >>>
> >>> 8<------------------------------------------------------------
> >>>
> >>>  From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> >>> From: Ian Campbell <ian.campbell@citrix.com>
> >>> Date: Fri, 26 Apr 2013 12:41:43 +0100
> >>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
> >>>
> >>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> >>> expect a device to exist." changed things to only stat the file when the phy
> >>> backend was explicitly requested. This broke the case where we are probing and
> >>> would normally be able to decide on the phy option.
> >> So at the moment qdisk backends aren't checked at all with this --
> >> which means that if you give a path to a file that doesn't exist via,
> >> for example, xl cd-insert, things fail in weird ways:
> >>
> >> 1. In qemu-traditional, the command silently completes; the effect is
> >> that the disk currently in the drive is ejected
> >>
> >> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
> >> is ejected from the guest, but the xenstore entries are not updated
> >> (still contain the old values)
> >>
> >> It seems like we should probably also at least check if disk_format is RAW.
> > A kit if these issues will come back with disk driver domains I think.
> >
> >> OTOH, I don't seen an option for disk_format to be ceph; is it just
> >> listed as "raw" as well?  That doesn't seem right...
> > That might well be the correct answer, but not for 4.3 I suspect?
> 
> I can't resolve this into an unambiguous meaning that makes sense. :-)  
> Are you saying that ceph *should* be "raw" long-term, but that it 
> shoudln't be for 4.3?  Or that right now ceph *is* raw, but we should 
> change it for 4.3?  Or that we should change it long-term but leave it 
> for 4.3?

A ceph type might be the correct long term answer, but I don't think we
want to be making that change now, do we?.

Ian.

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[David Scott]

On 10/05/13 15:09, Ian Campbell wrote:
> On Fri, 2013-05-10 at 14:55 +0100, George Dunlap wrote:
>> On 10/05/13 14:49, Ian Campbell wrote:
>>> On Fri, 2013-05-10 at 14:46 +0100, George Dunlap wrote:
>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>>> revisited to rationalize the probing.
>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>>> already AFAIK
>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>>> remote blktap case.
>>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>>
>>>>> 8<------------------------------------------------------------
>>>>>
>>>>>   From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>>
>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>>> expect a device to exist." changed things to only stat the file when the phy
>>>>> backend was explicitly requested. This broke the case where we are probing and
>>>>> would normally be able to decide on the phy option.
>>>> So at the moment qdisk backends aren't checked at all with this --
>>>> which means that if you give a path to a file that doesn't exist via,
>>>> for example, xl cd-insert, things fail in weird ways:
>>>>
>>>> 1. In qemu-traditional, the command silently completes; the effect is
>>>> that the disk currently in the drive is ejected
>>>>
>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>>>> is ejected from the guest, but the xenstore entries are not updated
>>>> (still contain the old values)
>>>>
>>>> It seems like we should probably also at least check if disk_format is RAW.
>>> A kit if these issues will come back with disk driver domains I think.
>>>
>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>>>> listed as "raw" as well?  That doesn't seem right...
>>> That might well be the correct answer, but not for 4.3 I suspect?
>>
>> I can't resolve this into an unambiguous meaning that makes sense. :-)
>> Are you saying that ceph *should* be "raw" long-term, but that it
>> shoudln't be for 4.3?  Or that right now ceph *is* raw, but we should
>> change it for 4.3?  Or that we should change it long-term but leave it
>> for 4.3?
>
> A ceph type might be the correct long term answer, but I don't think we
> want to be making that change now, do we?.

I think if we add an explicit ceph "format" we'll end up adding 
"formats" for all the other kinds of storage supported by qemu/tapdisk-- 
this seems like avoidable work. The way things stand atm, if someone 
adds a cool new storage type to qemu/tapdisk it should just work with libxl.

Perhaps we could use 'qemu-img' (and some tapdisk equivalent) to verify 
the existence of the disk in place of stat()?

Cheers,
Dave

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[David Scott]

On 10/05/13 14:46, George Dunlap wrote:
> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>> impact fix appears to be to make that explicit. This should probably be
>>>> revisited to rationalize the probing.
>>>
>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>> already AFAIK
>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>> remote blktap case.
>>
>> Right, sounds like I should have gone with my first instinct which was:
>>
>> 8<------------------------------------------------------------
>>
>>  From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>> From: Ian Campbell <ian.campbell@citrix.com>
>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>
>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>> expect a device to exist." changed things to only stat the file when the phy
>> backend was explicitly requested. This broke the case where we are probing and
>> would normally be able to decide on the phy option.
>
> So at the moment qdisk backends aren't checked at all with this --
> which means that if you give a path to a file that doesn't exist via,
> for example, xl cd-insert, things fail in weird ways:
>
> 1. In qemu-traditional, the command silently completes; the effect is
> that the disk currently in the drive is ejected
>
> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
> is ejected from the guest, but the xenstore entries are not updated
> (still contain the old values)
>
> It seems like we should probably also at least check if disk_format is RAW.
>
> OTOH, I don't seen an option for disk_format to be ceph; is it just
> listed as "raw" as well?  That doesn't seem right...

AFAICT a ceph disk is in the "raw" format but it uses a custom network 
protocol to actually read and write the blocks. I imagine on the ceph 
servers the disk is stored in a cleverer format, but all qemu/tapdisk 
see are plain blocks with no fancy encoding, no .vhd or .qcow.

Cheers,
Dave

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On 10/05/13 15:09, Dave Scott wrote:
> On 10/05/13 14:46, George Dunlap wrote:
>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>> revisited to rationalize the probing.
>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>> already AFAIK
>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>> remote blktap case.
>>> Right, sounds like I should have gone with my first instinct which was:
>>>
>>> 8<------------------------------------------------------------
>>>
>>>   From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>> From: Ian Campbell <ian.campbell@citrix.com>
>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>
>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>> expect a device to exist." changed things to only stat the file when the phy
>>> backend was explicitly requested. This broke the case where we are probing and
>>> would normally be able to decide on the phy option.
>> So at the moment qdisk backends aren't checked at all with this --
>> which means that if you give a path to a file that doesn't exist via,
>> for example, xl cd-insert, things fail in weird ways:
>>
>> 1. In qemu-traditional, the command silently completes; the effect is
>> that the disk currently in the drive is ejected
>>
>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>> is ejected from the guest, but the xenstore entries are not updated
>> (still contain the old values)
>>
>> It seems like we should probably also at least check if disk_format is RAW.
>>
>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>> listed as "raw" as well?  That doesn't seem right...
> AFAICT a ceph disk is in the "raw" format but it uses a custom network
> protocol to actually read and write the blocks. I imagine on the ceph
> servers the disk is stored in a cleverer format, but all qemu/tapdisk
> see are plain blocks with no fancy encoding, no .vhd or .qcow.

Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it 
*is* just a raw file for these purposes; and we should still be able to 
call stat() on it.

I guess I'm not yet clear why qdisk files are excluded from the stat().  
Under what circumstances would disk->pdev not be a valid file if 
disk->backend_domid == LIBXL_TOOLSTACK_ID?

  -George

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[David Scott]

On 10/05/13 15:19, George Dunlap wrote:
> On 10/05/13 15:09, Dave Scott wrote:
>> On 10/05/13 14:46, George Dunlap wrote:
>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>> revisited to rationalize the probing.
>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>> already AFAIK
>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>> remote blktap case.
>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>
>>>> 8<------------------------------------------------------------
>>>>
>>>>    From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>
>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>> expect a device to exist." changed things to only stat the file when the phy
>>>> backend was explicitly requested. This broke the case where we are probing and
>>>> would normally be able to decide on the phy option.
>>> So at the moment qdisk backends aren't checked at all with this --
>>> which means that if you give a path to a file that doesn't exist via,
>>> for example, xl cd-insert, things fail in weird ways:
>>>
>>> 1. In qemu-traditional, the command silently completes; the effect is
>>> that the disk currently in the drive is ejected
>>>
>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>>> is ejected from the guest, but the xenstore entries are not updated
>>> (still contain the old values)
>>>
>>> It seems like we should probably also at least check if disk_format is RAW.
>>>
>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>>> listed as "raw" as well?  That doesn't seem right...
>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
>> protocol to actually read and write the blocks. I imagine on the ceph
>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
>
> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
> *is* just a raw file for these purposes; and we should still be able to
> call stat() on it.

There is no file :) All we have is a "URL"-like thing which looks like:

   rbd:rbd/foo.img

which qemu/tapdisk accesses via a C library called librados. The C 
library takes the "URL" and makes a network connection to a storage 
server and reads and writes blocks over TCP/IP. No filesystems are 
involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.

I think this kind of thing will become more common in future because you 
get to stay in userspace (no kernel driver required) and you don't have 
to go through FUSE and suffer a performance hit there.

[ Disclaimer: I haven't checked to see whether the disk is actually 
represented as a file on the ceph server side. However this doesn't 
matter since this is on a different host altogether and so you still 
can't stat() it :) ]

Cheers,
Dave

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On 10/05/13 15:31, Dave Scott wrote:
> On 10/05/13 15:19, George Dunlap wrote:
>> On 10/05/13 15:09, Dave Scott wrote:
>>> On 10/05/13 14:46, George Dunlap wrote:
>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>>> revisited to rationalize the probing.
>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>>> already AFAIK
>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>>> remote blktap case.
>>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>>
>>>>> 8<------------------------------------------------------------
>>>>>
>>>>>     From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>>
>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>>> expect a device to exist." changed things to only stat the file when the phy
>>>>> backend was explicitly requested. This broke the case where we are probing and
>>>>> would normally be able to decide on the phy option.
>>>> So at the moment qdisk backends aren't checked at all with this --
>>>> which means that if you give a path to a file that doesn't exist via,
>>>> for example, xl cd-insert, things fail in weird ways:
>>>>
>>>> 1. In qemu-traditional, the command silently completes; the effect is
>>>> that the disk currently in the drive is ejected
>>>>
>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>>>> is ejected from the guest, but the xenstore entries are not updated
>>>> (still contain the old values)
>>>>
>>>> It seems like we should probably also at least check if disk_format is RAW.
>>>>
>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>>>> listed as "raw" as well?  That doesn't seem right...
>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
>>> protocol to actually read and write the blocks. I imagine on the ceph
>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
>>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
>> *is* just a raw file for these purposes; and we should still be able to
>> call stat() on it.
> There is no file :) All we have is a "URL"-like thing which looks like:
>
>     rbd:rbd/foo.img
>
> which qemu/tapdisk accesses via a C library called librados. The C
> library takes the "URL" and makes a network connection to a storage
> server and reads and writes blocks over TCP/IP. No filesystems are
> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.

Right -- so I think this should be a separate type to RAW.  I agree that 
making a new format for each new protocol isn't very scalable -- but 
there should be a "URL" or "VIRTUAL" format to indicate that there is no 
real file.

Right now RAW doesn't seem to be talking about the format, but about the 
format string; i.e., "I pass this straight into qemu and it figures out 
what it means."  That's not right.

For 4.3 -- I guess the simplest thing to fix the actual bug (xl 
cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before 
calling libxl_cdrom_insert().

  -George

  -George

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[David Scott]

On 10/05/13 15:42, George Dunlap wrote:
> On 10/05/13 15:31, Dave Scott wrote:
>> On 10/05/13 15:19, George Dunlap wrote:
>>> On 10/05/13 15:09, Dave Scott wrote:
>>>> On 10/05/13 14:46, George Dunlap wrote:
>>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>>>> revisited to rationalize the probing.
>>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>>>> already AFAIK
>>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>>>> remote blktap case.
>>>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>>>
>>>>>> 8<------------------------------------------------------------
>>>>>>
>>>>>>      From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>>>
>>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>>>> expect a device to exist." changed things to only stat the file when the phy
>>>>>> backend was explicitly requested. This broke the case where we are probing and
>>>>>> would normally be able to decide on the phy option.
>>>>> So at the moment qdisk backends aren't checked at all with this --
>>>>> which means that if you give a path to a file that doesn't exist via,
>>>>> for example, xl cd-insert, things fail in weird ways:
>>>>>
>>>>> 1. In qemu-traditional, the command silently completes; the effect is
>>>>> that the disk currently in the drive is ejected
>>>>>
>>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>>>>> is ejected from the guest, but the xenstore entries are not updated
>>>>> (still contain the old values)
>>>>>
>>>>> It seems like we should probably also at least check if disk_format is RAW.
>>>>>
>>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>>>>> listed as "raw" as well?  That doesn't seem right...
>>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
>>>> protocol to actually read and write the blocks. I imagine on the ceph
>>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
>>>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
>>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
>>> *is* just a raw file for these purposes; and we should still be able to
>>> call stat() on it.
>> There is no file :) All we have is a "URL"-like thing which looks like:
>>
>>      rbd:rbd/foo.img
>>
>> which qemu/tapdisk accesses via a C library called librados. The C
>> library takes the "URL" and makes a network connection to a storage
>> server and reads and writes blocks over TCP/IP. No filesystems are
>> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.
>
> Right -- so I think this should be a separate type to RAW.  I agree that
> making a new format for each new protocol isn't very scalable -- but
> there should be a "URL" or "VIRTUAL" format to indicate that there is no
> real file.

Something like that would be fine. I agree "format=raw" is a bit odd, 
when there's no real "format" involved, only a network protocol.

> Right now RAW doesn't seem to be talking about the format, but about the
> format string; i.e., "I pass this straight into qemu and it figures out
> what it means."  That's not right.
>
> For 4.3 -- I guess the simplest thing to fix the actual bug (xl
> cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before
> calling libxl_cdrom_insert().

That works for me. Do non-CDROM cases fail in a more reasonable way?

Dave

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[George Dunlap]

On 10/05/13 16:03, Dave Scott wrote:
> On 10/05/13 15:42, George Dunlap wrote:
>> On 10/05/13 15:31, Dave Scott wrote:
>>> On 10/05/13 15:19, George Dunlap wrote:
>>>> On 10/05/13 15:09, Dave Scott wrote:
>>>>> On 10/05/13 14:46, George Dunlap wrote:
>>>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
>>>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
>>>>>>>>> Since the intention of that commit was to allow for qdisk backends with no
>>>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
>>>>>>>>> impact fix appears to be to make that explicit. This should probably be
>>>>>>>>> revisited to rationalize the probing.
>>>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
>>>>>>>> already AFAIK
>>>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
>>>>>>>> remote blktap case.
>>>>>>> Right, sounds like I should have gone with my first instinct which was:
>>>>>>>
>>>>>>> 8<------------------------------------------------------------
>>>>>>>
>>>>>>>       From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
>>>>>>> From: Ian Campbell <ian.campbell@citrix.com>
>>>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
>>>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
>>>>>>>
>>>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
>>>>>>> expect a device to exist." changed things to only stat the file when the phy
>>>>>>> backend was explicitly requested. This broke the case where we are probing and
>>>>>>> would normally be able to decide on the phy option.
>>>>>> So at the moment qdisk backends aren't checked at all with this --
>>>>>> which means that if you give a path to a file that doesn't exist via,
>>>>>> for example, xl cd-insert, things fail in weird ways:
>>>>>>
>>>>>> 1. In qemu-traditional, the command silently completes; the effect is
>>>>>> that the disk currently in the drive is ejected
>>>>>>
>>>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
>>>>>> is ejected from the guest, but the xenstore entries are not updated
>>>>>> (still contain the old values)
>>>>>>
>>>>>> It seems like we should probably also at least check if disk_format is RAW.
>>>>>>
>>>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
>>>>>> listed as "raw" as well?  That doesn't seem right...
>>>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
>>>>> protocol to actually read and write the blocks. I imagine on the ceph
>>>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
>>>>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
>>>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
>>>> *is* just a raw file for these purposes; and we should still be able to
>>>> call stat() on it.
>>> There is no file :) All we have is a "URL"-like thing which looks like:
>>>
>>>       rbd:rbd/foo.img
>>>
>>> which qemu/tapdisk accesses via a C library called librados. The C
>>> library takes the "URL" and makes a network connection to a storage
>>> server and reads and writes blocks over TCP/IP. No filesystems are
>>> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.
>> Right -- so I think this should be a separate type to RAW.  I agree that
>> making a new format for each new protocol isn't very scalable -- but
>> there should be a "URL" or "VIRTUAL" format to indicate that there is no
>> real file.
> Something like that would be fine. I agree "format=raw" is a bit odd,
> when there's no real "format" involved, only a network protocol.
>
>> Right now RAW doesn't seem to be talking about the format, but about the
>> format string; i.e., "I pass this straight into qemu and it figures out
>> what it means."  That's not right.
>>
>> For 4.3 -- I guess the simplest thing to fix the actual bug (xl
>> cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before
>> calling libxl_cdrom_insert().
> That works for me. Do non-CDROM cases fail in a more reasonable way?

Well presumably at some point qemu will return an error if the spec 
(either a plain file or a network thing) doesn't work properly. Whether 
that will cause a proper failure for "xl block-attach" or 
libxl_device_disk_attach() I'm not sure.

  -George

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-05-10 at 16:03 +0100, Dave Scott wrote:
> On 10/05/13 15:42, George Dunlap wrote:
> > On 10/05/13 15:31, Dave Scott wrote:
> >> On 10/05/13 15:19, George Dunlap wrote:
> >>> On 10/05/13 15:09, Dave Scott wrote:
> >>>> On 10/05/13 14:46, George Dunlap wrote:
> >>>>> On Fri, Apr 26, 2013 at 3:29 PM, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> >>>>>> On Fri, 2013-04-26 at 15:17 +0100, Sylvain Munaut wrote:
> >>>>>>>> Since the intention of that commit was to allow for qdisk backends with no
> >>>>>>>> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> >>>>>>>> impact fix appears to be to make that explicit. This should probably be
> >>>>>>>> revisited to rationalize the probing.
> >>>>>>> What about the remote disk case of blktap ?  blktap2.5 supports NBD
> >>>>>>> already AFAIK
> >>>>>>> and I'm pretty sure I'll hit that same stat issue soon for another
> >>>>>>> remote blktap case.
> >>>>>> Right, sounds like I should have gone with my first instinct which was:
> >>>>>>
> >>>>>> 8<------------------------------------------------------------
> >>>>>>
> >>>>>>      From 884beff4a897d785f61705dcfa2f048536982d7c Mon Sep 17 00:00:00 2001
> >>>>>> From: Ian Campbell <ian.campbell@citrix.com>
> >>>>>> Date: Fri, 26 Apr 2013 12:41:43 +0100
> >>>>>> Subject: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)
> >>>>>>
> >>>>>> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> >>>>>> expect a device to exist." changed things to only stat the file when the phy
> >>>>>> backend was explicitly requested. This broke the case where we are probing and
> >>>>>> would normally be able to decide on the phy option.
> >>>>> So at the moment qdisk backends aren't checked at all with this --
> >>>>> which means that if you give a path to a file that doesn't exist via,
> >>>>> for example, xl cd-insert, things fail in weird ways:
> >>>>>
> >>>>> 1. In qemu-traditional, the command silently completes; the effect is
> >>>>> that the disk currently in the drive is ejected
> >>>>>
> >>>>> 2. in qemu-upstream, qmp returns an error which is reported.  The disk
> >>>>> is ejected from the guest, but the xenstore entries are not updated
> >>>>> (still contain the old values)
> >>>>>
> >>>>> It seems like we should probably also at least check if disk_format is RAW.
> >>>>>
> >>>>> OTOH, I don't seen an option for disk_format to be ceph; is it just
> >>>>> listed as "raw" as well?  That doesn't seem right...
> >>>> AFAICT a ceph disk is in the "raw" format but it uses a custom network
> >>>> protocol to actually read and write the blocks. I imagine on the ceph
> >>>> servers the disk is stored in a cleverer format, but all qemu/tapdisk
> >>>> see are plain blocks with no fancy encoding, no .vhd or .qcow.
> >>> Oh, hang on -- does qdisk / tapdisk then just open a plain file? Then it
> >>> *is* just a raw file for these purposes; and we should still be able to
> >>> call stat() on it.
> >> There is no file :) All we have is a "URL"-like thing which looks like:
> >>
> >>      rbd:rbd/foo.img
> >>
> >> which qemu/tapdisk accesses via a C library called librados. The C
> >> library takes the "URL" and makes a network connection to a storage
> >> server and reads and writes blocks over TCP/IP. No filesystems are
> >> involved, "rbd:rbd/foo.img" doesn't exist in the filesystem.
> >
> > Right -- so I think this should be a separate type to RAW.  I agree that
> > making a new format for each new protocol isn't very scalable -- but
> > there should be a "URL" or "VIRTUAL" format to indicate that there is no
> > real file.
> 
> Something like that would be fine. I agree "format=raw" is a bit odd, 
> when there's no real "format" involved, only a network protocol.

Yes, something along those lines seems like a good idea.

It's not clear how libxl would know which backend to choose, but I
suppose we either mandate backendtype= or pick qemu as the obvious
default.

> > Right now RAW doesn't seem to be talking about the format, but about the
> > format string; i.e., "I pass this straight into qemu and it figures out
> > what it means."  That's not right.
> >
> > For 4.3 -- I guess the simplest thing to fix the actual bug (xl
> > cd-insert giving weird results) is to put a stat in xl_cmdimpl.c before
> > calling libxl_cdrom_insert().
> 
> That works for me.

Yeah, sounds sensible/pragmatic enough.

Ian.

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Roger Pau Monné]

On 26/04/13 16:12, Ian Campbell wrote:
> The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> expect a device to exist." changed things to only stat the file when the phy
> backend was explicitly requested. This broke the case where we are probing and
> would normally be able to decide on the phy option.
> 
> Since the intention of that commit was to allow for qdisk backends with no
> explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> impact fix appears to be to make that explicit. This should probably be
> revisited to rationalize the probing.
> 
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

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

But I think we should consider this as a band-aid. As said by Sylvain,
tap could also support backends that cannot be stated, and the backend
checks we have in libxl are rather fragile and hard to understand IMHO.

> Cc: dave.scott@citrix.com
> Cc: george.dunlap@citrix.com
> Cc: roger.pau@citrix.com
> Cc: ian.jackson@citrix.com
> ---
>  tools/libxl/libxl_device.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> index eb60fd5..b01d109 100644
> --- a/tools/libxl/libxl_device.c
> +++ b/tools/libxl/libxl_device.c
> @@ -236,7 +236,7 @@ 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->backend == LIBXL_DISK_BACKEND_PHY &&
> +    } else if (disk->backend != LIBXL_DISK_BACKEND_QDISK &&
>                 disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
>                 !disk->script) {
>          if (stat(disk->pdev_path, &a.stab)) {
> 

Re: [PATCH] libxl: stat the path for all non-qdisk backends (including unknown)

[Ian Campbell]

On Fri, 2013-04-26 at 15:29 +0100, Roger Pau Monne wrote:
> On 26/04/13 16:12, Ian Campbell wrote:
> > The commit a8a1f236a296 "libxl: Only call stat() when adding a disk if we
> > expect a device to exist." changed things to only stat the file when the phy
> > backend was explicitly requested. This broke the case where we are probing and
> > would normally be able to decide on the phy option.
> > 
> > Since the intention of that commit was to allow for qdisk backends with no
> > explicit file in dom0 (i.e. network remote backend such as ceph) the lowest
> > impact fix appears to be to make that explicit. This should probably be
> > revisited to rationalize the probing.
> > 
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> 
> Acked-by: Roger Pau Monné <roger.pau@citrix.com>
> 
> But I think we should consider this as a band-aid.

Right, this was just the smallest fix to get us back to a working state
and it should certainly be revisited. Dave had a convincing looking
proposal in the "Only call stat() when adding a disk if we expect a
device to exist." thread, once that becomes more concrete we need to
make a call on 4.3 vs 4.4 material.

>  As said by Sylvain,
> tap could also support backends that cannot be stated,

Yes, I sent out V2 to support that.

> and the backend checks we have in libxl are rather fragile and hard to understand IMHO.

Ack.

> 
> > Cc: dave.scott@citrix.com
> > Cc: george.dunlap@citrix.com
> > Cc: roger.pau@citrix.com
> > Cc: ian.jackson@citrix.com
> > ---
> >  tools/libxl/libxl_device.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
> > index eb60fd5..b01d109 100644
> > --- a/tools/libxl/libxl_device.c
> > +++ b/tools/libxl/libxl_device.c
> > @@ -236,7 +236,7 @@ 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->backend == LIBXL_DISK_BACKEND_PHY &&
> > +    } else if (disk->backend != LIBXL_DISK_BACKEND_QDISK &&
> >                 disk->backend_domid == LIBXL_TOOLSTACK_DOMID &&
> >                 !disk->script) {
> >          if (stat(disk->pdev_path, &a.stab)) {
> > 
> 



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