diff mbox

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

Message ID 20100607160417.7adfbc2e@in.ibm.com
State New
Headers show

Commit Message

Sripathi Kodi June 7, 2010, 10:34 a.m. UTC 
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:

Comments

Sripathi Kodi June 7, 2010, 12:28 p.m. UTC | #1 
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
diff mbox

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++;
+    }
+
     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;