[PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Wei Liu]

Modify libxl and hotplug script to allow raw format file to use phy
backend.

The block script now tests the path and determine the actual type of
file (block device or regular file) then use the actual type to
determine which branch to run.

With these changes, plus the current ordering of backend preference (phy
> qdisk > tap), we will use phy backend for raw format file by default.

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Roger Pau Monné <roger.pau@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
---
 tools/hotplug/Linux/block  |   16 +++++++++-------
 tools/libxl/libxl.c        |    6 +++---
 tools/libxl/libxl_device.c |    2 ++
 tools/libxl/libxl_linux.c  |    6 +++---
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
index da26e22..8d2ee9d 100644
--- a/tools/hotplug/Linux/block
+++ b/tools/hotplug/Linux/block
@@ -206,6 +206,13 @@ and so cannot be mounted ${m2}${when}."
 
 
 t=$(xenstore_read_default "$XENBUS_PATH/type" 'MISSING')
+p=$(xenstore_read "$XENBUS_PATH/params")
+mode=$(xenstore_read "$XENBUS_PATH/mode")
+if [ -b "$p" ]; then
+    truetype="phy"
+elif [ -f "$p" ]; then
+    truetype="file"
+fi
 
 case "$command" in
   add)
@@ -217,16 +224,11 @@ case "$command" in
       exit 0
     fi
 
-    if [ -n "$t" ]
-    then
-      p=$(xenstore_read "$XENBUS_PATH/params")
-      mode=$(xenstore_read "$XENBUS_PATH/mode")
-    fi
     FRONTEND_ID=$(xenstore_read "$XENBUS_PATH/frontend-id")
     FRONTEND_UUID=$(xenstore_read_default \
             "/local/domain/$FRONTEND_ID/vm" 'unknown')
 
-    case $t in 
+    case $truetype in
       phy)
         dev=$(expand_dev $p)
 
@@ -319,7 +321,7 @@ mount it read-write in a guest domain."
     ;;
 
   remove)
-    case $t in 
+    case $truetype in
       phy)
 	exit 0
 	;;
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index de23fec..7ce1bc4 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -2392,9 +2392,9 @@ static void device_disk_add(libxl__egc *egc, uint32_t domid,
                 if (!disk->script &&
                     disk->backend_domid == LIBXL_TOOLSTACK_DOMID) {
                     int major, minor;
-                    libxl__device_physdisk_major_minor(dev, &major, &minor);
-                    flexarray_append_pair(back, "physical-device",
-                            libxl__sprintf(gc, "%x:%x", major, minor));
+                    if (!libxl__device_physdisk_major_minor(dev, &major, &minor))
+                        flexarray_append_pair(back, "physical-device",
+                                              libxl__sprintf(gc, "%x:%x", major, minor));
                 }
 
                 assert(device->backend_kind == LIBXL__DEVICE_KIND_VBD);
diff --git a/tools/libxl/libxl_device.c b/tools/libxl/libxl_device.c
index 4b51ded..0f50d04 100644
--- a/tools/libxl/libxl_device.c
+++ b/tools/libxl/libxl_device.c
@@ -332,6 +332,8 @@ int libxl__device_physdisk_major_minor(const char *physpath, int *major, int *mi
     struct stat buf;
     if (stat(physpath, &buf) < 0)
         return -1;
+    if (!S_ISBLK(buf.st_mode))
+        return -1;
     *major = major(buf.st_rdev);
     *minor = minor(buf.st_rdev);
     return 0;
diff --git a/tools/libxl/libxl_linux.c b/tools/libxl/libxl_linux.c
index ea5d8c1..b51930c 100644
--- a/tools/libxl/libxl_linux.c
+++ b/tools/libxl/libxl_linux.c
@@ -19,11 +19,11 @@
  
 int libxl__try_phy_backend(mode_t st_mode)
 {
-    if (!S_ISBLK(st_mode)) {
-        return 0;
+    if (S_ISBLK(st_mode) || S_ISREG(st_mode)) {
+        return 1;
     }
 
-    return 1;
+    return 0;
 }
 
 #define EXT_SHIFT 28
-- 
1.7.10.4


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

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Roger Pau Monné]

El 26/11/14 a les 17.55, Wei Liu ha escrit:
> Modify libxl and hotplug script to allow raw format file to use phy
> backend.
> 
> The block script now tests the path and determine the actual type of
> file (block device or regular file) then use the actual type to
> determine which branch to run.
> 
> With these changes, plus the current ordering of backend preference (phy
>> qdisk > tap), we will use phy backend for raw format file by default.

Maybe it's a stupid question, but I remember that last time this changed
was attempted it caused problems with local migration. Has this been fixed?

I'm asking because there's no comment of this in the commit message, and
the code doesn't seem to differ from the first try in 11a63a.

Roger.

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Wei Liu]

On Wed, Nov 26, 2014 at 06:32:21PM +0100, Roger Pau Monné wrote:
> El 26/11/14 a les 17.55, Wei Liu ha escrit:
> > Modify libxl and hotplug script to allow raw format file to use phy
> > backend.
> > 
> > The block script now tests the path and determine the actual type of
> > file (block device or regular file) then use the actual type to
> > determine which branch to run.
> > 
> > With these changes, plus the current ordering of backend preference (phy
> >> qdisk > tap), we will use phy backend for raw format file by default.
> 
> Maybe it's a stupid question, but I remember that last time this changed
> was attempted it caused problems with local migration. Has this been fixed?
> 
> I'm asking because there's no comment of this in the commit message, and
> the code doesn't seem to differ from the first try in 11a63a.
> 

Yes, it's fixed.

Actually I think the regression last time was not local migration but
another problem.

This patch is different from the last one, libxl won't write
"physical-device" if the file is regular file. I think last time the
hotplug script was not run correctly due to that problem.

This patch is tested with following setup:

1. Raw file and PV
2. Raw file and HVM
3. Block device and PV
4. Block device and HVM

Creation / destruction / local migration all worked.

Wei.

> Roger.

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Roger Pau Monné]

El 26/11/14 a les 18.44, Wei Liu ha escrit:
> On Wed, Nov 26, 2014 at 06:32:21PM +0100, Roger Pau Monné wrote:
>> El 26/11/14 a les 17.55, Wei Liu ha escrit:
>>> Modify libxl and hotplug script to allow raw format file to use phy
>>> backend.
>>>
>>> The block script now tests the path and determine the actual type of
>>> file (block device or regular file) then use the actual type to
>>> determine which branch to run.
>>>
>>> With these changes, plus the current ordering of backend preference (phy
>>>> qdisk > tap), we will use phy backend for raw format file by default.
>>
>> Maybe it's a stupid question, but I remember that last time this changed
>> was attempted it caused problems with local migration. Has this been fixed?
>>
>> I'm asking because there's no comment of this in the commit message, and
>> the code doesn't seem to differ from the first try in 11a63a.
>>
> 
> Yes, it's fixed.
> 
> Actually I think the regression last time was not local migration but
> another problem.
> 
> This patch is different from the last one, libxl won't write
> "physical-device" if the file is regular file. I think last time the
> hotplug script was not run correctly due to that problem.

IMHO it would be good to add that to the commit message.

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

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Ian Campbell]

On Wed, 2014-11-26 at 16:55 +0000, Wei Liu wrote:
> Modify libxl and hotplug script to allow raw format file to use phy
> backend.
> 
> The block script now tests the path and determine the actual type of
> file (block device or regular file) then use the actual type to
> determine which branch to run.
> 
> With these changes, plus the current ordering of backend preference (phy
> > qdisk > tap), we will use phy backend for raw format file by default.

http://lists.xen.org/archives/html/xen-devel/2012-04/msg00077.html
includes (in the quotes, Stefano's reply is about something else but has
conveniently trimmed most of the other uninteresting stuff):
        use /dev/loop+blkback. This requires loop driver AIO and
        O_DIRECT patches which are not (AFAIK) yet upstream.

and I have it in my mind that using /dev/loop+blkback is somehow unsafe,
for reasons relating to crash consistency and the proper implementation
(in /dev/loop, blkback is good I think) of barriers and such, e.g.
relating to whether data is really on the platter or not when we've to
the frontend that it is (which is critical for proper operation of
journalling file systems).

It's entirely possible that I'm either plain wrong or a decade out of
date on this though. CC-ing Konrad in case he has any insights as
blkback maintainer (I think, MAINTAINERS doesn't have a specific entry)

> 
> Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Cc: Roger Pau Monné <roger.pau@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
>  tools/hotplug/Linux/block  |   16 +++++++++-------
>  tools/libxl/libxl.c        |    6 +++---
>  tools/libxl/libxl_device.c |    2 ++
>  tools/libxl/libxl_linux.c  |    6 +++---
>  4 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
> index da26e22..8d2ee9d 100644
> --- a/tools/hotplug/Linux/block
> +++ b/tools/hotplug/Linux/block
> @@ -206,6 +206,13 @@ and so cannot be mounted ${m2}${when}."
>  
> 
>  t=$(xenstore_read_default "$XENBUS_PATH/type" 'MISSING')
> +p=$(xenstore_read "$XENBUS_PATH/params")
> +mode=$(xenstore_read "$XENBUS_PATH/mode")
> +if [ -b "$p" ]; then
> +    truetype="phy"
> +elif [ -f "$p" ]; then
> +    truetype="file"
> +fi

IIRC xend used to write file or phy to the type node as appropriate. I'm
unsure if that was in some way better than this approach, and perhaps
libxl doesn't even have the right information to hand.

Anyway, I just mention it for completeness, ignore it if you like.

Ian.


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

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Wei Liu]

On Thu, Jan 08, 2015 at 02:07:42PM +0000, Ian Campbell wrote:
> On Wed, 2014-11-26 at 16:55 +0000, Wei Liu wrote:
> > Modify libxl and hotplug script to allow raw format file to use phy
> > backend.
> > 
> > The block script now tests the path and determine the actual type of
> > file (block device or regular file) then use the actual type to
> > determine which branch to run.
> > 
> > With these changes, plus the current ordering of backend preference (phy
> > > qdisk > tap), we will use phy backend for raw format file by default.
> 
> http://lists.xen.org/archives/html/xen-devel/2012-04/msg00077.html
> includes (in the quotes, Stefano's reply is about something else but has
> conveniently trimmed most of the other uninteresting stuff):
>         use /dev/loop+blkback. This requires loop driver AIO and
>         O_DIRECT patches which are not (AFAIK) yet upstream.
> 
> and I have it in my mind that using /dev/loop+blkback is somehow unsafe,
> for reasons relating to crash consistency and the proper implementation
> (in /dev/loop, blkback is good I think) of barriers and such, e.g.
> relating to whether data is really on the platter or not when we've to
> the frontend that it is (which is critical for proper operation of
> journalling file systems).
> 
> It's entirely possible that I'm either plain wrong or a decade out of
> date on this though. CC-ing Konrad in case he has any insights as
> blkback maintainer (I think, MAINTAINERS doesn't have a specific entry)
> 

Too bad, as far as I can tell AIO and O_DIRECT are still missing in loop
device. I guess we will have to wait until those two things are
upstreamed.

Wei.

> > 
> > Signed-off-by: Wei Liu <wei.liu2@citrix.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Cc: Roger Pau Monné <roger.pau@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > ---
> >  tools/hotplug/Linux/block  |   16 +++++++++-------
> >  tools/libxl/libxl.c        |    6 +++---
> >  tools/libxl/libxl_device.c |    2 ++
> >  tools/libxl/libxl_linux.c  |    6 +++---
> >  4 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > diff --git a/tools/hotplug/Linux/block b/tools/hotplug/Linux/block
> > index da26e22..8d2ee9d 100644
> > --- a/tools/hotplug/Linux/block
> > +++ b/tools/hotplug/Linux/block
> > @@ -206,6 +206,13 @@ and so cannot be mounted ${m2}${when}."
> >  
> > 
> >  t=$(xenstore_read_default "$XENBUS_PATH/type" 'MISSING')
> > +p=$(xenstore_read "$XENBUS_PATH/params")
> > +mode=$(xenstore_read "$XENBUS_PATH/mode")
> > +if [ -b "$p" ]; then
> > +    truetype="phy"
> > +elif [ -f "$p" ]; then
> > +    truetype="file"
> > +fi
> 
> IIRC xend used to write file or phy to the type node as appropriate. I'm
> unsure if that was in some way better than this approach, and perhaps
> libxl doesn't even have the right information to hand.
> 
> Anyway, I just mention it for completeness, ignore it if you like.
> 
> Ian.

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Ian Campbell]

On Thu, 2015-01-08 at 16:07 +0000, Wei Liu wrote:
> On Thu, Jan 08, 2015 at 02:07:42PM +0000, Ian Campbell wrote:
> > On Wed, 2014-11-26 at 16:55 +0000, Wei Liu wrote:
> > > Modify libxl and hotplug script to allow raw format file to use phy
> > > backend.
> > > 
> > > The block script now tests the path and determine the actual type of
> > > file (block device or regular file) then use the actual type to
> > > determine which branch to run.
> > > 
> > > With these changes, plus the current ordering of backend preference (phy
> > > > qdisk > tap), we will use phy backend for raw format file by default.
> > 
> > http://lists.xen.org/archives/html/xen-devel/2012-04/msg00077.html
> > includes (in the quotes, Stefano's reply is about something else but has
> > conveniently trimmed most of the other uninteresting stuff):
> >         use /dev/loop+blkback. This requires loop driver AIO and
> >         O_DIRECT patches which are not (AFAIK) yet upstream.
> > 
> > and I have it in my mind that using /dev/loop+blkback is somehow unsafe,
> > for reasons relating to crash consistency and the proper implementation
> > (in /dev/loop, blkback is good I think) of barriers and such, e.g.
> > relating to whether data is really on the platter or not when we've to
> > the frontend that it is (which is critical for proper operation of
> > journalling file systems).
> > 
> > It's entirely possible that I'm either plain wrong or a decade out of
> > date on this though. CC-ing Konrad in case he has any insights as
> > blkback maintainer (I think, MAINTAINERS doesn't have a specific entry)
> > 
> 
> Too bad, as far as I can tell AIO and O_DIRECT are still missing in loop
> device. I guess we will have to wait until those two things are
> upstreamed.
> 

I'm kinda hoping someone will tell me I'm wrong...

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Stefano Stabellini]

On Thu, 8 Jan 2015, Ian Campbell wrote:
> On Thu, 2015-01-08 at 16:07 +0000, Wei Liu wrote:
> > On Thu, Jan 08, 2015 at 02:07:42PM +0000, Ian Campbell wrote:
> > > On Wed, 2014-11-26 at 16:55 +0000, Wei Liu wrote:
> > > > Modify libxl and hotplug script to allow raw format file to use phy
> > > > backend.
> > > > 
> > > > The block script now tests the path and determine the actual type of
> > > > file (block device or regular file) then use the actual type to
> > > > determine which branch to run.
> > > > 
> > > > With these changes, plus the current ordering of backend preference (phy
> > > > > qdisk > tap), we will use phy backend for raw format file by default.
> > > 
> > > http://lists.xen.org/archives/html/xen-devel/2012-04/msg00077.html
> > > includes (in the quotes, Stefano's reply is about something else but has
> > > conveniently trimmed most of the other uninteresting stuff):
> > >         use /dev/loop+blkback. This requires loop driver AIO and
> > >         O_DIRECT patches which are not (AFAIK) yet upstream.
> > > 
> > > and I have it in my mind that using /dev/loop+blkback is somehow unsafe,
> > > for reasons relating to crash consistency and the proper implementation
> > > (in /dev/loop, blkback is good I think) of barriers and such, e.g.
> > > relating to whether data is really on the platter or not when we've to
> > > the frontend that it is (which is critical for proper operation of
> > > journalling file systems).
> > > 
> > > It's entirely possible that I'm either plain wrong or a decade out of
> > > date on this though. CC-ing Konrad in case he has any insights as
> > > blkback maintainer (I think, MAINTAINERS doesn't have a specific entry)
> > > 
> > 
> > Too bad, as far as I can tell AIO and O_DIRECT are still missing in loop
> > device. I guess we will have to wait until those two things are
> > upstreamed.
> > 
> 
> I'm kinda hoping someone will tell me I'm wrong...

Even without O_DIRECT, using /dev/loop+blkback is safe if the barriers
in the block protocol are implemented correctly. Last time I look, they
are, so I think we should be able to switch to it.

Re: [PATCH for-4.6] libxl, hotplug/Linux: default to phy backend for raw format file

[Ian Campbell]

On Thu, 2015-01-08 at 16:47 +0000, Stefano Stabellini wrote:
> On Thu, 8 Jan 2015, Ian Campbell wrote:
> > On Thu, 2015-01-08 at 16:07 +0000, Wei Liu wrote:
> > > On Thu, Jan 08, 2015 at 02:07:42PM +0000, Ian Campbell wrote:
> > > > On Wed, 2014-11-26 at 16:55 +0000, Wei Liu wrote:
> > > > > Modify libxl and hotplug script to allow raw format file to use phy
> > > > > backend.
> > > > > 
> > > > > The block script now tests the path and determine the actual type of
> > > > > file (block device or regular file) then use the actual type to
> > > > > determine which branch to run.
> > > > > 
> > > > > With these changes, plus the current ordering of backend preference (phy
> > > > > > qdisk > tap), we will use phy backend for raw format file by default.
> > > > 
> > > > http://lists.xen.org/archives/html/xen-devel/2012-04/msg00077.html
> > > > includes (in the quotes, Stefano's reply is about something else but has
> > > > conveniently trimmed most of the other uninteresting stuff):
> > > >         use /dev/loop+blkback. This requires loop driver AIO and
> > > >         O_DIRECT patches which are not (AFAIK) yet upstream.
> > > > 
> > > > and I have it in my mind that using /dev/loop+blkback is somehow unsafe,
> > > > for reasons relating to crash consistency and the proper implementation
> > > > (in /dev/loop, blkback is good I think) of barriers and such, e.g.
> > > > relating to whether data is really on the platter or not when we've to
> > > > the frontend that it is (which is critical for proper operation of
> > > > journalling file systems).
> > > > 
> > > > It's entirely possible that I'm either plain wrong or a decade out of
> > > > date on this though. CC-ing Konrad in case he has any insights as
> > > > blkback maintainer (I think, MAINTAINERS doesn't have a specific entry)
> > > > 
> > > 
> > > Too bad, as far as I can tell AIO and O_DIRECT are still missing in loop
> > > device. I guess we will have to wait until those two things are
> > > upstreamed.
> > > 
> > 
> > I'm kinda hoping someone will tell me I'm wrong...
> 
> Even without O_DIRECT, using /dev/loop+blkback is safe if the barriers
> in the block protocol are implemented correctly. Last time I look, they
> are, so I think we should be able to switch to it.

Nobody has contradicted you in this, so I've applied the resend of
this patch (from
<1420724813-17920-1-git-send-email-wei.liu2@citrix.com>)