Re: [Xen-devel] [PATCH 01/10] blktap: Add include/linux/blktap.h

[Daniel Stodden <daniel.stodden@xxxxxxxxxx>]

On Thu, 2011-03-10 at 03:28 -0500, Ian Campbell wrote:
> On Wed, 2011-03-09 at 22:37 +0000, Daniel Stodden wrote:
> > On Wed, 2011-03-09 at 05:18 -0500, Ian Campbell wrote:
> > > On Wed, 2011-03-09 at 00:42 +0000, Daniel Stodden wrote:
> > > > Moves blktap2 definitions into a common header file.
> > > >
> > > > Includes xen/interface/io/ring.h and new ring definitions. Makes
> > > > blktap build independently from xen-devel headers.
> > > >
> > > > New blktap_ring structs are fully congrent to blkif rings, for binary
> > > > compat.
> > > >
> > > > Signed-off-by: Daniel Stodden <daniel.stodden@xxxxxxxxxx>
> > > > ---
> > > >  drivers/xen/blktap/blktap.h  |   66 ++++----------------------------
> > > >  drivers/xen/blktap/control.c |   14 +++---
> > > >  drivers/xen/blktap/device.c  |   12 +++---
> > > >  drivers/xen/blktap/request.c |    8 ++--
> > > >  drivers/xen/blktap/ring.c    |   51 ++++++++++++++-----------
> > > >  drivers/xen/blktap/sysfs.c   |    6 +-
> > > >  include/linux/blktap.h       |   85 
> > > > ++++++++++++++++++++++++++++++++++++++++++
> > >
> > > This new file defines the kernel<->user (tapdisk process) ring protocol,
> > > right?
> >
> > Yes. It's exactly as far as I can go right now maintaining
> > compatibility. The main objective was rather to get off xen-devel
> > headers in favour of kernel sources.
> >
> > - includes xen/interface/io/ring.
> > - doesn't include xen/interface/io/blkif.
> > - certainly doesn't include xen/interface/blkif.h
> >   (the alignment stuff for guests).
> >
> > The old code used blkif and struct blkif_* definitions. The new one got
> > it's own struct blktap_*s, identical as far as READ/WRITE commands go.
> >
> > But this also means one can develop the userland stuff independently
> > from blkif.h. New commands (flush, trim, ...) get quite a bit more
> > useful freedom.
> >
> > > I think its proper home would be under include/xen somewhere, which is
> > > where the gntdev and evtchn etc driver interfaces are defined.
> >
> > A very long time ago, a somewhat obvious choice was made to use xen ring
> > headers for the blktap user <-> kernel interface. So this header
> > presently still wants xen/interface.
> >
> > It doesn't depend on anything xenish, nor is this a Xen driver anymore.
> > I thought even with that header dependency, that's somewhat a
> > linux/blktap.h already, so I made it so.
> 
> OK.
> 
> > I'm feeling some heat from boston-newxen people because in XCP I'm
> > actually building blktap.hg against the kernel devel rpm contents right
> > now. That's got to vanish. It's great for hacking extensions, but the
> > component dependency is a bit gross, admittedly.
> 
> Sure, but you could build against a copy of the kernel source tree,
> which would remove the dependency on the kernel binary RPMs.
> 
> > Once doing so, it's a standalone kernel blktap.h which can be copied
> > over into userland trees, without additional definitions included.
> 
> If the other user of this interface is the tapdisk userspace, but that
> includes a copy of the interface header (note: I'm not convinced that is
> a good idea) then I think the right place for this copy of the header is
> drivers/block/blktap/.
> 
> If on the other hand userland is building against this exact header then
> include/linux is probably right given that the driver has no Xen
> dependency.

If include/linux is acceptable, good, I'd keep it.

Copies might sound dangerous, but building against kernel headers means
sources need to be in sync. The compile-time #ifdef-mess would cause
much more grief. Without, it's just negotiating at runtime, that's much
more flexible.

> > This isn't sick: Blktap2 doesn't need the full ring.h macro contents
> > with memory barriers etc anyway, because the userland dispatching is
> > synchronous. It could be just bare structs, and the standard PUSH/PULL
> > macros are rather decoration and could be dropped (or reimplemented as
> > memcpy()s).
> >
> > Will this justify linux/blktap.h?
> >
> > One could also revert that ring.h pad space hack.
> >
> > I'm not passionate about it. If you still disagree, I'll give up and we
> > move it elsewhere.
> >
> > In this case, it could as well go back into drivers/block/blktap, and
> > I'll just give up on 'development mode' hacks to verify tapdisk builds
> > against the kernel tree altogether.
> 
> What sort of "'development mode' hacks"?

Just referring to the above: Userspace requiring original kernel
includes. Nice for development, but iirc mainline stopped promoting that
entirely, a long time ago.

Daniel

> > > Where is the canonical definition of this interface stored? In the
> > > kernel tree or the hypervisor tree?
> >
> > You mean blktap.h? This is not a xen driver. I'd call this the canonical
> > definition, a reference with what that kernel/driver revision supports,
> > that's why I put it there.
> >
> > It wouldn't belong elsewhere, except for occasionally updated verbatim
> > copies in updated blktap sources, to unstress build dependencies.
> >
> > Daniel
> >
> > > Ian.
> > >
> > > >  7 files changed, 142 insertions(+), 100 deletions(-)
> > > >  create mode 100644 include/linux/blktap.h
> > > >
> > > > diff --git a/drivers/xen/blktap/blktap.h b/drivers/xen/blktap/blktap.h
> > > > index fe63fc9..1318cad 100644
> > > > --- a/drivers/xen/blktap/blktap.h
> > > > +++ b/drivers/xen/blktap/blktap.h

> 



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