Re: [PATCH 2/2] kvm tools: Add virtio-9p

[Sasha Levin <levinsasha928@gmail.com>]

On Tue, 2011-05-17 at 20:18 -0500, Eric Van Hensbergen wrote:
> On Tue, May 17, 2011 at 3:27 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > On Tue, 2011-05-17 at 22:08 +0300, Sasha Levin wrote:
> >> 'kvm_9p' isn't created as a device under /dev, it's just a name used
> >> internally by 9pnet_virtio (and located under sysfs).
> >>
> >> I couldn't figure out which params the kernel would expect to boot using
> >> 9p over virtio (theres no device name to begin with).
> >>
> >> I've also couldn't find anything that suggested it's possible to boot
> >> using virtio-9p as rootfs.
> >
> > Ignore that.
> >
> > Naming the virtio transport "/dev/root" and passing proper params to the
> > kernel makes it work:
> >
> > [    1.844983] VFS: Mounted root (9p filesystem) on device 0:11.
> >
> > I'll make some changes to the virtio-9p patch to make it easier for the
> > user to do that.
> >
> 
> This is really sweet.  Thanks for beating me to the punch of porting
> the 9p support to kvm tools.

Clear RFC and good source code to refer to within 9p modules made this
easy (and fun) :)

> > - Multiple virtio-9p devices.
> 
> This should be pretty straightforward.

Yes, Most of the work here is within the kvm tool.

> > - Ugly hack in virtio_p9_stat() (See desc in code).
> >       /*
> >+        * HACK: For some reason the p9 virtio transport reads a u16 and discards
> >+        * it before reading the p9_rstat struct. I couldn't find a logical reason for
> >+        * that, so we just add an extra u16 before the struct.
> >+        */
> 
> This is part of the protocol spec (from
> http://ericvh.github.com/9p-rfc/rfc9p2000.html#anchor32):
> "To make the contents of a directory, such as returned by read(5),
> easy to parse, each directory entry begins with a size field. For
> consistency, the entries in Twstat and Rstat messages also contain
> their size, which means the size appears twice. For example, the Rstat
> message is formatted as ``(4+1+2+2+n)[4] Rstat tag[2] n[2] (n-2)[2]
> type[2] dev[4]...,'' where n is the value returned by convD2M."
> It's appropriate to duplicate the size.  I think the Linux client
> ignores it, but others implementations may complain.

Thanks for the explanation!
Yes, Linux implementation just throws it away - which was what confused
me initially.
Why not add a u16 to the beginning of 'struct p9_rstat'?

> > - Update atime/mtime in p9_wstat, not really needed.
> 
> The underlying storage may handle this for you, I think 9p avoids
> updating atime by default, at least in caching scenarios -- too much
> unnecessary protocol traffic.

My assumption was that the storage I read/write to will take care of it
for me, and unless it bothers anyone in the future I'll assume it's
doing a good job at it.

> > - Pass usernames in p9_stat, not really needed and not really sure how p9 expects to handle them.
> 
> The username, group name issue is one of the principle reasons behind
> the extended protocol operations (.u and .L) -- of course, if there
> was a Plan 9 or Inferno guest they would be quite happy with the
> usernames, but Linux (and other UNIX variants) will want the ids.  To
> really keep things simple we could add a client option that would let
> you pass the various ids as strings.  Although no doubt folks will
> want the other extensions (symlinks, links, device nodes, etc.) before
> long.  When we built the qemu server for .L, the team tried to keep
> everything in a library, but there is some entanglement with the qemu
> APIs -- it'd be nice if we could reuse that code here, maybe we need
> an abstract glue layer so that the core code can be used by both the
> kvm tool and qemu.  I'm copy the lead of that team on this message
> just so he's aware how far you've come.

I'd prefer using a tested lib which also implements .L over what we have
now, assuming it's not tangled into qemu too hard.


> 
>          -eric


-- 

Sasha.