Re: [V9fs-developer] [Qemu-devel] Re: [PATCH] virtio-9p: getattr server implementation for 9P2000.L protocol.

[Sripathi Kodi <sripathik@in.ibm.com>]

On Mon, 7 Jun 2010 16:04:17 +0530
Sripathi Kodi <sripathik@in.ibm.com> wrote:

There was one mistake in my patch. See below.

> On Sat, 05 Jun 2010 19:11:53 +0530
> "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> 
> > On Fri, 04 Jun 2010 07:59:42 -0700, "Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com> wrote:
> > > Aneesh Kumar K. V wrote:
> > > > On Thu, 3 Jun 2010 18:29:02 +0530, Sripathi Kodi <sripathik@in.ibm.com> wrote:
> > > >> On Wed, 02 Jun 2010 19:49:24 +0530
> > > >> "Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> wrote:
> > > >>
> > > >>> On Fri, 28 May 2010 16:08:43 +0530, Sripathi Kodi <sripathik@in.ibm.com> wrote:
> > > >>>> From: M. Mohan Kumar <mohan@in.ibm.com>
> > > >>>>
> > > >>>> SYNOPSIS
> > > >>>>
> > > >>>>       size[4] Tgetattr tag[2] fid[4]
> > > >>>>
> > > >>>>       size[4] Rgetattr tag[2] lstat[n]
> > > >>>>
> > > >>>>    DESCRIPTION
> > > >>>>
> > > >>>>       The getattr transaction inquires about the file identified by fid.
> > > >>>>       The reply will contain a machine-independent directory entry,
> > > >>>>       laid out as follows:
> > > >>>>
> > > >>>>          qid.type[1]
> > > >>>>             the type of the file (directory, etc.), represented as a bit
> > > >>>>             vector corresponding to the high 8 bits of the file's mode
> > > >>>>             word.
> > > >>>>
> > > >>>>          qid.vers[4]
> > > >>>>             version number for given path
> > > >>>>
> > > >>>>          qid.path[8]
> > > >>>>             the file server's unique identification for the file
> > > >>>>
> > > >>>>          st_mode[4]
> > > >>>>             Permission and flags
> > > >>>>
> > > >>>>          st_nlink[8]
> > > >>>>             Number of hard links
> > > >>>>
> > > >>>>          st_uid[4]
> > > >>>>             User id of owner
> > > >>>>
> > > >>>>          st_gid[4]
> > > >>>>             Group ID of owner
> > > >>>>
> > > >>>>          st_rdev[8]
> > > >>>>             Device ID (if special file)
> > > >>>>
> > > >>>>          st_size[8]
> > > >>>>             Size, in bytes
> > > >>>>
> > > >>>>          st_blksize[8]
> > > >>>>             Block size for file system IO
> > > >>>
> > > >>> So it should be scaled by iounit right ? If we say 9p block size is iounit.
> > > >> Yes, I think it should be iounit. Currently st_blksize being returned
> > > >> in stat structure to the user space does not use this field that comes
> > > >> from the server. It is being calculated as follows in
> > > >> generic_fillattr():
> > > >>
> > > >>         stat->blksize = (1 << inode->i_blkbits);
> > > >>
> > > >> So there may not be a need to put st_blksize on the protocol. Further,
> > > >> inode->i_blkbits is copied from sb->s_blocksize_bits. For 9P this value
> > > >> is obtained as:
> > > > 
> > > > That is what linux kernel currently does. But from the protocol point of
> > > > view and not looking at specific linux implementation i would suggest to
> > > > put st_blksize on wire. 
> > > 
> > > This is part of .L protocol. Specifically for Linux systems. So, if Linux is already
> > > doing it, I don't think we need to repeat it.
> > > 
> > 
> > But nothing prevents from changing Linux internal implementation. So we
> > can't depend on Linux kernel internal implementation. Later in 2.6.x we
> > may not derive stat->blksize from inode->i_blkbits at all. So we cannot
> > depend on Linux kernel internal implementation.
> 
> Okay, agreed.
> 
> I changed my patch to implement the change you have suggested.
> Basically I return 'iounit' as 'st_blksize' and in 'st_blocks' I return
> the number of iounit blocks required to fit st_size of the file. The
> following patches are diffs from my old patch. They require the iounit
> patches that Mohan has sent to this list on 4th.
> 
> Comments welcome. Specifically please look at the changes in 
> v9fs_getattr_post_lstat() below.
> 
> Thanks,
> Sripathi.
> 
> 
> Kernel patch:
> =============
> 
> Fix block size of getattr call. Depends on Mohan's iounit patch.
> 
> Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
> 
> 
> ---
> 
>  fs/9p/vfs_inode.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> 
> diff --git a/fs/9p/vfs_inode.c b/fs/9p/vfs_inode.c
> index 19067de..c01d33b 100644
> --- a/fs/9p/vfs_inode.c
> +++ b/fs/9p/vfs_inode.c
> @@ -955,6 +955,8 @@ v9fs_vfs_getattr_dotl(struct vfsmount *mnt, struct dentry *dentry,
> 
>  	v9fs_stat2inode_dotl(st, dentry->d_inode);
>  	generic_fillattr(dentry->d_inode, stat);
> +	/* Change block size to what the server returned */
> +	stat->blksize = st->st_blksize;
> 
>  	kfree(st);
>  	return 0;
> 
> 
> 
> QEMU patch:
> ===========
> 
> Fix block size of getattr call. Depends on Mohan's iounit patch.
> 
> Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
> 
> 
> ---
> 
>  hw/virtio-9p.c |   55 +++++++++++++++++++++++++++++++------------------------
>  hw/virtio-9p.h |    1 +
>  2 files changed, 32 insertions(+), 24 deletions(-)
> 
> 
> diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> index 4843820..d164ad3 100644
> --- a/hw/virtio-9p.c
> +++ b/hw/virtio-9p.c
> @@ -1180,6 +1180,26 @@ out:
>      qemu_free(vs);
>  }
> 
> +static int32_t get_iounit(V9fsState *s, V9fsString *name)
> +{
> +    struct statfs stbuf;
> +    int32_t iounit = 0;
> +
> +    /*
> +     * iounit should be multiples of f_bsize (host filesystem block size
> +     * and as well as less than (client msize - P9_IOHDRSZ))
> +     */
> +    if (!v9fs_do_statfs(s, name, &stbuf)) {
> +        iounit = stbuf.f_bsize;
> +        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> +    }
> +
> +    if (!iounit) {
> +        iounit = s->msize - P9_IOHDRSZ;
> +    }
> +    return iounit;
> +}
> +
>  static void v9fs_getattr_post_lstat(V9fsState *s, V9fsStatStateDotl *vs,
>                                                                  int err)
>  {
> @@ -1188,7 +1208,15 @@ static void v9fs_getattr_post_lstat(V9fsState *s, V9fsStatStateDotl *vs,
>          goto out;
>      }
> 
> +    /* Recalculate block size and number of blocks based on iounit */
>      stat_to_v9stat_dotl(s, &vs->stbuf, &vs->v9stat_dotl);
> +    vs->v9stat_dotl.st_blksize = get_iounit(s, &vs->fidp->path);
> +    vs->v9stat_dotl.st_blocks = vs->v9stat_dotl.st_size /
> +                                vs->v9stat_dotl.st_blksize;
> +    if (vs->v9stat_dotl.st_size % vs->v9stat_dotl.st_blksize) {
> +        vs->v9stat_dotl.st_blocks++;
> +    }
> +

I should not update st_blocks when I update st_blksize. This is
because from the manpage, st_blocks is always number of 512 byte blocks
needed for this file.

blkcnt_t  st_blocks;  /* number of 512B blocks allocated */

Hence by changing st_blocks user space tools like 'du' go wrong.

Thanks,
Sripathi.


>      vs->offset += pdu_marshal(vs->pdu, vs->offset, "A", &vs->v9stat_dotl);
>      err = vs->offset;
> 
> @@ -1202,7 +1230,6 @@ static void v9fs_getattr(V9fsState *s, V9fsPDU *pdu)
>      int32_t fid;
>      V9fsStatStateDotl *vs;
>      ssize_t err = 0;
> -    V9fsFidState *fidp;
> 
>      vs = qemu_malloc(sizeof(*vs));
>      vs->pdu = pdu;
> @@ -1212,13 +1239,13 @@ static void v9fs_getattr(V9fsState *s, V9fsPDU *pdu)
> 
>      pdu_unmarshal(vs->pdu, vs->offset, "d", &fid);
> 
> -    fidp = lookup_fid(s, fid);
> -    if (fidp == NULL) {
> +    vs->fidp = lookup_fid(s, fid);
> +    if (vs->fidp == NULL) {
>          err = -ENOENT;
>          goto out;
>      }
> 
> -    err = v9fs_do_lstat(s, &fidp->path, &vs->stbuf);
> +    err = v9fs_do_lstat(s, &vs->fidp->path, &vs->stbuf);
>      v9fs_getattr_post_lstat(s, vs, err);
>      return;
> 
> @@ -1390,26 +1417,6 @@ out:
>      v9fs_walk_complete(s, vs, err);
>  }
> 
> -static int32_t get_iounit(V9fsState *s, V9fsString *name)
> -{
> -    struct statfs stbuf;
> -    int32_t iounit = 0;
> -
> -    /*
> -     * iounit should be multiples of f_bsize (host filesystem block size
> -     * and as well as less than (client msize - P9_IOHDRSZ))
> -     */
> -    if (!v9fs_do_statfs(s, name, &stbuf)) {
> -        iounit = stbuf.f_bsize;
> -        iounit *= (s->msize - P9_IOHDRSZ)/stbuf.f_bsize;
> -    }
> -
> -    if (!iounit) {
> -        iounit = s->msize - P9_IOHDRSZ;
> -    }
> -    return iounit;
> -}
> -
>  static void v9fs_open_post_opendir(V9fsState *s, V9fsOpenState *vs, int err)
>  {
>      if (vs->fidp->dir == NULL) {
> diff --git a/hw/virtio-9p.h b/hw/virtio-9p.h
> index 700666a..6b09b4b 100644
> --- a/hw/virtio-9p.h
> +++ b/hw/virtio-9p.h
> @@ -211,6 +211,7 @@ typedef struct V9fsStatDotl {
>  typedef struct V9fsStatStateDotl {
>      V9fsPDU *pdu;
>      size_t offset;
> +    V9fsFidState *fidp;
>      V9fsStatDotl v9stat_dotl;
>      struct stat stbuf;
>  } V9fsStatStateDotl;
> 
> ------------------------------------------------------------------------------
> ThinkGeek and WIRED's GeekDad team up for the Ultimate 
> GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the 
> lucky parental unit.  See the prize list and enter to win: 
> http://p.sf.net/sfu/thinkgeek-promo
> _______________________________________________
> V9fs-developer mailing list
> V9fs-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/v9fs-developer