On Mon, 7 Jun 2010 16:04:17 +0530 Sripathi Kodi <sripat...@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.ku...@linux.vnet.ibm.com> wrote: > > > On Fri, 04 Jun 2010 07:59:42 -0700, "Venkateswararao Jujjuri (JV)" > > <jv...@linux.vnet.ibm.com> wrote: > > > Aneesh Kumar K. V wrote: > > > > On Thu, 3 Jun 2010 18:29:02 +0530, Sripathi Kodi <sripat...@in.ibm.com> > > > > wrote: > > > >> On Wed, 02 Jun 2010 19:49:24 +0530 > > > >> "Aneesh Kumar K. V" <aneesh.ku...@linux.vnet.ibm.com> wrote: > > > >> > > > >>> On Fri, 28 May 2010 16:08:43 +0530, Sripathi Kodi > > > >>> <sripat...@in.ibm.com> wrote: > > > >>>> From: M. Mohan Kumar <mo...@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 <sripat...@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 <sripat...@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-develo...@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/v9fs-developer