RFC: blktap3

RFC: blktap3

[Thanos Makatos]

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

Hi,

I'd like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I'll maintain it.

In this patch, blktap2 binaries are suffixed with "2", so it's not yet possible to use it along with blktap3.

An example configuration file I used is the following:
name = "debian bktap3 without pygrub"
memory = 256
disk = ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian-blktap3.vhd']
kernel = "vmlinuz-2.6.32-5-amd64"
root = '/dev/xvda1'
ramdisk = "initrd.img-2.6.32-5-amd64"
cpu_weight=256
vif=['bridge=xenbr0']

Before starting any blktap3 VM, the xenio daemon must be started.

I've tested it on change set 472fc515a463 without pygrub.

Any comments are welcome :)

--
Thanos Makatos


[-- Attachment #1.2: Type: text/html, Size: 3852 bytes --]

[-- Attachment #2: blktap3.bz2 --]
[-- Type: application/octet-stream, Size: 230728 bytes --]

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

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

Re: RFC: blktap3

[Ian Campbell]

On Thu, 2012-08-09 at 15:03 +0100, Thanos Makatos wrote:
> Hi,
> 
>  
> 
> I’d like to introduce blktap3: essentially blktap2 without the need of
> blkback. This has been developed by Santosh Jodh, and I’ll maintain
> it.
> 
>  
> 
> In this patch, blktap2 binaries are suffixed with “2”, so it’s not yet
> possible to use it along with blktap3.

I'll take a proper look at this when I get back from vacation but just a
few quick first comments:

I don't think renaming blktap2 is going to fly. For better or worse
blktap2 uses the names it uses and people (and xend) use them with those
names, so I think blktap3 needs to avoid the conflict by adding "3" as
appropriate.

I noticed that the README looks pretty blktap1 specific and references
xm etc. It would be nice to decruft the tree as part of this transition
rather than carrying across obsolete and out of date components from
blktap1 & 2 into the blktap3 stack. I bet there is other stuff which is
no longer used, e.g. the libaio-compat.h -- does the need for that still
exist or is libaio now up to date in distros (it reference 2.6.21, which
is quite a while ago now). I expect you also want to ditch
linux-blktap.h -- it looks like ioctl definitions for talking to the
(now non-existent) kernel driver. It'd be good to strip all this sort of
thing out before people start reviewing in depth -- there's not much
point in reviewing stuff which should just be removed and the patch is
big enough as is.

On a similar note if there are plugin modules which are not often used
in normal configurations perhaps they could be omitted from the initial
upstreaming? (I don't know what all of block-* etc are,  but maybe some
of them are not really used in practice?)


Ian.


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

Re: RFC: blktap3

[Konrad Rzeszutek Wilk]

On Thu, Aug 09, 2012 at 03:03:06PM +0100, Thanos Makatos wrote:
> Hi,
> 
> I'd like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I'll maintain it.
> 

So where is the source of this driver located?
> In this patch, blktap2 binaries are suffixed with "2", so it's not yet possible to use it along with blktap3.
> 
> An example configuration file I used is the following:
> name = "debian bktap3 without pygrub"
> memory = 256
> disk = ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian-blktap3.vhd']
> kernel = "vmlinuz-2.6.32-5-amd64"
> root = '/dev/xvda1'
> ramdisk = "initrd.img-2.6.32-5-amd64"
> cpu_weight=256
> vif=['bridge=xenbr0']
> 
> Before starting any blktap3 VM, the xenio daemon must be started.
> 
> I've tested it on change set 472fc515a463 without pygrub.
> 
> Any comments are welcome :)
> 
> --
> Thanos Makatos
> 


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

Re: RFC: blktap3

[Ian Campbell]

On Thu, 2012-08-09 at 19:04 +0100, Konrad Rzeszutek Wilk wrote:
> On Thu, Aug 09, 2012 at 03:03:06PM +0100, Thanos Makatos wrote:
> > Hi,
> > 
> > I'd like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I'll maintain it.
> > 
> 
> So where is the source of this driver located?

I suspect that Thanos meant the blktap kernel driver rather than
blkback.

This is a completely userspace implementation of blktap, the fact that
you don't need to layer blkback on top of it is largely incidental.

This certainly doesn't aim to replace blkback, they both serve different
use cases (essentially speed vs flexibility).

Ian.

Re: RFC: blktap3

[Thanos Makatos]

Hi Konrad,

I'm not sure I understand your question. Blktap3 lives in tools/blktap3. The component that allows tapdisk3 to talk directly to blkfront is xenio, it lives in blktap3/tools/xenio. Since tapdisk3 can talk to blkfront via xenio, it doesn't interact with blkback/blktap kernel drivers.

-----Original Message-----
From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] 
Sent: 09 August 2012 19:05
To: Thanos Makatos
Cc: xen-devel@lists.xen.org
Subject: Re: [Xen-devel] RFC: blktap3

On Thu, Aug 09, 2012 at 03:03:06PM +0100, Thanos Makatos wrote:
> Hi,
> 
> I'd like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I'll maintain it.
> 

So where is the source of this driver located?
> In this patch, blktap2 binaries are suffixed with "2", so it's not yet possible to use it along with blktap3.
> 
> An example configuration file I used is the following:
> name = "debian bktap3 without pygrub"
> memory = 256
> disk = ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian-blktap3.vhd']
> kernel = "vmlinuz-2.6.32-5-amd64"
> root = '/dev/xvda1'
> ramdisk = "initrd.img-2.6.32-5-amd64"
> cpu_weight=256
> vif=['bridge=xenbr0']
> 
> Before starting any blktap3 VM, the xenio daemon must be started.
> 
> I've tested it on change set 472fc515a463 without pygrub.
> 
> Any comments are welcome :)
> 
> --
> Thanos Makatos
> 


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

Re: RFC: blktap3

[Stefano Stabellini]

On Fri, 10 Aug 2012, Thanos Makatos wrote:
> Hi Konrad,
> 
> I'm not sure I understand your question. Blktap3 lives in tools/blktap3. The component that allows tapdisk3 to talk directly to blkfront is xenio, it lives in blktap3/tools/xenio. Since tapdisk3 can talk to blkfront via xenio, it doesn't interact with blkback/blktap kernel drivers.

Konrad, Blktap3 is purely userspace ;-)

Re: RFC: blktap3

[Konrad Rzeszutek Wilk]

On Fri, Aug 10, 2012 at 12:17:10PM +0100, Stefano Stabellini wrote:
> On Fri, 10 Aug 2012, Thanos Makatos wrote:
> > Hi Konrad,
> > 
> > I'm not sure I understand your question. Blktap3 lives in tools/blktap3. The component that allows tapdisk3 to talk directly to blkfront is xenio, it lives in blktap3/tools/xenio. Since tapdisk3 can talk to blkfront via xenio, it doesn't interact with blkback/blktap kernel drivers.
> 
> Konrad, Blktap3 is purely userspace ;-)

I somehow managed to miss the blktap3.bz2 file in the original thread and then
started looking for a git tree or hg tree and couldn't find it. So the answer
to my question is:  http://lists.xen.org/archives/html/xen-devel/2012-08/binRhvyhiESWY.bin

:-)

Re: RFC: blktap3

[Goncalo Gomes]

On Thu, 09 Aug 2012, Thanos Makatos wrote:

> Hi,
> 
> I’d like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I’ll maintain it.

Welcome! precisely, xenio (aka blktap3) was developed by Daniel 
Stodden and recently continued/improved by Santosh Jodh :-)

As I have a slight interest in this area, I was wondering what are the 
main improvements over blktap2?

Goncalo

> In this patch, blktap2 binaries are suffixed with “2”, so it’s not yet possible to use it along with blktap3.
> 
> An example configuration file I used is the following:
> name = "debian bktap3 without pygrub"
> memory = 256
> disk = ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian-blktap3.vhd']
> kernel = "vmlinuz-2.6.32-5-amd64"
> root = '/dev/xvda1'
> ramdisk = "initrd.img-2.6.32-5-amd64"
> cpu_weight=256
> vif=['bridge=xenbr0']
> 
> Before starting any blktap3 VM, the xenio daemon must be started.
> 
> I’ve tested it on change set 472fc515a463 without pygrub.
> 
> Any comments are welcome :)
> 
> --
> Thanos Makatos
> 


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


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

Re: RFC: blktap3

[Thanos Makatos]

I haven't yet looked at the code in depth, but I suppose blktap3 can, in some cases, be faster than blktap2 since the dom0 kernel doesn't participate in the data path. At least that's what I'd expect
;-).

-----Original Message-----
From: Goncalo Gomes 
Sent: 09 August 2012 22:40
To: Thanos Makatos
Cc: xen-devel@lists.xen.org
Subject: Re: [Xen-devel] RFC: blktap3

On Thu, 09 Aug 2012, Thanos Makatos wrote:

> Hi,
> 
> I’d like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I’ll maintain it.

Welcome! precisely, xenio (aka blktap3) was developed by Daniel Stodden and recently continued/improved by Santosh Jodh :-)

As I have a slight interest in this area, I was wondering what are the main improvements over blktap2?

Goncalo

> In this patch, blktap2 binaries are suffixed with “2”, so it’s not yet possible to use it along with blktap3.
> 
> An example configuration file I used is the following:
> name = "debian bktap3 without pygrub"
> memory = 256
> disk = 
> ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian
> -blktap3.vhd']
> kernel = "vmlinuz-2.6.32-5-amd64"
> root = '/dev/xvda1'
> ramdisk = "initrd.img-2.6.32-5-amd64"
> cpu_weight=256
> vif=['bridge=xenbr0']
> 
> Before starting any blktap3 VM, the xenio daemon must be started.
> 
> I’ve tested it on change set 472fc515a463 without pygrub.
> 
> Any comments are welcome :)
> 
> --
> Thanos Makatos
> 


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

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

Re: RFC: blktap3

[Jan Beulich]

>>> On 10.08.12 at 13:06, Thanos Makatos <thanos.makatos@citrix.com> wrote:
> I haven't yet looked at the code in depth,

How come you post this not insignificant amount of code then?
You ought to be able to address review comments...

Jan

> but I suppose blktap3 can, in some 
> cases, be faster than blktap2 since the dom0 kernel doesn't participate in 
> the data path. At least that's what I'd expect
> ;-).

Re: RFC: blktap3

[Thanos Makatos]

This code was not implemented by me, it was handed over to me in order to release it and maintain it. I would really like to have the time to read and understand it to be able to address review comments as you said, but my allocated time for blktap3 isn't enough for this. My big problem is that code in libxl keeps evolving and renders blktap3 code out of sync, and it's a pain to rebase it. So, I wanted to see if it was possible to incorporate to xen-unstable ASAP, avoiding the rebase effort. I'll be now dividing the code into pieces and send it in individual patches, so I believe I'll get a better understanding of the code during this process.

-----Original Message-----
From: Jan Beulich [mailto:JBeulich@suse.com] 
Sent: 10 August 2012 14:12
To: Thanos Makatos
Cc: Goncalo Gomes; xen-devel@lists.xen.org
Subject: Re: [Xen-devel] RFC: blktap3

>>> On 10.08.12 at 13:06, Thanos Makatos <thanos.makatos@citrix.com> wrote:
> I haven't yet looked at the code in depth,

How come you post this not insignificant amount of code then?
You ought to be able to address review comments...

Jan

> but I suppose blktap3 can, in some
> cases, be faster than blktap2 since the dom0 kernel doesn't 
> participate in the data path. At least that's what I'd expect ;-).

Re: RFC: blktap3

[Thanos Makatos]

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

I got some (internal) feedback: the best thing to do is to break blktap3 in individual patches to make it easier for people to review it, which makes perfect sense. Also, it's been suggested that documenting it would hel. I'll start by sending patches that affect the existing code in order to minimize rebasing.

From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Thanos Makatos
Sent: 09 August 2012 15:03
To: xen-devel@lists.xen.org
Subject: [Xen-devel] RFC: blktap3

Hi,

I'd like to introduce blktap3: essentially blktap2 without the need of blkback. This has been developed by Santosh Jodh, and I'll maintain it.

In this patch, blktap2 binaries are suffixed with "2", so it's not yet possible to use it along with blktap3.

An example configuration file I used is the following:
name = "debian bktap3 without pygrub"
memory = 256
disk = ['backendtype=xenio,format=vhd,vdev=xvda,access=rw,target=/root/debian-blktap3.vhd']
kernel = "vmlinuz-2.6.32-5-amd64"
root = '/dev/xvda1'
ramdisk = "initrd.img-2.6.32-5-amd64"
cpu_weight=256
vif=['bridge=xenbr0']

Before starting any blktap3 VM, the xenio daemon must be started.

I've tested it on change set 472fc515a463 without pygrub.

Any comments are welcome :)

--
Thanos Makatos


[-- Attachment #1.2: Type: text/html, Size: 5088 bytes --]

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

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

Re: RFC: blktap3

[Ian Campbell]

On Thu, 2012-08-09 at 15:03 +0100, Thanos Makatos wrote:
> I’d like to introduce blktap3: essentially blktap2 without the need of
> blkback. This has been developed by Santosh Jodh, and I’ll maintain
> it.

I think you are working on reposting in a more manageable form but
here's a few things which I noticed on a top level scroll though. (I
might be repeating myself occasionally from the quick comments I made
earlier, sorry)

> diff --git a/tools/Makefile b/tools/Makefile
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -201,3 +203,20 @@
>  
>  subdir-distclean-firmware: .phony
>  	$(MAKE) -C firmware distclean
> +
> +subdir-all-blktap3 subdir-install-blktap3: .phony
> +	source=.; \
> +	cd blktap3; \
> +	./autogen.sh; \

If anything this should be called from the top-level ./autogen.sh and
not here. We shouldn't expect end users to have autoconf available.

> +	./configure \

I think autoconf has a construct which can cause configure to call other
sub-configures in subdirs. If I'm right then it would be better to use
this instead of calling it here.

However I think that the real correct answer is that blktap3 shouldn't
have it's own configure anyway but should simply add the tests which it
needs to the global tools level one and use the result like everyone
else.

> +	CFLAGS="-I$(XEN_ROOT)/tools/include \
> +		-I$(XEN_ROOT)/tools/libxc \
> +		-I$(XEN_ROOT)/tools/xenstore" \
> +	LDFLAGS="-L$(XEN_ROOT)/tools/xenstore \
> +		 -L$(XEN_ROOT)/tools/libxc"; \

Your Makefiles should start with

        XEN_ROOT = $(CURDIR)/../..
        include $(XEN_ROOT)/tools/Rules.mk

And then make use of the variables defined in Rules.mk. e.g.
CFLAGS_libxenctrl, LIBS_libxenctrl etc rather than doing this.

I suppose blktap3 once lived outside of the xen tree and this (and the
configurey) is a hangover from that. But we should clean it up on its
way into the tree

> diff --git a/tools/blktap2/drivers/Makefile b/tools/blktap2/drivers/Makefile
> --- a/tools/blktap2/drivers/Makefile
> +++ b/tools/blktap2/drivers/Makefile
> @@ -4,9 +4,9 @@
>  
>  LIBVHDDIR  = $(BLKTAP_ROOT)/vhd/lib
>  
> -IBIN       = tapdisk2 td-util tapdisk-client tapdisk-stream tapdisk-diff
> -QCOW_UTIL  = img2qcow qcow-create qcow2raw
> -LOCK_UTIL  = lock-util
> +IBIN       = tapdisk2 td-util2 tapdisk-client2 tapdisk-stream2 tapdisk-diff2
> +QCOW_UTIL  = img2qcow2 qcow-create2 qcow2raw2
> +LOCK_UTIL  = lock-util2

This series shouldn't be renaming bits of blktap2. In fact I think as a
general rule it should not be touching tools/blktap2 at all. If it does
it should be in a separate patch I think.

> diff --git a/tools/blktap3/Makefile.am b/tools/blktap3/Makefile.am
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/Makefile.am

This is adding a new dependency on automake which is something we'll
have to discuss.

As part of the initial push I think it would be less controversial to
simply use the existing Xen tools build infrastructure (such as it is).
I think the majority of this could be cribbed petty directly from
blktap2 and other parts of the tools tree.

> diff --git a/tools/blktap3/README b/tools/blktap3/README
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/README

I think I mentioned this before but it looks like this document could do
with a pretty hefty update.

> diff --git a/tools/blktap3/control/tap-ctl-attach.c b/tools/blktap3/control/tap-ctl-attach.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/control/tap-ctl-attach.c
> @@ -0,0 +1,66 @@
> +/*
> + * Copyright (c) 2008, XenSource Inc.

You probably want to do an update of all these copyright headers.


> + * All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions are met:
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in the
> + *       documentation and/or other materials provided with the distribution.
> + *     * Neither the name of XenSource Inc. nor the names of its contributors

And I suppose this ought to be updated too.

> + *       may be used to endorse or promote products derived from this software
> + *       without specific prior written permission.


The actual three clause BSD says "The name of the author may not be used
to endorse or promote products derived from this software without
specific prior written permission.

This weird variant of the 3-clause BSD is something you might want to
discuss with your management to see if it can't be rationalised.

> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER
> + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
> + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
> + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
> + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF
> + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
> + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */

I think it would be worthwhile to have a tools/blktap3/COPYING file to
clarify the licesing terms of blktap3 as a whole.

[...] I didn't look at the majority of the actual tools/blktap3 code.
There's quite a lot of it. I mentioned earlier that you might want to
consider dropping some of the optional components for the time being to
keep the initial upstreaming more manageable.

> diff --git a/tools/blktap3/drivers/td-rated.1.txt b/tools/blktap3/drivers/td-rated.1.txt
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/drivers/td-rated.1.txt

Is this a generated file? I didn't see the source but it'd be nice to
have e.g. the actual man page etc.

This made me grep for "doc", "man" and "txt" in the patch, which only
found this one file. Hopefully I just missed it all, or at least can we
expect that additional docs will be forthcoming in the future?


> diff --git a/tools/blktap3/include/blktap2.h b/tools/blktap3/include/blktap2.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/include/blktap2.h

s/2/3/ Or does this file belong at all? It seems to mostly relate to the
blktap2 kernel driver ioctl interface. Please can you kill all this
cruft before reposting.

> diff --git a/tools/blktap3/include/list.h b/tools/blktap3/include/list.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/include/list.h
> @@ -0,0 +1,149 @@
> +/*
> + * list.h
> + * 
> + * This is a subset of linux's list.h intended to be used in user-space.
> + * 
> + */

If this came from Linux then it is GPL licensed and must have a GPL
header on it.

The intention seems to be that blktap3 is BSD but this would make it
overall GPL. You could either relicense the whole thing as (L)GPL or
perhaps reimplement using the BSD licensed list macros (see
tools/include/xen-external for the BSD macros which libxl and mini-os
use)


> diff --git a/tools/blktap3/xenio/blkif.h b/tools/blktap3/xenio/blkif.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/xenio/blkif.h

Given that this is in-tree you might perhaps want to use the in-three
interface declarations from tools/include.

> diff --git a/tools/blktap3/xenio/list.h b/tools/blktap3/xenio/list.h
> new file mode 100644
> --- /dev/null
> +++ b/tools/blktap3/xenio/list.h
> @@ -0,0 +1,134 @@
> +/*
> + * list.h
> + * 
> + * This is a subset of linux's list.h intended to be used in user-space.
> + * 
> + */

Another duplicated copy of some GPL code.

Apart from the licensing things perhaps you could rationalise the number
of copies of things like this which you are introducing?


> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> --- a/tools/libxl/libxl.c
> +++ b/tools/libxl/libxl.c
> @@ -1171,6 +1171,8 @@

Can you add the following to your ~/.hgrc please:
        [diff]
        showfunc = True

This will inject the current function name into the hunk header which
makes review much easier.

>          disk->backend = LIBXL_DISK_BACKEND_TAP;
>      } else if (!strcmp(backend_type, "qdisk")) {
>          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> +    } else if (!strcmp(backend_type, "xenio")) {
> +        disk->backend = LIBXL_DISK_BACKEND_XENIO;

I think you want to replace LIBXL_DISK_BACKEND_TAP rather than add a new
one. You could also steal the name if you like I reckon.

> 
>      } else {
>          disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
>      }


> @@ -1961,6 +1981,7 @@
>  }
>  
>  static void libxl__device_disk_from_xs_be(libxl__gc *gc,
> +                                          xs_transaction_t t,
>                                            const char *be_path,
>                                            libxl_device_disk *disk)
>  {

This sort of thing should be done as a separate pre-cursor patch.


> diff --git a/tools/libxl/libxl_tapdisk.c b/tools/libxl/libxl_tapdisk.c
> new file mode 100644
> --- /dev/null
> +++ b/tools/libxl/libxl_tapdisk.c

Is this actually a move of of the existing ibxl_blktap? I think "hg diff
-g" will cause it to use git style patches which make this clearer.

Although I don't see libxl_blktap getting removed, so perhaps not? I
thought I saw you changing the Makefile as if you were renamng as well.

Renaming should generally be done as a standalone patch with no
non-related changes in them, to make them eaiser to review.

> @@ -0,0 +1,162 @@
[...]
> +        struct list_head list;
> +	tap_list_t *entry, *next_t;

Something odd with whitespace here.

> +        int ret = -ENOENT, err;
> +
> +	fprintf(stderr, "blktap_find(%s:%s)\n", type, path);

Please drop this sort of debug.

> +        INIT_LIST_HEAD(&list);
> +        err = tap_ctl_list(&list);
> +        if (err < 0)
> +                return err;
> [...]
> +//        tap_ctl_list_free(&list);

Leak?


> char *libxl__blktap_devpath(libxl__gc *gc,
> +                            const char *disk,
> +                            libxl_disk_format format)
> +{
> +    const char *type;
> +    char *params, *devname = NULL;
> +    tap_list_t tap;
> +    int err;
> +
> +    type = libxl__device_disk_string_of_format(format);
> +    fprintf(stderr, "libxl__blktap_devpath(%s:%s)\n", disk, type);
> +    err = blktap_find(type, disk, &tap);
> +    if (err == 0) {
> +        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d", tap.minor);

Surely not any more?

> +        if (devname)
> +            return devname;
> +    }
> +
> +    params = libxl__sprintf(gc, "%s:%s", type, disk);
> +    err = tap_ctl_create(params, &devname, 0, -1, NULL);
> +    if (!err) {
> +        libxl__ptr_add(gc, devname);
> +        return devname;
> +    }
> +
> +    return NULL;
> +}

[...]
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -1862,7 +1862,7 @@
>  
>          child1 = xl_fork(child_waitdaemon);
>          if (child1) {
> -            printf("Daemon running with PID %d\n", child1);
> +            printf("Daemon running with PID %d for domain %d\n", child1, domid);

This is probably a useful change but it has nothing at all to do with
blktap3, please separate all this sort of stuff out.

>  
>              for (;;) {
>                  got_child = xl_waitpid(child_waitdaemon, &status, 0);


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

Re: RFC: blktap3

[Thanos Makatos]

Thanks for your comments, Ian, I'll address them before reposting.

To start with, you say that I should replace LIBXL_DISK_BACKEND_TAP:
> >          disk->backend = LIBXL_DISK_BACKEND_TAP;
> >      } else if (!strcmp(backend_type, "qdisk")) {
> >          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> > +    } else if (!strcmp(backend_type, "xenio")) {
> > +        disk->backend = LIBXL_DISK_BACKEND_XENIO;
> 
> I think you want to replace LIBXL_DISK_BACKEND_TAP rather than add a
> new one. You could also steal the name if you like I reckon.
But in tools/libxl/libxl.c:1876, libxl__blktap_devpath is called which seems blktap2 dependant, so we need a new backend type to be able to use blktap2 along with blktap3, no?

> -----Original Message-----
> From: Ian Campbell
> Sent: 16 August 2012 17:10
> To: Thanos Makatos
> Cc: xen-devel@lists.xen.org
> Subject: Re: [Xen-devel] RFC: blktap3
> 
> On Thu, 2012-08-09 at 15:03 +0100, Thanos Makatos wrote:
> > I’d like to introduce blktap3: essentially blktap2 without the need
> of
> > blkback. This has been developed by Santosh Jodh, and I’ll maintain
> > it.
> 
> I think you are working on reposting in a more manageable form but
> here's a few things which I noticed on a top level scroll though. (I
> might be repeating myself occasionally from the quick comments I made
> earlier, sorry)
> 
> > diff --git a/tools/Makefile b/tools/Makefile
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -201,3 +203,20 @@
> >
> >  subdir-distclean-firmware: .phony
> >  	$(MAKE) -C firmware distclean
> > +
> > +subdir-all-blktap3 subdir-install-blktap3: .phony
> > +	source=.; \
> > +	cd blktap3; \
> > +	./autogen.sh; \
> 
> If anything this should be called from the top-level ./autogen.sh and
> not here. We shouldn't expect end users to have autoconf available.
> 
> > +	./configure \
> 
> I think autoconf has a construct which can cause configure to call
> other sub-configures in subdirs. If I'm right then it would be better
> to use this instead of calling it here.
> 
> However I think that the real correct answer is that blktap3 shouldn't
> have it's own configure anyway but should simply add the tests which it
> needs to the global tools level one and use the result like everyone
> else.
> 
> > +	CFLAGS="-I$(XEN_ROOT)/tools/include \
> > +		-I$(XEN_ROOT)/tools/libxc \
> > +		-I$(XEN_ROOT)/tools/xenstore" \
> > +	LDFLAGS="-L$(XEN_ROOT)/tools/xenstore \
> > +		 -L$(XEN_ROOT)/tools/libxc"; \
> 
> Your Makefiles should start with
> 
>         XEN_ROOT = $(CURDIR)/../..
>         include $(XEN_ROOT)/tools/Rules.mk
> 
> And then make use of the variables defined in Rules.mk. e.g.
> CFLAGS_libxenctrl, LIBS_libxenctrl etc rather than doing this.
> 
> I suppose blktap3 once lived outside of the xen tree and this (and the
> configurey) is a hangover from that. But we should clean it up on its
> way into the tree
> 
> > diff --git a/tools/blktap2/drivers/Makefile
> > b/tools/blktap2/drivers/Makefile
> > --- a/tools/blktap2/drivers/Makefile
> > +++ b/tools/blktap2/drivers/Makefile
> > @@ -4,9 +4,9 @@
> >
> >  LIBVHDDIR  = $(BLKTAP_ROOT)/vhd/lib
> >
> > -IBIN       = tapdisk2 td-util tapdisk-client tapdisk-stream tapdisk-
> diff
> > -QCOW_UTIL  = img2qcow qcow-create qcow2raw -LOCK_UTIL  = lock-util
> > +IBIN       = tapdisk2 td-util2 tapdisk-client2 tapdisk-stream2
> tapdisk-diff2
> > +QCOW_UTIL  = img2qcow2 qcow-create2 qcow2raw2 LOCK_UTIL  = lock-
> util2
> 
> This series shouldn't be renaming bits of blktap2. In fact I think as a
> general rule it should not be touching tools/blktap2 at all. If it does
> it should be in a separate patch I think.
> 
> > diff --git a/tools/blktap3/Makefile.am b/tools/blktap3/Makefile.am
> new
> > file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/Makefile.am
> 
> This is adding a new dependency on automake which is something we'll
> have to discuss.
> 
> As part of the initial push I think it would be less controversial to
> simply use the existing Xen tools build infrastructure (such as it is).
> I think the majority of this could be cribbed petty directly from
> blktap2 and other parts of the tools tree.
> 
> > diff --git a/tools/blktap3/README b/tools/blktap3/README new file
> mode
> > 100644
> > --- /dev/null
> > +++ b/tools/blktap3/README
> 
> I think I mentioned this before but it looks like this document could
> do with a pretty hefty update.
> 
> > diff --git a/tools/blktap3/control/tap-ctl-attach.c
> > b/tools/blktap3/control/tap-ctl-attach.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/control/tap-ctl-attach.c
> > @@ -0,0 +1,66 @@
> > +/*
> > + * Copyright (c) 2008, XenSource Inc.
> 
> You probably want to do an update of all these copyright headers.
> 
> 
> > + * All rights reserved.
> > + *
> > + * Redistribution and use in source and binary forms, with or
> without
> > + * modification, are permitted provided that the following
> conditions are met:
> > + *     * Redistributions of source code must retain the above
> copyright
> > + *       notice, this list of conditions and the following
> disclaimer.
> > + *     * Redistributions in binary form must reproduce the above
> copyright
> > + *       notice, this list of conditions and the following
> disclaimer in the
> > + *       documentation and/or other materials provided with the
> distribution.
> > + *     * Neither the name of XenSource Inc. nor the names of its
> contributors
> 
> And I suppose this ought to be updated too.
> 
> > + *       may be used to endorse or promote products derived from
> this software
> > + *       without specific prior written permission.
> 
> 
> The actual three clause BSD says "The name of the author may not be
> used to endorse or promote products derived from this software without
> specific prior written permission.
> 
> This weird variant of the 3-clause BSD is something you might want to
> discuss with your management to see if it can't be rationalised.
> 
> > + *
> > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
> > + CONTRIBUTORS
> > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> > + FOR
> > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> > + COPYRIGHT OWNER
> > + * OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> > + SPECIAL,
> > + * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED
> > + TO,
> > + * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA,
> OR
> > + * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> > + THEORY OF
> > + * LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> > + (INCLUDING
> > + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> THIS
> > + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > + */
> 
> I think it would be worthwhile to have a tools/blktap3/COPYING file to
> clarify the licesing terms of blktap3 as a whole.
> 
> [...] I didn't look at the majority of the actual tools/blktap3 code.
> There's quite a lot of it. I mentioned earlier that you might want to
> consider dropping some of the optional components for the time being to
> keep the initial upstreaming more manageable.
> 
> > diff --git a/tools/blktap3/drivers/td-rated.1.txt
> > b/tools/blktap3/drivers/td-rated.1.txt
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/drivers/td-rated.1.txt
> 
> Is this a generated file? I didn't see the source but it'd be nice to
> have e.g. the actual man page etc.
> 
> This made me grep for "doc", "man" and "txt" in the patch, which only
> found this one file. Hopefully I just missed it all, or at least can we
> expect that additional docs will be forthcoming in the future?
> 
> 
> > diff --git a/tools/blktap3/include/blktap2.h
> > b/tools/blktap3/include/blktap2.h new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/include/blktap2.h
> 
> s/2/3/ Or does this file belong at all? It seems to mostly relate to
> the
> blktap2 kernel driver ioctl interface. Please can you kill all this
> cruft before reposting.
> 
> > diff --git a/tools/blktap3/include/list.h
> > b/tools/blktap3/include/list.h new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/include/list.h
> > @@ -0,0 +1,149 @@
> > +/*
> > + * list.h
> > + *
> > + * This is a subset of linux's list.h intended to be used in user-
> space.
> > + *
> > + */
> 
> If this came from Linux then it is GPL licensed and must have a GPL
> header on it.
> 
> The intention seems to be that blktap3 is BSD but this would make it
> overall GPL. You could either relicense the whole thing as (L)GPL or
> perhaps reimplement using the BSD licensed list macros (see
> tools/include/xen-external for the BSD macros which libxl and mini-os
> use)
> 
> 
> > diff --git a/tools/blktap3/xenio/blkif.h
> b/tools/blktap3/xenio/blkif.h
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/xenio/blkif.h
> 
> Given that this is in-tree you might perhaps want to use the in-three
> interface declarations from tools/include.
> 
> > diff --git a/tools/blktap3/xenio/list.h b/tools/blktap3/xenio/list.h
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/blktap3/xenio/list.h
> > @@ -0,0 +1,134 @@
> > +/*
> > + * list.h
> > + *
> > + * This is a subset of linux's list.h intended to be used in user-
> space.
> > + *
> > + */
> 
> Another duplicated copy of some GPL code.
> 
> Apart from the licensing things perhaps you could rationalise the
> number of copies of things like this which you are introducing?
> 
> 
> > diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
> > --- a/tools/libxl/libxl.c
> > +++ b/tools/libxl/libxl.c
> > @@ -1171,6 +1171,8 @@
> 
> Can you add the following to your ~/.hgrc please:
>         [diff]
>         showfunc = True
> 
> This will inject the current function name into the hunk header which
> makes review much easier.
> 
> >          disk->backend = LIBXL_DISK_BACKEND_TAP;
> >      } else if (!strcmp(backend_type, "qdisk")) {
> >          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> > +    } else if (!strcmp(backend_type, "xenio")) {
> > +        disk->backend = LIBXL_DISK_BACKEND_XENIO;
> 
> I think you want to replace LIBXL_DISK_BACKEND_TAP rather than add a
> new one. You could also steal the name if you like I reckon.
> 
> >
> >      } else {
> >          disk->backend = LIBXL_DISK_BACKEND_UNKNOWN;
> >      }
> 
> 
> > @@ -1961,6 +1981,7 @@
> >  }
> >
> >  static void libxl__device_disk_from_xs_be(libxl__gc *gc,
> > +                                          xs_transaction_t t,
> >                                            const char *be_path,
> >                                            libxl_device_disk *disk)
> {
> 
> This sort of thing should be done as a separate pre-cursor patch.
> 
> 
> > diff --git a/tools/libxl/libxl_tapdisk.c
> b/tools/libxl/libxl_tapdisk.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/tools/libxl/libxl_tapdisk.c
> 
> Is this actually a move of of the existing ibxl_blktap? I think "hg
> diff -g" will cause it to use git style patches which make this
> clearer.
> 
> Although I don't see libxl_blktap getting removed, so perhaps not? I
> thought I saw you changing the Makefile as if you were renamng as well.
> 
> Renaming should generally be done as a standalone patch with no non-
> related changes in them, to make them eaiser to review.
> 
> > @@ -0,0 +1,162 @@
> [...]
> > +        struct list_head list;
> > +	tap_list_t *entry, *next_t;
> 
> Something odd with whitespace here.
> 
> > +        int ret = -ENOENT, err;
> > +
> > +	fprintf(stderr, "blktap_find(%s:%s)\n", type, path);
> 
> Please drop this sort of debug.
> 
> > +        INIT_LIST_HEAD(&list);
> > +        err = tap_ctl_list(&list);
> > +        if (err < 0)
> > +                return err;
> > [...]
> > +//        tap_ctl_list_free(&list);
> 
> Leak?
> 
> 
> > char *libxl__blktap_devpath(libxl__gc *gc,
> > +                            const char *disk,
> > +                            libxl_disk_format format) {
> > +    const char *type;
> > +    char *params, *devname = NULL;
> > +    tap_list_t tap;
> > +    int err;
> > +
> > +    type = libxl__device_disk_string_of_format(format);
> > +    fprintf(stderr, "libxl__blktap_devpath(%s:%s)\n", disk, type);
> > +    err = blktap_find(type, disk, &tap);
> > +    if (err == 0) {
> > +        devname = libxl__sprintf(gc, "/dev/xen/blktap-2/tapdev%d",
> > + tap.minor);
> 
> Surely not any more?
> 
> > +        if (devname)
> > +            return devname;
> > +    }
> > +
> > +    params = libxl__sprintf(gc, "%s:%s", type, disk);
> > +    err = tap_ctl_create(params, &devname, 0, -1, NULL);
> > +    if (!err) {
> > +        libxl__ptr_add(gc, devname);
> > +        return devname;
> > +    }
> > +
> > +    return NULL;
> > +}
> 
> [...]
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -1862,7 +1862,7 @@
> >
> >          child1 = xl_fork(child_waitdaemon);
> >          if (child1) {
> > -            printf("Daemon running with PID %d\n", child1);
> > +            printf("Daemon running with PID %d for domain %d\n",
> > + child1, domid);
> 
> This is probably a useful change but it has nothing at all to do with
> blktap3, please separate all this sort of stuff out.
> 
> >
> >              for (;;) {
> >                  got_child = xl_waitpid(child_waitdaemon, &status,
> 0);

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

Re: RFC: blktap3

[Ian Campbell]

On Thu, 2012-08-30 at 16:41 +0100, Thanos Makatos wrote:
> Thanks for your comments, Ian, I'll address them before reposting.
> 
> To start with, you say that I should replace LIBXL_DISK_BACKEND_TAP:
> > >          disk->backend = LIBXL_DISK_BACKEND_TAP;
> > >      } else if (!strcmp(backend_type, "qdisk")) {
> > >          disk->backend = LIBXL_DISK_BACKEND_QDISK;
> > > +    } else if (!strcmp(backend_type, "xenio")) {
> > > +        disk->backend = LIBXL_DISK_BACKEND_XENIO;
> >
> > I think you want to replace LIBXL_DISK_BACKEND_TAP rather than add a
> > new one. You could also steal the name if you like I reckon.
> But in tools/libxl/libxl.c:1876, libxl__blktap_devpath is called which
> seems blktap2 dependant, so we need a new backend type to be able to
> use blktap2 along with blktap3, no?

You can remove all the blktap2 support from libxl IMHO. I don't think
there is any need to support both in parallel in (lib)xl, especially
given that blktap2 is basically unmaintained.

I'm curious what other people think though.

We should leave blktap2 in the tree for the time being because xend
uses. Once we deprecate and remove xend we can clear that up too. In the
meantime you should leave the names of the blktap2 stuff alone etc.

> > -----Original Message-----
[... snip hundreds of lines of unnecessary quoted material, please trim
your quotes ...]

Ian.