[PATCH] xl: Special case tap/aio for disk validation

[PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

Current disk validation code will fail when the disk file path is prefixed with tap:aio:vhd in the disk configuration file option.  This patch special cases tap:aio validation.

Note:  It appears qcow/qcow2 file format does not work with the current tapdisk.  So, I am checking only for vhd file format.  If there are other formats to check also, let me know.

Signed-off-by: Kamala Narasimhan <kamala.narasimhan@citrix.com>

Kamala

diff -r 67d5b8004947 tools/libxl/libxl.c
--- a/tools/libxl/libxl.c       Wed Jan 26 11:58:45 2011 +0000
+++ b/tools/libxl/libxl.c       Wed Jan 26 14:26:57 2011 -0500
@@ -836,22 +836,40 @@ static int validate_virtual_disk(libxl_c
 static int validate_virtual_disk(libxl_ctx *ctx, char *file_name, libxl_disk_phystype disk_type)
 {
     struct stat stat_buf;
+    const char *fname;

     if ( (file_name[0] == '\0') && (disk_type == PHYSTYPE_EMPTY) )
         return 0;

-    if ( stat(file_name, &stat_buf) != 0 ) {
-        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", file_name);
+    if ( disk_type == PHYSTYPE_AIO ) {
+        fname = strchr(file_name, ':');
+        if ( fname == NULL ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "missing file format prefix!"
+                   " tap:aio disk option must be followed by file format type");
+            return ERROR_INVAL;
+        }
+        fname++;
+        if ( strncmp(file_name, "vhd:", sizeof("vhd:")-1) != 0 ) {
+            LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Only vhd file format supported"
+                   " with tapdisk");
+            return ERROR_INVAL;
+        }
+    }
+    else
+        fname = file_name;
+
+    if ( stat(fname, &stat_buf) != 0 ) {
+        LIBXL__LOG_ERRNO(ctx, LIBXL__LOG_ERROR, "failed to stat %s", fname);
         return ERROR_INVAL;
     }
     if ( disk_type == PHYSTYPE_PHY ) {
         if ( !(S_ISBLK(stat_buf.st_mode)) ) {
             LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s is not a block device!\n",
-                file_name);
+                fname);
             return ERROR_INVAL;
         }
     } else if ( S_ISREG(stat_buf.st_mode) && stat_buf.st_size == 0 ) {
-        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", file_name);
+        LIBXL__LOG(ctx, LIBXL__LOG_ERROR, "Virtual disk %s size is 0!\n", fname);
         return ERROR_INVAL;
     }

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Wed, 26 Jan 2011, Kamala Narasimhan wrote:
> 
> Current disk validation code will fail when the disk file path is prefixed with tap:aio:vhd in the disk configuration file option.  This patch special cases tap:aio validation.
> 
> Note:  It appears qcow/qcow2 file format does not work with the current tapdisk.  So, I am checking only for vhd file format.  If there are other formats to check also, let me know.
> 

This patch made me realize that we really need a specification for the
disk format syntax in the config file and a better xl parser to match it.
The syntax should be clear and at the same time backward compatible as
much as we can.
The followings are disk configurations supported in libxl and what they
mean:

- phy:/path/to/device
blkback is used as block backend, if blkback is missing from your dom0
kernel, this configuration will fail.
A block device must be specified because blkback cannot handle files by
itself.
The format must be raw.

- file:/path/to/file
tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
instead.
Both a file or a block device can be specified.
The format is raw.

- tap:/path/to/file
same as file:

- tap:qcow:/path/to/file
qemu is used as block backend because tapdisk2 has broken qcow support.
Both a file or a block device can be specified.
The format is qcow.

- tap:qcow2:/path/to/file
qemu is used as block backend because tapdisk2 has broken qcow2 support.
Both a file or a block device can be specified.
The format is qcow2.

- tap:vhd:/path/to/file
tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
instead.
Both a file or a block device can be specified.
The format is vhd.


The previous cases cover all the supported disk formats, but for
backward compatibility we also support the following disk configurations:

- tap:aio:, tap:aio:qcow:, tap:aio:qcow2:, tap:aio:vhd:
considering that aio is just an implementation detail, it should not be
part of a user exposed interface, in the xl/libxenlight context aio is
miningless. aio: should just be ignored by the xl config parser.

- tap:tapdisk:
exactly as for aio:, tapdisk: should just be ignored.

- tap:ioemu:
exactly as for aio: and tapdisk:, ioemu: should just be ignored.





Following these notes, we need:

- a document describing this in full details.

- A fix for parse_disk_config.
The new algorithm should assume format=raw and ignore tap:, tap2:, aio:,
tapdisk:, ioemu:, until it gets to a real disk format (qcow:, qcow2:,
vhd:) or the file name.

- A fix for validate_virtual_disk.

- A fix for the usage of aio in libxl: aio is currently considered a
type and should be renamed PHYSTYPE_RAW.

- A fix for libxl_device_disk_add so that QCOW and QCOW2 are
handled by qemu.

All these fixes don't have to be separate patches, some of them might
be collapsed in a sigle patch.

The last fix should be something like this:

---


diff -r 7873885ec74d tools/libxl/libxl.c
--- a/tools/libxl/libxl.c	Thu Jan 27 09:37:19 2011 +0000
+++ b/tools/libxl/libxl.c	Thu Jan 27 14:56:51 2011 +0000
@@ -917,8 +919,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
             /* let's pretend is tap:aio for the moment */
             disk->phystype = PHYSTYPE_AIO;
         case PHYSTYPE_AIO:
-        case PHYSTYPE_QCOW:
-        case PHYSTYPE_QCOW2:
         case PHYSTYPE_VHD:
             if (libxl__blktap_enabled(&gc)) {
                 const char *dev = libxl__blktap_devpath(&gc,
@@ -939,6 +939,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
 
                 break;
             }
+        case PHYSTYPE_QCOW:
+        case PHYSTYPE_QCOW2:
             flexarray_append(back, "params");
             flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
                           libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> - A fix for parse_disk_config.
> The new algorithm should assume format=raw and ignore tap:, tap2:, aio:,
> tapdisk:, ioemu:, until it gets to a real disk format (qcow:, qcow2:,
> vhd:) or the file name.

You can recognise the filename by the fact that it starts with a "/",
since most of our stuff doesn't work with relative pathnames for block
devices and we don't want it to try to make it work now.

So the specification string should always be either
   someprefix:[stuff]
in which case you strip off someprefix: (perhaps making a note of its
implications) or
   /somepath
in which case you've got to the end and know what to do.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Thu, 2011-01-27 at 15:22 +0000, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> > - A fix for parse_disk_config.
> > The new algorithm should assume format=raw and ignore tap:, tap2:, aio:,
> > tapdisk:, ioemu:, until it gets to a real disk format (qcow:, qcow2:,
> > vhd:) or the file name.
> 
> You can recognise the filename by the fact that it starts with a "/",
> since most of our stuff doesn't work with relative pathnames for block
> devices and we don't want it to try to make it work now.

FWIW xend notices a relative path and wacks a "/dev/" on the front.

> So the specification string should always be either
>    someprefix:[stuff]
> in which case you strip off someprefix: (perhaps making a note of its
> implications) or /somepath
> in which case you've got to the end and know what to do.

Shall we pre-deprecate the someprefix: along with all the
tap:{aio,ioemu,FOO}: stuff while we are at this? IOW the syntax we want
to actually support going forward would be:
	(raw|vhd|qcow2|qcow):/path/to/something
with the toolstack being at liberty to best decide how to achieve the
necessary format support? (optionally raw: as the default). All the rest
of the format is really semantically meaningless and supported only for
compatibility with existing configuration files.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Philipp Hahn]

[-- Attachment #1.1: Type: text/plain, Size: 647 bytes --]

Hello,

Am Donnerstag 27 Januar 2011 16:17:01 schrieb Stefano Stabellini:
> - tap:vhd:/path/to/file
> tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
> instead.

As far as I know "vhd" is not supported by qemu and only is implemented in 
tapdisk2. You might recheck that.

Sincerely
Philipp
-- 
Philipp Hahn           Open Source Software Engineer      hahn@univention.de   
Univention GmbH        Linux for Your Business        fon: +49 421 22 232- 0
Mary-Somerville-Str.1  28359 Bremen                   fax: +49 421 22 232-99
                                                    http://www.univention.de/

[-- Attachment #1.2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[-- Attachment #2: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> FWIW xend notices a relative path and wacks a "/dev/" on the front.

Oh dear.  So is the syntax
   some:pile:of:useless:prefixes:sponge
allowed to mean /dev/sponge ?  What if you want to refer to
/dev/sponge:1 ?  I guess that probably doesn't work right now.

So the right algorithm is:
   - rhs starts with '/': it's a pathname
   - rhs has a colon: it's a prefix
   - rhs has neither: hope it's a pathname in /dev
?

> Shall we pre-deprecate the someprefix: along with all the
> tap:{aio,ioemu,FOO}: stuff while we are at this? IOW the syntax we want
> to actually support going forward would be:
> 	(raw|vhd|qcow2|qcow):/path/to/something
> with the toolstack being at liberty to best decide how to achieve the
> necessary format support? (optionally raw: as the default). All the rest
> of the format is really semantically meaningless and supported only for
> compatibility with existing configuration files.

Yes.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

Stefano - Thanks, this is great information that I couldn't find before!  Rest in-line.

> This patch made me realize that we really need a specification for the
> disk format syntax in the config file and a better xl parser to match it.
> The syntax should be clear and at the same time backward compatible as
> much as we can.
> The followings are disk configurations supported in libxl and what they
> mean:
> 
> - phy:/path/to/device
> blkback is used as block backend, if blkback is missing from your dom0
> kernel, this configuration will fail.
So, we add a check in libxl validate disk code to find whether or not blkback is present rather than go all the way and fail?

> A block device must be specified because blkback cannot handle files by
> itself.
> The format must be raw.
> 
We probably need to extend the validation to confirm what is passed is a block device and not a file and I will add that.

> - file:/path/to/file
> tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
> instead.
> Both a file or a block device can be specified.
> The format is raw.
> 
> - tap:/path/to/file
> same as file:
> 
> - tap:qcow:/path/to/file
> qemu is used as block backend because tapdisk2 has broken qcow support.
Maybe we should explicitly fail this case and let the user know that tapdisk2 is broken with qcow so they change the prefix in the config file.  Alternately, we could simply turn the error into a warning and keep going with qemu.

> Both a file or a block device can be specified.
> The format is qcow.
> 
> - tap:qcow2:/path/to/file
> qemu is used as block backend because tapdisk2 has broken qcow2 support.
Same as my comment for qcow.

> Both a file or a block device can be specified.
> The format is qcow2.
> 
> - tap:vhd:/path/to/file
> tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
> instead.
Same as my comment for qcow.

> Both a file or a block device can be specified.
> The format is vhd.
> 
> 
> The previous cases cover all the supported disk formats, but for
> backward compatibility we also support the following disk configurations:
> 
> - tap:aio:, tap:aio:qcow:, tap:aio:qcow2:, tap:aio:vhd:
> considering that aio is just an implementation detail, it should not be
> part of a user exposed interface, in the xl/libxenlight context aio is
> miningless. aio: should just be ignored by the xl config parser.
>
This would require a bit of refactoring in the parsing code to simply skip past aio.  Since we are not accepting interface changes for 4.1, this refactoring would have to wait till post 4.1?
 
> - tap:tapdisk:
> exactly as for aio:, tapdisk: should just be ignored.
> 
I don't think our disk parsing code even considers this option at the moment.  So, we are probably failing when this option is specified.  We would need to tweak the parsing code to parse and ignore this option for backward compatibility.

> - tap:ioemu:
> exactly as for aio: and tapdisk:, ioemu: should just be ignored.
> 
Comment same as that of tap:tapdisk.
> 
> 
Argg... Sorry, some of what I said above might be redundant given that you have summarized what is needed.
> 
> 
> Following these notes, we need:
> 
> - a document describing this in full details.
Would a txt file do?  Where would it go - under tools/examples where other config options are described (albeit in a config file and not a separate file)?
> 
> - A fix for parse_disk_config.
> The new algorithm should assume format=raw and ignore tap:, tap2:, aio:,
> tapdisk:, ioemu:, until it gets to a real disk format (qcow:, qcow2:,
> vhd:) or the file name.
> 
Yes. Specifics/further questions above.

> - A fix for validate_virtual_disk.
>
Agreed.  Further clarifications above.

> - A fix for the usage of aio in libxl: aio is currently considered a
> type and should be renamed PHYSTYPE_RAW.
> 
Of course.  This is the interface change I mentioned above that IanJ might not take before 4.1.

> - A fix for libxl_device_disk_add so that QCOW and QCOW2 are
> handled by qemu.
>
Is you patch below sufficient for that?  Or, is there more that I need done that I am missing?
 
> All these fixes don't have to be separate patches, some of them might
> be collapsed in a sigle patch.
> 
> The last fix should be something like this:
>

Last question -  Are we looking to merge these patches for 4.1 or is this for post 4.1?

Kamala
 
> ---
> 
> 
> diff -r 7873885ec74d tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c	Thu Jan 27 09:37:19 2011 +0000
> +++ b/tools/libxl/libxl.c	Thu Jan 27 14:56:51 2011 +0000
> @@ -917,8 +919,6 @@ int libxl_device_disk_add(libxl_ctx *ctx
>              /* let's pretend is tap:aio for the moment */
>              disk->phystype = PHYSTYPE_AIO;
>          case PHYSTYPE_AIO:
> -        case PHYSTYPE_QCOW:
> -        case PHYSTYPE_QCOW2:
>          case PHYSTYPE_VHD:
>              if (libxl__blktap_enabled(&gc)) {
>                  const char *dev = libxl__blktap_devpath(&gc,
> @@ -939,6 +939,8 @@ int libxl_device_disk_add(libxl_ctx *ctx
>  
>                  break;
>              }
> +        case PHYSTYPE_QCOW:
> +        case PHYSTYPE_QCOW2:
>              flexarray_append(back, "params");
>              flexarray_append(back, libxl__sprintf(&gc, "%s:%s",
>                            libxl__device_disk_string_of_phystype(disk->phystype), disk->physpath));
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

> Shall we pre-deprecate the someprefix: along with all the
> tap:{aio,ioemu,FOO}: stuff while we are at this? IOW the syntax we want
> to actually support going forward would be:
> 	(raw|vhd|qcow2|qcow):/path/to/something
> with the toolstack being at liberty to best decide how to achieve the
> necessary format support? (optionally raw: as the default). All the rest
> of the format is really semantically meaningless and supported only for
> compatibility with existing configuration files.
>

That sure does simply things!  When we have multiple backends that work with different file formats for users to choose from, we can revisit this.

Kamala
 
> Ian.
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

>> Shall we pre-deprecate the someprefix: along with all the
>> tap:{aio,ioemu,FOO}: stuff while we are at this? IOW the syntax we want
>> to actually support going forward would be:
>> 	(raw|vhd|qcow2|qcow):/path/to/something
>> with the toolstack being at liberty to best decide how to achieve the
>> necessary format support? (optionally raw: as the default). All the rest
>> of the format is really semantically meaningless and supported only for
>> compatibility with existing configuration files.
> 
> Yes.
> 

That brings the question on what we do to make it obvious to the user that we are deprecating these options.  That is other than explicitly mentioning it in the documentation.  Do we display a warning in our validation code?

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Thu, 2011-01-27 at 16:23 +0000, Ian Jackson wrote: 
> Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> > FWIW xend notices a relative path and wacks a "/dev/" on the front.
> 
> Oh dear.  So is the syntax
>    some:pile:of:useless:prefixes:sponge
> allowed to mean /dev/sponge ?  What if you want to refer to
> /dev/sponge:1 ?  I guess that probably doesn't work right now.
> 
> So the right algorithm is:
>    - rhs starts with '/': it's a pathname
>    - rhs has a colon: it's a prefix
>    - rhs has neither: hope it's a pathname in /dev
> ?

Sounds about right.

FWIW from xend:

def _parse_uname(uname):
    fn = taptype = None
    if uname.find(":") != -1:
        (typ, fn) = uname.split(":", 1)

        if typ in ("phy", "drbd") and not fn.startswith("/"):
            fn = "/dev/%s" %(fn,)
               
        if typ in ("tap", "tap2"):
            (taptype, fn) = fn.split(":", 1)
    return (fn, taptype)

/me cries.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> 
> Stefano - Thanks, this is great information that I couldn't find before!  Rest in-line.
> 
> > This patch made me realize that we really need a specification for the
> > disk format syntax in the config file and a better xl parser to match it.
> > The syntax should be clear and at the same time backward compatible as
> > much as we can.
> > The followings are disk configurations supported in libxl and what they
> > mean:
> > 
> > - phy:/path/to/device
> > blkback is used as block backend, if blkback is missing from your dom0
> > kernel, this configuration will fail.
> So, we add a check in libxl validate disk code to find whether or not blkback is present rather than go all the way and fail?
> 

Yeah, you can check for the presence of:

/sys/modules/{blkbk,xen_blkback}

the modules are called differently in pvops compared to traditional
xenolinux kernels.


> > A block device must be specified because blkback cannot handle files by
> > itself.
> > The format must be raw.
> > 
> We probably need to extend the validation to confirm what is passed is a block device and not a file and I will add that.

Right.

> > - file:/path/to/file
> > tapdisk2 is used as block backend, if tapdisk2 is missing qemu is used
> > instead.
> > Both a file or a block device can be specified.
> > The format is raw.
> > 
> > - tap:/path/to/file
> > same as file:
> > 
> > - tap:qcow:/path/to/file
> > qemu is used as block backend because tapdisk2 has broken qcow support.
> Maybe we should explicitly fail this case and let the user know that tapdisk2 is broken with qcow so they change the prefix in the config file.  Alternately, we could simply turn the error into a warning and keep going with qemu.
> 

I think there is nothing wrong in using the qemu disk backend,
especially when the new qemu will be available with much better aio
support.
I also think that we shouldn't bother the users with implementation
details on the disk backend we use, the toolstack should just set it up
behind the scene.
So in the qcow and qcow2 cases I wouldn't print anything at all.
However in the vhd case, considering that I have never tested qemu's vhd
support, I don't know how that would actually work. So for the vhd
format it might be OK to just print an error an exit if tapdisk2 is not
present.


> > Both a file or a block device can be specified.
> > The format is vhd.
> > 
> > 
> > The previous cases cover all the supported disk formats, but for
> > backward compatibility we also support the following disk configurations:
> > 
> > - tap:aio:, tap:aio:qcow:, tap:aio:qcow2:, tap:aio:vhd:
> > considering that aio is just an implementation detail, it should not be
> > part of a user exposed interface, in the xl/libxenlight context aio is
> > miningless. aio: should just be ignored by the xl config parser.
> >
> This would require a bit of refactoring in the parsing code to simply skip past aio.  Since we are not accepting interface changes for 4.1, this refactoring would have to wait till post 4.1?
>  

I think this fix can be considered critical so we'll accept it for 4.1.


> > - tap:tapdisk:
> > exactly as for aio:, tapdisk: should just be ignored.
> > 
> I don't think our disk parsing code even considers this option at the moment.  So, we are probably failing when this option is specified.  We would need to tweak the parsing code to parse and ignore this option for backward compatibility.
> 

Right.


> > 
> > Following these notes, we need:
> > 
> > - a document describing this in full details.
> Would a txt file do?  Where would it go - under tools/examples where other config options are described (albeit in a config file and not a separate file)?

Yep. A text file under docs will be perfect.


 
> > - A fix for the usage of aio in libxl: aio is currently considered a
> > type and should be renamed PHYSTYPE_RAW.
> > 
> Of course.  This is the interface change I mentioned above that IanJ might not take before 4.1.

We'll take it because it is a critical fix. Xl has to be compatibile
with xend.

> 
> > - A fix for libxl_device_disk_add so that QCOW and QCOW2 are
> > handled by qemu.
> >
> Is you patch below sufficient for that?  Or, is there more that I need done that I am missing?
>  

I think the patch below should be sufficient, once the disk parsing is
fixed (in particular "aio:" is removed).

> > All these fixes don't have to be separate patches, some of them might
> > be collapsed in a sigle patch.
> > 
> > The last fix should be something like this:
> >
> 
> Last question -  Are we looking to merge these patches for 4.1 or is this for post 4.1?
> 

the target is 4.1

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> >> Shall we pre-deprecate the someprefix: along with all the
> >> tap:{aio,ioemu,FOO}: stuff while we are at this? IOW the syntax we want
> >> to actually support going forward would be:
> >> 	(raw|vhd|qcow2|qcow):/path/to/something
> >> with the toolstack being at liberty to best decide how to achieve the
> >> necessary format support? (optionally raw: as the default). All the rest
> >> of the format is really semantically meaningless and supported only for
> >> compatibility with existing configuration files.
> > 
> > Yes.
> > 
> 
> That brings the question on what we do to make it obvious to the user that we are deprecating these options.  That is other than explicitly mentioning it in the documentation.  Do we display a warning in our validation code?
> 

While fixing the disk parsing is critical for 4.1, I am not sure if we
can deprecate the current syntax for 4.1 as well.
We should keep in mind that xend is still using the other syntax and it
is still present in-tree, so if we really want to deprecate it we need
to fix xend too. 

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> Yeah, you can check for the presence of:
> 
> /sys/modules/{blkbk,xen_blkback}
> 
> the modules are called differently in pvops compared to traditional
> xenolinux kernels.

No, that doesn't work because it may not be a module.  And we need an
answer that works with 2.6.18 as well as 2.6.27 and 2.6.32 and 2.6.37
and 2.6.38 ...

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Thu, 2011-01-27 at 18:35 +0000, Ian Jackson wrote: 
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> > Yeah, you can check for the presence of:
> > 
> > /sys/modules/{blkbk,xen_blkback}
> > 
> > the modules are called differently in pvops compared to traditional
> > xenolinux kernels.
> 
> No, that doesn't work because it may not be a module.

These entries appear for builtin things too (badly named directory? why
yes) if the "module" has any parameters. blkback does appear, I checked
on both types of dom0 with static blkback.

It looks like /sys/bus/xen-backend/drivers/vbd/
and /sys/bus/xen/drivers/vbd/ are also both present on systems with
blkback available and have consistent names on both classic-Xen and
pvops systems AFAICT.

One of these last two is likely to be the right answer, since the "vbd"
part of the string is the xenstore directory name too and the two are
linked.

> And we need an answer that works with 2.6.18 as well as 2.6.27 and 2.6.32 and 2.6.37
> and 2.6.38 ...

2.6.18 would be the unknown here. Should be easy enough for someone to
confirm I guess.

I'm not sure that failing to detect blkback on 2.6.18 and therefore
falling back to tapdisk* and/or qemu is a huge problem but I suppose
it's best avoided.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Thu, 27 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> > Yeah, you can check for the presence of:
> > 
> > /sys/modules/{blkbk,xen_blkback}
> > 
> > the modules are called differently in pvops compared to traditional
> > xenolinux kernels.
> 
> No, that doesn't work because it may not be a module.  And we need an
> answer that works with 2.6.18 as well as 2.6.27 and 2.6.32 and 2.6.37
> and 2.6.38 ...
> 

I think that they should be present even if they are built-in.
All the pvops kernels should have:

/sys/modules/xen_blkback

while all the xenlinux kernels should have:

/sys/modules/blkbk

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

> While fixing the disk parsing is critical for 4.1, I am not sure if we
> can deprecate the current syntax for 4.1 as well.
> We should keep in mind that xend is still using the other syntax and it
> is still present in-tree, so if we really want to deprecate it we need
> to fix xend too. 

By deprecating I didn't mean we would remove the functionality and introduce inconsistency between xend and xl in terms of usage.  I wasn't sure if we should stop with documenting the fact that it is deprecated or also display a warning message (in libxl) to that effect.  I would go with a warning also unless someone disapproves.

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

> - phy:/path/to/device
> - file:/path/to/file
> - tap:/path/to/file

Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?  If so do we treat it same as 'phy' and use blkback for backend?

Regarding VDEV (appears to be referred to as drive designation in some specificatons) -

Other than hd[x] and xvd[x], are there other supported VDEV formats?

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[M A Young]

On Thu, 27 Jan 2011, Stefano Stabellini wrote:

> - phy:/path/to/device
> blkback is used as block backend, if blkback is missing from your dom0
> kernel, this configuration will fail.
> A block device must be specified because blkback cannot handle files by
> itself.
> The format must be raw.

If you want backwards compatibility then phy:devname works in xm as well 
as phy:/dev/devname . Many of the example files in /etc/xen still use the 
earlier format.

 	Michael Young

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

>>> Following these notes, we need:
>>>
>>> - a document describing this in full details.
>> Would a txt file do?  Where would it go - under tools/examples where other config options are described (albeit in a config file and not a separate file)?
> 
> Yep. A text file under docs will be perfect.
> 
> 
>  
Attached is a first stab at this for your perusal.  It is mostly based on the discussion we had in this thread.  Please let me know any/all changes you would recommend.

I am sending this in advance as anything I have captured wrong here would mean it is likely that the code changes I am making right now could also be wrong!  So please do respond back if I have incorrectly captured any  conclusion we arrived at in terms of the supported/deprecated attributes for the disk configuration option.  Thanks!

Kamala


[-- Attachment #2: xl-disk-configuration.txt --]
[-- Type: text/plain, Size: 3617 bytes --]

                                             ----------------------------
                                             xl Disk Configuration Option
                                             ----------------------------

This document provides a brief description of xl disk configuration option, different attributes that can be passed through it and the format in which they need to be specified.

At a higher level, xl disk configuration option takes the following format:

    disk = [ 'format:path,vdev:type,attrib', 'format:path,vdev:type,attrib', ... ]

Not all attributes are required and some are deprecated.  Following is a brief description of each of the attribute along with information on whether or not they are mandatory.

format:

Description:         Specifies the format of image file.
Supported values:    raw, qcow, qcow2, vhd 
Deprecated values:   None
Mandatory:           No. When not specified raw format is assumed.  For a physical block device the format must be raw and need not be explicitly specified.  For an image file the format could be one of the supported values and when not specified assumed to be raw.
Backend used:        For qcow/qcow2 qemu backend is used as tapdisk2 does not work with these formats.  For raw format image file, tapdisk2 is used and when not available qemu backend is used as fallback.  For vhd format tapdisk2 is used and tapdisk2 unavailability will result in failure as qemu fallback option does not support vhd file format.                       
 
path:

Description:         Block device or image file path.  For a physical block device a /dev will be prepended when not specified and when the path doesn't start with a '/'.
Supported values:    N/A
Deprecated values:   N/A
Mandatory:           Yes

vdev:

Description:         Virtual device as seen by the guest (also referred to as guest drive designation in some specifications).
Supported values:    hd[x], xvd[x]
Deprecated values:   None
Mandatory:           Yes

type:

Description:         Qualifies virtual device type.
Supported values:    cdrom
Deprecated values:   None
Mandatory:           No

attrib:

Description:         Specifies access control information.  Whether or not the block device is provided to the guest in read-only or read-write mode depends on this attribute.
Supported values:    'r', 'w' 
Deprecated values:   None
Mandatory:           Yes

Following are deprecated attributes.  Though these atrributes are supported for backward compatibility, it's usage is not recommended and support for it is likely to be dropped in future versions of xl.

block-dev-type:

Description:         Specifies the block device type.
Supported values:    phy, file, tap 
Backend used:        blkback for type 'phy'.  Will result in failure when dom0 does not support blkback backend.  For block device type 'file' with format raw or when no format specfied, tapdisk2 is used when present otherwise qemu fallback option is used.  For 'file' or 'tap' block device type with format 'vhd', only tapdisk2 is used as qemu does not support vhd format.  Absence of tapdisk2 support in this case will result in failure as specified earlier.  

access-type:

Description:         Backend implementation option to choose from while accessing block device. Example: tap:aio:vhd:/path/to/file
Supported values:    'aio', 'tapdisk', 'ioemu'

Example usages:

disk = [ 'vhd:/path/to/dev,hda,w', '/path/to/iso,hdc:cdrom,r' ]
disk = [ 'path/to/phy/dev,hda,w', '/dev/cdrom,hdc:cdrom,r' ]
disk = [ 'qcow:/path/to/file,hda,w' ]
disk = [ 'qcow2:/path/to/file,hda,w', 'raw:/path/to/dev,hdc:cdrom,r' ]



[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Thu, 2011-01-27 at 20:14 +0000, Kamala Narasimhan wrote:
> > While fixing the disk parsing is critical for 4.1, I am not sure if we
> > can deprecate the current syntax for 4.1 as well.
> > We should keep in mind that xend is still using the other syntax and it
> > is still present in-tree, so if we really want to deprecate it we need
> > to fix xend too. 
> 
> By deprecating I didn't mean we would remove the functionality and
> introduce inconsistency between xend and xl in terms of usage.  I
> wasn't sure if we should stop with documenting the fact that it is
> deprecated or also display a warning message (in libxl) to that
> effect.  I would go with a warning also unless someone disapproves.

When I originally said "deprecate" I really meant "plan to deprecate",
i.e. accept the new and old syntax now and intend to deprecate the old
syntax e.g. in 4.2.

WRT xend compatibility, I think the goal that a xend/xm configuration
file can be fed to xl without modification is a worthy one and obviously
aids in the transition etc.

The opposite goal (that an xl configuration file can be fed directly to
xm) is not so obviously worthwhile.

I don't think we should be hobbling the evolution and ongoing use of the
xl configuration file syntax (or indeed the (lib)xl functionality)
simply to keep xend happy, in just the same way as we currently don't
insist that every feature added to (lib)xl also gets added to xend.

I guess what I mean is that if people use xl specific syntax/options in
their configuration files then they should not expect things to Just
Work if they go back to xend.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Fri, 2011-01-28 at 01:56 +0000, Kamala Narasimhan wrote:
> >>> Following these notes, we need:
> >>>
> >>> - a document describing this in full details.
> >> Would a txt file do?  Where would it go - under tools/examples where other config options are described (albeit in a config file and not a separate file)?
> > 
> > Yep. A text file under docs will be perfect.
> > 
> > 
> >  
> Attached is a first stab at this for your perusal.  It is mostly based
> on the discussion we had in this thread.  Please let me know any/all
> changes you would recommend.
> 
> I am sending this in advance as anything I have captured wrong here
> would mean it is likely that the code changes I am making right now
> could also be wrong!  So please do respond back if I have incorrectly
> captured any  conclusion we arrived at in terms of the
> supported/deprecated attributes for the disk configuration option.
> Thanks!

Nice one. I have a handful of minor comments.

There's a bunch of examples of this stuff in tools/examples, much of
which would benefit from the addition of a reference to this document in
its eventual resting place and sanity checking for adherence to what the
document says. It's hard to believe (ok, not really) that we don't have
any other in tree documentation of any of the other configuration syntax
other than those examples (and some vagueish lists in
tools/python/README.*) but it does seem to be the case :-(. Perhaps
there is something in the wiki or else maybe this document can be the
seed around which the rest grows...

Several elements of the 'format:path,vdev:type,attrib' string are
optional. Can we delineate those in some non-confusing way? Often man
pages and the like use [], which would end up as (I think):
	[format:]path,vdev[:type],attrib

I don't know if that meets the non-confusing criterion though. I didn't
initially notice the "Mandatory" tag (see comments on line-wrapping
below). The thing the tag doesn't cover is how the punctuation binds to
the mandatory vs. optional elements. Perhaps insert
        The "format:" and ":type" elements are optional
alongside the comment about not all attributes being required?

IanJ wrote a existing spec for vdev at one point. I thought it had been
committed somewhere but I can't see it. I think the most recent version
was
http://lists.xensource.com/archives/html/xen-devel/2010-09/msg01329.html
but perhaps IanJ can confirm. We should incorporate it, either by
reference or as a separate chapter in the same document or something
similar.

The discussion of which backend corresponds to each option is useful
from an implementation detail point of view but I'm not sure it is
necessary as part of the documentation of the configuration syntax. It's
liable get out of date IMHO. Perhaps those bits would be better as a
comment alongside the implementation?

It's not clear where the deprecated block-dev-type and access-type
attributes are acceptable. I think we can just say that multiple
deprecated attributes are accepted before the first valid "format" type
is discovered, with the last one taking precedence, or something along
those lines.

Lastly, I think the doc should be line-wrapped to 80 columns, otherwise
the autowrapping of the table like elements ends up quite hard to
follow. e.g. it currently comes out looking like:
        
        Description:         Specifies the format of image file.
        Supported values:    raw, qcow, qcow2, vhd 
        Deprecated values:   None
        Mandatory:           No. When not specified raw format is
        assumed.  For a physical block device the format must be raw and
        need not be explicitly specified.  For an image file the format
        could be one of the supported values and when not specified
        assumed to be raw.
        Backend used:        For qcow/qcow2 qemu backend is u[...]
        
should be (subject to my guessing where 80 columns actually is in this
case):

        Description:         Specifies the format of image file.
        Supported values:    raw, qcow, qcow2, vhd 
        Deprecated values:   None
        Mandatory:           No. When not specified raw format is assumed.  For a physical block device the format
                             must be raw and need not be explicitly specified.  For an image file the format could
                             be one of the supported values and when not specified assumed to be raw.
        Backend used:        For qcow/qcow2 qemu backend is u[...]
                             [...] etc 

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Fri, 2011-01-28 at 10:06 +0000, Ian Campbell wrote:
> It's hard to believe (ok, not really) that we don't have
> any other in tree documentation of any of the other configuration
> syntax other than those examples (and some vagueish lists in
> tools/python/README.*) but it does seem to be the case :-(. Perhaps
> there is something in the wiki or else maybe this document can be the
> seed around which the rest grows... 

I found http://wiki.xen.org/xenwiki/XenConfigurationFileOptions (which
also refers to
http://www.xen.org/files/Support/XenConfigurationDetails.pdf).

I'm not sure if the canonical place for this sort of thing should be the
wiki (where it is simple for anyone to update, but where it might get
out of date or suffer from version skew etc) or in-tree (where we can
patch as part of the implementation of new features).

I'd be inclined to suggest we take that wiki page, text-ify it, sanity
check for updatedness, stick it in the tree and update the wiki to
reference it. Opinions?

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Ian Campbell writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> I'm not sure if the canonical place for this sort of thing should be the
> wiki (where it is simple for anyone to update, but where it might get
> out of date or suffer from version skew etc) or in-tree (where we can
> patch as part of the implementation of new features).

There should be primary reference documentation, of every syntax,
public interface, and feature, in-tree.  This should be in plain text
files or perhaps manpages or something equally easy to edit and view.

That way we can require people who add new features to update the
documentation, and users will have the documentation which corresponds
to the code that they have.

> I'd be inclined to suggest we take that wiki page, text-ify it, sanity
> check for updatedness, stick it in the tree and update the wiki to
> reference it. Opinions?

That would be a good start.  NB that there's also the xm-config.5
manpage.  All of this needs to be unified.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Campbell wrote:
> On Thu, 2011-01-27 at 20:14 +0000, Kamala Narasimhan wrote:
> > > While fixing the disk parsing is critical for 4.1, I am not sure if we
> > > can deprecate the current syntax for 4.1 as well.
> > > We should keep in mind that xend is still using the other syntax and it
> > > is still present in-tree, so if we really want to deprecate it we need
> > > to fix xend too. 
> > 
> > By deprecating I didn't mean we would remove the functionality and
> > introduce inconsistency between xend and xl in terms of usage.  I
> > wasn't sure if we should stop with documenting the fact that it is
> > deprecated or also display a warning message (in libxl) to that
> > effect.  I would go with a warning also unless someone disapproves.
> 
> When I originally said "deprecate" I really meant "plan to deprecate",
> i.e. accept the new and old syntax now and intend to deprecate the old
> syntax e.g. in 4.2.
> 

Yes, this plan is much more feasible.

> WRT xend compatibility, I think the goal that a xend/xm configuration
> file can be fed to xl without modification is a worthy one and obviously
> aids in the transition etc.
> 
> The opposite goal (that an xl configuration file can be fed directly to
> xm) is not so obviously worthwhile.
> 
> I don't think we should be hobbling the evolution and ongoing use of the
> xl configuration file syntax (or indeed the (lib)xl functionality)
> simply to keep xend happy, in just the same way as we currently don't
> insist that every feature added to (lib)xl also gets added to xend.
> 
> I guess what I mean is that if people use xl specific syntax/options in
> their configuration files then they should not expect things to Just
> Work if they go back to xend.
 
+1

xl: drdb support

[Stefano Stabellini]

On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > - phy:/path/to/device
> > - file:/path/to/file
> > - tap:/path/to/file
> 
> Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?  If so do we treat it same as 'phy' and use blkback for backend?

Yes, I think so. Given his recent work on drdb support in qemu, maybe
James can comment on this.

> 
> Regarding VDEV (appears to be referred to as drive designation in some specificatons) -
> 
> Other than hd[x] and xvd[x], are there other supported VDEV formats?
> 

No.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Kamala Narasimhan wrote:
> >>> Following these notes, we need:
> >>>
> >>> - a document describing this in full details.
> >> Would a txt file do?  Where would it go - under tools/examples where other config options are described (albeit in a config file and not a separate file)?
> > 
> > Yep. A text file under docs will be perfect.
> > 
> > 
> >  
> Attached is a first stab at this for your perusal.  It is mostly based on the discussion we had in this thread.  Please let me know any/all changes you would recommend.
> 
> I am sending this in advance as anything I have captured wrong here would mean it is likely that the code changes I am making right now could also be wrong!  So please do respond back if I have incorrectly captured any  conclusion we arrived at in terms of the supported/deprecated attributes for the disk configuration option.  Thanks!
> 

I think it is pretty good.
You need to add tap2 among the block-dev-types.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Campbell wrote:
> The discussion of which backend corresponds to each option is useful
> from an implementation detail point of view but I'm not sure it is
> necessary as part of the documentation of the configuration syntax. It's
> liable get out of date IMHO. Perhaps those bits would be better as a
> comment alongside the implementation?
> 

I think it is really important that we write it down somewhere in a
document, after all users have to know what to compile in order to get
things to work.
However considering that this is an interface description, there is an
argument for moving this information elsewhere, but for simplicity's
sake I would just leave it there.

> Lastly, I think the doc should be line-wrapped to 80 columns, otherwise

if you are using thunderbird you can use this extension to word wrap
your email:

https://addons.mozilla.org/en-US/thunderbird/addon/toggle-word-wrap/

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Fri, 2011-01-28 at 13:19 +0000, Stefano Stabellini wrote:
> 
> 
> > Lastly, I think the doc should be line-wrapped to 80 columns,
> otherwise
> 
> if you are using thunderbird you can use this extension to word wrap
> your email:
> 
> https://addons.mozilla.org/en-US/thunderbird/addon/toggle-word-wrap/

I'm not worried about the email, I'm worried about the format of the
document checked into xen-unstable which will be read with all sorts of
different tools.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Campbell wrote:
> On Fri, 2011-01-28 at 13:19 +0000, Stefano Stabellini wrote:
> > 
> > 
> > > Lastly, I think the doc should be line-wrapped to 80 columns,
> > otherwise
> > 
> > if you are using thunderbird you can use this extension to word wrap
> > your email:
> > 
> > https://addons.mozilla.org/en-US/thunderbird/addon/toggle-word-wrap/
> 
> I'm not worried about the email, I'm worried about the format of the
> document checked into xen-unstable which will be read with all sorts of
> different tools.

the implicit next step is to write the document inline in the email with
that extension:)

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Campbell]

On Fri, 2011-01-28 at 13:28 +0000, Stefano Stabellini wrote:
> On Fri, 28 Jan 2011, Ian Campbell wrote:
> > On Fri, 2011-01-28 at 13:19 +0000, Stefano Stabellini wrote:
> > > 
> > > 
> > > > Lastly, I think the doc should be line-wrapped to 80 columns,
> > > otherwise
> > > 
> > > if you are using thunderbird you can use this extension to word wrap
> > > your email:
> > > 
> > > https://addons.mozilla.org/en-US/thunderbird/addon/toggle-word-wrap/
> > 
> > I'm not worried about the email, I'm worried about the format of the
> > document checked into xen-unstable which will be read with all sorts of
> > different tools.
> 
> the implicit next step is to write the document inline in the email with
> that extension:)

Oh right, sure ;-)

I think it probably needs a bit more manual formatting than just
automatic wordwrap to make it readable though, e.g. to indent the table
in a readable way.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

> Several elements of the 'format:path,vdev:type,attrib' string are
> optional. Can we delineate those in some non-confusing way? Often man
> pages and the like use [], which would end up as (I think):
> 	[format:]path,vdev[:type],attrib
>
Believe it or not I tried that!  It ended up looking very confusing which is why
I switched to explicit verbal description of whether or not a particular
attribute is mandatory.

> I don't know if that meets the non-confusing criterion though. I didn't
> initially notice the "Mandatory" tag (see comments on line-wrapping
> below). The thing the tag doesn't cover is how the punctuation binds to
> the mandatory vs. optional elements. Perhaps insert
>         The "format:" and ":type" elements are optional
> alongside the comment about not all attributes being required?
> 
Sure, if that helps.

> IanJ wrote a existing spec for vdev at one point. I thought it had been
> committed somewhere but I can't see it. I think the most recent version
> was
> http://lists.xensource.com/archives/html/xen-devel/2010-09/msg01329.html
> but perhaps IanJ can confirm. We should incorporate it, either by
> reference or as a separate chapter in the same document or something
> similar.
> 
Sure.

> The discussion of which backend corresponds to each option is useful
> from an implementation detail point of view but I'm not sure it is
> necessary as part of the documentation of the configuration syntax. It's
> liable get out of date IMHO. Perhaps those bits would be better as a
> comment alongside the implementation?
> 
I do agree this is an information a pure end user won't care about.  The
question then gets to who is the target audience for this document.  If it is
mostly developers, it might help for them to have this information.

> It's not clear where the deprecated block-dev-type and access-type
> attributes are acceptable. I think we can just say that multiple
> deprecated attributes are accepted before the first valid "format" type
> is discovered, with the last one taking precedence, or something along
> those lines.
>
Sure.

> Lastly, I think the doc should be line-wrapped to 80 columns, otherwise
> the autowrapping of the table like elements ends up quite hard to
> follow. e.g. it currently comes out looking like:
>         
>         Description:         Specifies the format of image file.
>         Supported values:    raw, qcow, qcow2, vhd 
>         Deprecated values:   None
>         Mandatory:           No. When not specified raw format is
>         assumed.  For a physical block device the format must be raw and
>         need not be explicitly specified.  For an image file the format
>         could be one of the supported values and when not specified
>         assumed to be raw.
>         Backend used:        For qcow/qcow2 qemu backend is u[...]
>
> should be (subject to my guessing where 80 columns actually is in this
> case):
> 
>         Description:         Specifies the format of image file.
>         Supported values:    raw, qcow, qcow2, vhd 
>         Deprecated values:   None
>         Mandatory:           No. When not specified raw format is assumed.  For a physical block device the format
>                              must be raw and need not be explicitly specified.  For an image file the format could
>                              be one of the supported values and when not specified assumed to be raw.
>         Backend used:        For qcow/qcow2 qemu backend is u[...]
>                              [...] etc 
> 
Yep, I will do that and send out a revised version.

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

Stefano Stabellini wrote:
> On Fri, 28 Jan 2011, Ian Campbell wrote:
>> On Fri, 2011-01-28 at 13:19 +0000, Stefano Stabellini wrote:
>>>
>>>> Lastly, I think the doc should be line-wrapped to 80 columns,
>>> otherwise
>>>
>>> if you are using thunderbird you can use this extension to word wrap
>>> your email:
>>>
>>> https://addons.mozilla.org/en-US/thunderbird/addon/toggle-word-wrap/
>> I'm not worried about the email, I'm worried about the format of the
>> document checked into xen-unstable which will be read with all sorts of
>> different tools.
> 
> the implicit next step is to write the document inline in the email with
> that extension:)

Yes, I have that addon installed.  But it doesn't allow me to selectively word
wrap parts of the message which I would need while submitting patches (as I
wouldn't want to word wrap code) which is why I turned off word wrapping
completely.  I included the text file as an attachment and wasn't expecting it
to inline it to the email (which I then realized would be the case with some
email clients)!  Anyway, as suggested I should be fixing the format in the txt
file which I will.

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[Jim Fehlig]

Kamala Narasimhan wrote:
>> - phy:/path/to/device
>> - file:/path/to/file
>> - tap:/path/to/file
>>     
>
> Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?

The xend toolstack supports arbitrary external block device types.  We
have block-iscsi and block-npiv,  and the drbd project provides
block-drbd.  I'm quite sure I've seen other custom block types as well. 
Will these arbitrary block device types still be supported in the new
toolstack?

Thanks,
Jim

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Jim Fehlig wrote:
> Kamala Narasimhan wrote:
> >> - phy:/path/to/device
> >> - file:/path/to/file
> >> - tap:/path/to/file
> >>     
> >
> > Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?
> 
> The xend toolstack supports arbitrary external block device types.  We
> have block-iscsi and block-npiv,  and the drbd project provides
> block-drbd.  I'm quite sure I've seen other custom block types as well. 
> Will these arbitrary block device types still be supported in the new
> toolstack?

Is it just a matter of forking and executing an external script to
setup a block device and threat it as phy: afterwards?
It shouldn't be too difficult but still non-trivial at this point of the
release cycle.
Maybe users could work around the lack of support in 4.1 calling the
script themselves and using the name of the block device in the VM
config file directly?

Re: xl: drdb support

[Shriram Rajagopalan]

its drbd :)

On Fri, Jan 28, 2011 at 4:57 AM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
>> > - phy:/path/to/device
>> > - file:/path/to/file
>> > - tap:/path/to/file
>>
>> Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?  If so do we treat it same as 'phy' and use blkback for backend?
>
> Yes, I think so. Given his recent work on drdb support in qemu, maybe
> James can comment on this.
>
>>
>> Regarding VDEV (appears to be referred to as drive designation in some specificatons) -
>>
>> Other than hd[x] and xvd[x], are there other supported VDEV formats?
>>
>
> No.
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
>



-- 
perception is but an offspring of its own self

Re: [PATCH] xl: Special case tap/aio for disk validation

[Jim Fehlig]

Stefano Stabellini wrote:
> On Fri, 28 Jan 2011, Jim Fehlig wrote:
>   
>> Kamala Narasimhan wrote:
>>     
>>>> - phy:/path/to/device
>>>> - file:/path/to/file
>>>> - tap:/path/to/file
>>>>     
>>>>         
>>> Along with phy, file and tap for block device types there appear to be a 'drdb' option too.  Do we actively support it?
>>>       
>> The xend toolstack supports arbitrary external block device types.  We
>> have block-iscsi and block-npiv,  and the drbd project provides
>> block-drbd.  I'm quite sure I've seen other custom block types as well. 
>> Will these arbitrary block device types still be supported in the new
>> toolstack?
>>     
>
> Is it just a matter of forking and executing an external script to
> setup a block device and threat it as phy: afterwards?
>   

Currently, IIUC, the external script is invoked by udev

SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap
$env{ACTION}"
SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block
$env{ACTION}"

The block script will delegate to a helper if needed, which configures
the block device and writes some info (like resulting dev node) to
xenstore.  xend and/or qemu then read the info and continue configuring
the device from backend perspective.

> It shouldn't be too difficult but still non-trivial at this point of the
> release cycle.
> Maybe users could work around the lack of support in 4.1 calling the
> script themselves and using the name of the block device in the VM
> config file directly?
>   

Yes, this is certainly a workaround.  One could argue this is the proper
solution and remove support for external block scripts.  But this
functionality has been around for years.

Jim

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

> IanJ wrote a existing spec for vdev at one point. I thought it had been
> committed somewhere but I can't see it. I think the most recent version
> was
> http://lists.xensource.com/archives/html/xen-devel/2010-09/msg01329.html
> but perhaps IanJ can confirm. We should incorporate it, either by
> reference or as a separate chapter in the same document or something
> similar.
> 

IanJ - I am also unable to find the txt doc mentioned in the link above.  Please
could you confirm whether or not this txt file is likely to be added, if not why
and whether there are other relevant docs for reference?  I will add then all to
a "Reference" or "Related documents" section.

Kamala

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Jim Fehlig wrote:
> Currently, IIUC, the external script is invoked by udev
> 
> SUBSYSTEM=="xen-backend", KERNEL=="tap*", RUN+="/etc/xen/scripts/blktap
> $env{ACTION}"
> SUBSYSTEM=="xen-backend", KERNEL=="vbd*", RUN+="/etc/xen/scripts/block
> $env{ACTION}"
> 
> The block script will delegate to a helper if needed, which configures
> the block device and writes some info (like resulting dev node) to
> xenstore.  xend and/or qemu then read the info and continue configuring
> the device from backend perspective.
> 

When we add support for this to xl we are probably going to execute
these scripts directly. Having udev doing it for us doesn't simplify
things.


> > It shouldn't be too difficult but still non-trivial at this point of the
> > release cycle.
> > Maybe users could work around the lack of support in 4.1 calling the
> > script themselves and using the name of the block device in the VM
> > config file directly?
> >   
> 
> Yes, this is certainly a workaround.  One could argue this is the proper
> solution and remove support for external block scripts.  But this
> functionality has been around for years.

I think it is the best we can do for 4.1, given the timeframe available.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Jim Fehlig writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> The xend toolstack supports arbitrary external block device types.  We
> have block-iscsi and block-npiv,  and the drbd project provides
> block-drbd.  I'm quite sure I've seen other custom block types as well. 
> Will these arbitrary block device types still be supported in the new
> toolstack?

Yes, but I think we will have to change the interface to the block
scripts.  The current arrangements are baroque to say the least.  So
probably not in 4.1's xl, sorry.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Ian Jackson]

Kamala Narasimhan writes ("Re: [Xen-devel] [PATCH] xl: Special case tap/aio for disk validation"):
> Attached is a first stab at this for your perusal.  It is mostly
> based on the discussion we had in this thread.  Please let me know
> any/all changes you would recommend.

Thanks.  I concur with Ian Campbell's comments, but I have some of my own:

> At a higher level, xl disk configuration option takes the following format:
> 
>     disk = [ 'format:path,vdev:type,attrib', 'format:path,vdev:type,attrib', ... ]

I think Ian's comments about using [ ] for optional elements are very
apposite.  If it's too confusing because of the surrounding
pythonesque [ ] you could try something like
   disk = [ 'disk-spec', 'disk-spec' ]
   where disk-spec is
   [format:]path,vdev[:type][,attribute]
(or whatever is actually true).

Also as well as the doc needing to stay within 70-75 columns, and
certainly within 79, it would be sensible for your emails to do so too
as it makes them easier to read in "traditional" clients.


> Description:         Block device or image file path.  For a physical block device a /dev will be prepended when not specified and when the path doesn't start with a '/'.
> Supported values:    N/A
> Deprecated values:   N/A
> Mandatory:           Yes

I think in fact it's not mandatory because for removeable devices it
can be specified as empty to mean "removed".


> Description:         Virtual device as seen by the guest (also referred to as guest drive designation in some specifications).

It would be sensible refer here to my virtual disk specification doc.


On which subject you wrote:
> IanJ - I am also unable to find the txt doc mentioned in the link
> above.  Please could you confirm whether or not this txt file is
> likely to be added, if not why and whether there are other relevant
> docs for reference?  I will add then all to a "Reference" or
> "Related documents" section.

The document in that url which Ian refers to
  http://lists.xensource.com/archives/html/xen-devel/2010-09/msg01329.html
was never checked into the Xen tree.

There was a bit of discussion about it but I never got a clear ack.
If someone would like to give me an ack now I will commit it :-).

> Supported values:    hd[x], xvd[x]

As you'll see from the doc, it's a bit more complicated than that.


Thanks,
Ian.

Re: xl: drdb support

[Ian Jackson]

Stefano Stabellini writes ("[Xen-devel] xl: drdb support"):
> On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > Other than hd[x] and xvd[x], are there other supported VDEV formats?

This is not the case.  There are numeric formats of various kinds,
too, and also sd[x].

Ian.

Re: xl: drdb support

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] xl: drdb support"):
> > On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > > Other than hd[x] and xvd[x], are there other supported VDEV formats?
> 
> This is not the case.  There are numeric formats of various kinds,
> too, and also sd[x].
 
sd[x] doesn't work at the moment, so I wouldn't add it to the document.
When (and if) it gets fixed then we can add it.

Re: xl: drdb support

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] xl: drdb support"):
> sd[x] doesn't work at the moment, so I wouldn't add it to the document.
> When (and if) it gets fixed then we can add it.

What you mean is that it doesn't work for HVM guests with current
versions of qemu.

However it _does_ work with at least some PV guests, for example
Linux 2.6.18.  Therefore it should be listed.

Ian.

Re: xl: drdb support

[Stefano Stabellini]

On Fri, 28 Jan 2011, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] xl: drdb support"):
> > sd[x] doesn't work at the moment, so I wouldn't add it to the document.
> > When (and if) it gets fixed then we can add it.
> 
> What you mean is that it doesn't work for HVM guests with current
> versions of qemu.
> 
> However it _does_ work with at least some PV guests, for example
> Linux 2.6.18.  Therefore it should be listed.

this is a quote from your document:

So for Linux PV guests, users are recommended to supply xvd* devices
only.  Modern PV drivers will map these to identically-named devices
in the guest.

Re: xl: drdb support

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] xl: drdb support"):
> this is a quote from your document:
> 
> So for Linux PV guests, users are recommended to supply xvd* devices
> only.  Modern PV drivers will map these to identically-named devices
> in the guest.

Yes.  But existing setups which specify sd* must still work.  Because
we must continue to support the configuration inside those old guests
which expects sd*.

So using sd* with Linux is deprecated but (a) it should still be in
the document and (b) this is a fact which is decided on by those who
develop Linux, not by Xen.

Using sd* with other PV guests is entirely supported and reasonable.
I don't have a complete inventory of which guests support what.

Ian.

Re: [PATCH] xl: Special case tap/aio for disk validation

[Kamala Narasimhan]

[-- Attachment #1: Type: text/plain, Size: 475 bytes --]

I am attaching a newer version of the txt file that incorporates all the review
comments.  Here are the only changes I am yet to make -

1) Refer to IanJ's virtual disk specification once it is checked-in.
2) Include information pertaining to drbd and other arbitrary block device
types.  This information can be included post 4.1 as I gather from the
discussion on this topic.

Please let me know if I neglected any of your comments or if you have further
comments.

Kamala

[-- Attachment #2: xl-disk-configuration.txt --]
[-- Type: text/plain, Size: 5251 bytes --]



                         ----------------------------
                         xl Disk Configuration Option
                         ----------------------------

This document provides a brief description of xl disk configuration
option, different attributes that can be passed through it and the
format in which they need to be specified.

At a higher level, xl disk configuration option takes the following
format:

    disk = [ '[format:][path],vdev[:type],attrib',
             '[format:][path],vdev[:type],attrib', ... ]

Not all attributes are required (the attributes enclosed within square
brackets above are optional) and some are deprecated.  Following is a
brief description of each of the attribute along with information on
whether or not they are mandatory.


------------------
Attribute Details
------------------


format:
------

Description:           Specifies the format of image file.
Supported values:      raw, qcow, qcow2, vhd 
Deprecated values:     None 
Mandatory:             No.  When not specified raw format is assumed.
                       For a physical block device the format must be 
                       raw and need not be explicitly specified.  For
                       an image file the format could be one of the
                       supported values and when not specified assumed
                       to be raw.
path:
----

Description:           Block  device or image file path.  For a  
                       physical block device a /dev  will be prepended 
                       when not specified and when the path doesn't 
                       start  with a '/'.  
Supported values:      N/A
Deprecated values:     N/A
Mandatory:             No.  While a path is provided in most cases
                       there is an exception.  For a cdrom device, lack
                       of this attribute would imply an empty cdrom
                       drive. 

vdev:
----

Description:           Virtual device as seen by the guest (also 
                       referred to as guest drive designation in some 
                       specifications).  [ @TODO:  Refer to IanJ's virtual
                       disk specification here.]
Supported values:      hd[x], xvd[x], sd[x] etc.  Please refer to the
                       above specification for further details.
Deprecated values:     None 
Mandatory:             Yes

type:
----

Description:           Qualifies  virtual device type.
Supported values:      cdrom
Deprecated values:     None 
Mandatory:             No

attrib:
------

Description:           Specified access control information.  Whether 
                       or not the block device is provided to the
                       guest in read-only or read-write mode depends
                       on this attribute.
Supported values:      'r', 'w'
Deprecated values:     None
Mandatory:             Yes


--------------
Example usages
--------------

disk = [ 'vhd:/path/to/dev,hda,w', '/path/to/iso,hdc:cdrom,r' ]
disk = [ 'path/to/phy/dev,hda,w', '/dev/cdrom,hdc:cdrom,r' ]
disk = [ 'qcow:/path/to/file,hda,w' ]
disk = [ 'qcow2:/path/to/file,hda,w', 'raw:/path/to/dev,hdc:cdrom,r' ]


-------------------------
Miscellaneous Information
-------------------------

---------------------
Deprecated Attributes
---------------------

Deprecated values are acceptable and will work the way they did
earlier except with a warning message printed to the xl log or screen.
However it's usage is supported purely for backward compatibility
purpose and not recommended. Also, support for these deprecated
attributes are likely to be dropped in future versions of xl.

The block-dev-type and access-type deprecated attributes described
below are prepended to the format attribute.  Example - tap:aio:qcow
Further details on the deprecated attributes are below:

block-dev-type:
--------------

Description:           Specifies the block device type.
Supported values:      phy,file, tap, tap2

access-type:
-----------

Description:           Backend implementation option to choose from 
                       while accessing block device. 
                       Example: tap:aio:vhd:/path/to/file
Supported values:      'aio', 'tapdisk', 'ioemu'

---------------------
Impementation Details
---------------------

Backend Details:
---------------

For 'phy' block device type, blkback is always used as the backend.
When the running dom0 instance does not support blkback, block device
access will fail.  For block device type 'file' with format raw or
when no format specfied, tapdisk2 is used when present otherwise qemu
fallback option is used.  For 'file', 'tap' or 'tap2' block device
type with format 'vhd', only tapdisk2 is used as qemu does not support
vhd format.  Absence of tapdisk2 support in this case will result in
failure.


When it comes to image format and how that affects the backend choice,
for qcow/qcow2 qemu backend is used as tapdisk2 does not work with
these formats. For raw format image file, tapdisk2 is used and when
not available qemu backend is used as fallback.  For vhd format, as
mentioned earlier tapdisk2 is used and tapdisk2 unavailability will
result in failure as qemu fallback option does not support vhd file
format.








[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

RE: xl: drdb support

[James Harper]

> 
> On Fri, 28 Jan 2011, Ian Jackson wrote:
> > Stefano Stabellini writes ("[Xen-devel] xl: drdb support"):
> > > On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > > > Other than hd[x] and xvd[x], are there other supported VDEV
formats?
> >
> > This is not the case.  There are numeric formats of various kinds,
> > too, and also sd[x].
> 
> sd[x] doesn't work at the moment, so I wouldn't add it to the
document.
> When (and if) it gets fixed then we can add it.

Not even as an "sd[x] doesn't work at the moment" footnote?

James

RE: drdb support

[James Harper]

> 
> On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > > - phy:/path/to/device
> > > - file:/path/to/file
> > > - tap:/path/to/file
> >
> > Along with phy, file and tap for block device types there appear to
be a
> 'drdb' option too.  Do we actively support it?  If so do we treat it
same as
> 'phy' and use blkback for backend?
> 
> Yes, I think so. Given his recent work on drdb support in qemu, maybe
> James can comment on this.
> 

Aside from the qemu problems I posted a patch for, drbd works great
under Windows. I haven't tested live migration (don't have a pair of
servers with compatible CPU's) so I don't know if that works. I also
don't allow multiple-primary as single-primary is the only locking
mechanism I have at this time to make sure the same DomU isn't started
on two machines concurrently.

James

Re: RE: drdb support / xend locking for live migration

[Pasi Kärkkäinen]

On Sat, Jan 29, 2011 at 12:12:50PM +1100, James Harper wrote:
> > 
> > On Thu, 27 Jan 2011, Kamala Narasimhan wrote:
> > > > - phy:/path/to/device
> > > > - file:/path/to/file
> > > > - tap:/path/to/file
> > >
> > > Along with phy, file and tap for block device types there appear to
> be a
> > 'drdb' option too.  Do we actively support it?  If so do we treat it
> same as
> > 'phy' and use blkback for backend?
> > 
> > Yes, I think so. Given his recent work on drdb support in qemu, maybe
> > James can comment on this.
> > 
> 
> Aside from the qemu problems I posted a patch for, drbd works great
> under Windows. I haven't tested live migration (don't have a pair of
> servers with compatible CPU's) so I don't know if that works. I also
> don't allow multiple-primary as single-primary is the only locking
> mechanism I have at this time to make sure the same DomU isn't started
> on two machines concurrently.
> 

IIRC Novell has some additional xend locking patch included
in SLES11 Xen .. I guess it was this one:
http://lists.xensource.com/archives/html/xen-devel/2008-08/msg00196.html
http://serverfault.com/questions/21699/how-to-manage-xen-virtual-machines-on-shared-san-storage

Also Oracle has some locking patch: http://lists.xensource.com/archives/html/xen-devel/2009-08/msg00101.html

-- Pasi

Re: [PATCH] xl: Special case tap/aio for disk validation

[Stefano Stabellini]

On Fri, 28 Jan 2011, Kamala Narasimhan wrote:
> I am attaching a newer version of the txt file that incorporates all the review
> comments.  Here are the only changes I am yet to make -
> 
> 1) Refer to IanJ's virtual disk specification once it is checked-in.
> 2) Include information pertaining to drbd and other arbitrary block device
> types.  This information can be included post 4.1 as I gather from the
> discussion on this topic.
> 
> Please let me know if I neglected any of your comments or if you have further
> comments.
 

I think it looks good: I would just unify the two paragraphs under
"Backend Details".