[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [V9fs-developer] [Qemu-devel] Re: [PATCH] virtio-9p: getattr server
From: |
Sripathi Kodi |
Subject: |
Re: [V9fs-developer] [Qemu-devel] Re: [PATCH] virtio-9p: getattr server implementation for 9P2000.L protocol. |
Date: |
Mon, 7 Jun 2010 17:58:11 +0530 |
On Mon, 7 Jun 2010 16:04:17 +0530
Sripathi Kodi <address@hidden> wrote:
There was one mistake in my patch. See below.
> On Sat, 05 Jun 2010 19:11:53 +0530
> "Aneesh Kumar K. V" <address@hidden> wrote:
>
> > On Fri, 04 Jun 2010 07:59:42 -0700, "Venkateswararao Jujjuri (JV)"
> > <address@hidden> wrote:
> > > Aneesh Kumar K. V wrote:
> > > > On Thu, 3 Jun 2010 18:29:02 +0530, Sripathi Kodi <address@hidden> wrote:
> > > >> On Wed, 02 Jun 2010 19:49:24 +0530
> > > >> "Aneesh Kumar K. V" <address@hidden> wrote:
> > > >>
> > > >>> On Fri, 28 May 2010 16:08:43 +0530, Sripathi Kodi <address@hidden>
> > > >>> wrote:
> > > >>>> From: M. Mohan Kumar <address@hidden>
> > > >>>>
> > > >>>> 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 <address@hidden>
>
>
> ---
>
> 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 <address@hidden>
>
>
> ---
>
> 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
> address@hidden
> https://lists.sourceforge.net/lists/listinfo/v9fs-developer