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

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

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:

sb->s_blocksize_bits = fls(v9ses->maxdata - 1);
and
v9ses->maxdata = v9ses->clnt->msize - P9_IOHDRSZ;

Due to the above calculation sometimes stat() on a file can report
incorrect values. For example, if I mount 9P file system with
msize=5000 stat on a file shows me IO Block: 8192! However, we don't
consider this when we do actual file data transfer. We use 
clnt->msize - P9_IOHDRSZ.
Hence it looks to me like i_blkbits is only used to return stat data.

> 
> 
> > 
> >          st_blocks[8]
> >             Number of file system blocks allocated
> 
> same here. 

Yes, this should be file size/iounit.

Thanks,
Sripathi.

> 
> > 
> >          st_atime_sec[8]
> >             Time of last access, seconds
> > 
> >          st_atime_nsec[8]
> >             Time of last access, nanoseconds
> > 
> >          st_mtime_sec[8]
> >             Time of last modification, seconds
> > 
> >          st_mtime_nsec[8]
> >             Time of last modification, nanoseconds
> > 
> >          st_ctime_sec[8]
> >             Time of last status change, seconds
> > 
> >          st_ctime_nsec[8]
> >             Time of last status change, nanoseconds
> > 
> > 
> > This patch implements the client side of getattr implementation for 9P2000.L.
> > It introduces a new structure p9_stat_dotl for getting Linux stat information
> > along with QID. The data layout is similar to stat structure in Linux user
> > space with the following major differences:
> > 
> > inode (st_ino) is not part of data. Instead qid is.
> > 
> > device (st_dev) is not part of data because this doesn't make sense on the
> > client.
> > 
> > All time variables are 64 bit wide on the wire. The kernel seems to use
> > 32 bit variables for these variables. However, some of the architectures
> > have used 64 bit variables and glibc exposes 64 bit variables to user
> > space on some architectures. Hence to be on the safer side we have made
> > these 64 bit in the protocol. Refer to the comments in
> > include/asm-generic/stat.h
> > 
> > 
> > Signed-off-by: M. Mohan Kumar <mohan@in.ibm.com>
> > Signed-off-by: Sripathi Kodi <sripathik@in.ibm.com>
> > ---
> > 
> >  hw/virtio-9p-debug.c |   32 ++++++++++++++++++++
> >  hw/virtio-9p.c       |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  hw/virtio-9p.h       |   28 +++++++++++++++++
> >  3 files changed, 142 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/virtio-9p-debug.c b/hw/virtio-9p-debug.c
> > index a82b771..8bb817d 100644
> > --- a/hw/virtio-9p-debug.c
> > +++ b/hw/virtio-9p-debug.c
> > @@ -178,6 +178,30 @@ static void pprint_stat(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> >      fprintf(llogfile, "}");
> >  }
> > 
> > +static void pprint_stat_dotl(V9fsPDU *pdu, int rx, size_t *offsetp,
> > +                                                  const char *name)
> > +{
> > +    fprintf(llogfile, "%s={", name);
> > +    pprint_qid(pdu, rx, offsetp, "qid");
> > +    pprint_int32(pdu, rx, offsetp, ", st_mode");
> > +    pprint_int64(pdu, rx, offsetp, ", st_nlink");
> > +    pprint_int32(pdu, rx, offsetp, ", st_uid");
> > +    pprint_int32(pdu, rx, offsetp, ", st_gid");
> > +    pprint_int64(pdu, rx, offsetp, ", st_rdev");
> > +    pprint_int64(pdu, rx, offsetp, ", st_size");
> > +    pprint_int64(pdu, rx, offsetp, ", st_blksize");
> > +    pprint_int64(pdu, rx, offsetp, ", st_blocks");
> > +    pprint_int64(pdu, rx, offsetp, ", atime");
> > +    pprint_int64(pdu, rx, offsetp, ", atime_nsec");
> > +    pprint_int64(pdu, rx, offsetp, ", mtime");
> > +    pprint_int64(pdu, rx, offsetp, ", mtime_nsec");
> > +    pprint_int64(pdu, rx, offsetp, ", ctime");
> > +    pprint_int64(pdu, rx, offsetp, ", ctime_nsec");
> > +    fprintf(llogfile, "}");
> > +}
> > +
> > +
> > +
> >  static void pprint_strs(V9fsPDU *pdu, int rx, size_t *offsetp, const char *name)
> >  {
> >      int sg_count = get_sg_count(pdu, rx);
> > @@ -351,6 +375,14 @@ void pprint_pdu(V9fsPDU *pdu)
> >          pprint_int32(pdu, 1, &offset, "msize");
> >          pprint_str(pdu, 1, &offset, ", version");
> >          break;
> > +    case P9_TGETATTR:
> > +        fprintf(llogfile, "TGETATTR: (");
> > +        pprint_int32(pdu, 0, &offset, "fid");
> > +        break;
> > +    case P9_RGETATTR:
> > +        fprintf(llogfile, "RGETATTR: (");
> > +        pprint_stat_dotl(pdu, 1, &offset, "getattr");
> > +        break;
> >      case P9_TAUTH:
> >          fprintf(llogfile, "TAUTH: (");
> >          pprint_int32(pdu, 0, &offset, "afid");
> > diff --git a/hw/virtio-9p.c b/hw/virtio-9p.c
> > index 097dce8..23ae8b8 100644
> > --- a/hw/virtio-9p.c
> > +++ b/hw/virtio-9p.c
> > @@ -737,6 +737,17 @@ static size_t pdu_marshal(V9fsPDU *pdu, size_t offset, const char *fmt, ...)
> >                          statp->n_gid, statp->n_muid);
> >              break;
> >          }
> > +        case 'A': {
> > +            V9fsStatDotl *statp = va_arg(ap, V9fsStatDotl *);
> > +            offset += pdu_marshal(pdu, offset, "Qdqddqqqqqqqqqq",
> > +                        &statp->qid, statp->st_mode, statp->st_nlink,
> > +                        statp->st_uid, statp->st_gid, statp->st_rdev,
> > +                        statp->st_size, statp->st_blksize, statp->st_blocks,
> > +                        statp->st_atime_sec, statp->st_atime_nsec,
> > +                        statp->st_mtime_sec, statp->st_mtime_nsec,
> > +                        statp->st_ctime_sec, statp->st_ctime_nsec);
> > +            break;
> > +        }
> >          default:
> >              break;
> >          }
> > @@ -964,6 +975,29 @@ static int stat_to_v9stat(V9fsState *s, V9fsString *name,
> >      return 0;
> >  }
> > 
> > +static void stat_to_v9stat_dotl(V9fsState *s, const struct stat *stbuf,
> > +                            V9fsStatDotl *v9lstat)
> > +{
> > +    memset(v9lstat, 0, sizeof(*v9lstat));
> > +
> > +    v9lstat->st_mode = stbuf->st_mode;
> > +    v9lstat->st_nlink = stbuf->st_nlink;
> > +    v9lstat->st_uid = stbuf->st_uid;
> > +    v9lstat->st_gid = stbuf->st_gid;
> > +    v9lstat->st_rdev = stbuf->st_rdev;
> > +    v9lstat->st_size = stbuf->st_size;
> > +    v9lstat->st_blksize = stbuf->st_blksize;
> > +    v9lstat->st_blocks = stbuf->st_blocks;
> > +    v9lstat->st_atime_sec = stbuf->st_atime;
> > +    v9lstat->st_atime_nsec = stbuf->st_atim.tv_nsec;
> > +    v9lstat->st_mtime_sec = stbuf->st_mtime;
> > +    v9lstat->st_mtime_nsec = stbuf->st_mtim.tv_nsec;
> > +    v9lstat->st_ctime_sec = stbuf->st_ctime;
> > +    v9lstat->st_ctime_nsec = stbuf->st_ctim.tv_nsec;
> > +
> > +    stat_to_qid(stbuf, &v9lstat->qid);
> > +}
> > +
> >  static struct iovec *adjust_sg(struct iovec *sg, int len, int *iovcnt)
> >  {
> >      while (len && *iovcnt) {
> > @@ -1131,6 +1165,53 @@ out:
> >      qemu_free(vs);
> >  }
> > 
> > +static void v9fs_getattr_post_lstat(V9fsState *s, V9fsStatStateDotl *vs,
> > +                                                                int err)
> > +{
> > +    if (err == -1) {
> > +        err = -errno;
> > +        goto out;
> > +    }
> > +
> > +    stat_to_v9stat_dotl(s, &vs->stbuf, &vs->v9stat_dotl);
> > +    vs->offset += pdu_marshal(vs->pdu, vs->offset, "A", &vs->v9stat_dotl);
> > +    err = vs->offset;
> > +
> > +out:
> > +    complete_pdu(s, vs->pdu, err);
> > +    qemu_free(vs);
> > +}
> > +
> > +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;
> > +    vs->offset = 7;
> > +
> > +    memset(&vs->v9stat_dotl, 0, sizeof(vs->v9stat_dotl));
> > +
> > +    pdu_unmarshal(vs->pdu, vs->offset, "d", &fid);
> > +
> > +    fidp = lookup_fid(s, fid);
> > +    if (fidp == NULL) {
> > +        err = -ENOENT;
> > +        goto out;
> > +    }
> > +
> > +    err = v9fs_do_lstat(s, &fidp->path, &vs->stbuf);
> > +    v9fs_getattr_post_lstat(s, vs, err);
> > +    return;
> > +
> > +out:
> > +    complete_pdu(s, vs->pdu, err);
> > +    qemu_free(vs);
> > +}
> > +
> >  static void v9fs_walk_complete(V9fsState *s, V9fsWalkState *vs, int err)
> >  {
> >      complete_pdu(s, vs->pdu, err);
> > @@ -2343,6 +2424,7 @@ typedef void (pdu_handler_t)(V9fsState *s, V9fsPDU *pdu);
> >  static pdu_handler_t *pdu_handlers[] = {
> >      [P9_TREADDIR] = v9fs_readdir,
> >      [P9_TSTATFS] = v9fs_statfs,
> > +    [P9_TGETATTR] = v9fs_getattr,
> >      [P9_TVERSION] = v9fs_version,
> >      [P9_TATTACH] = v9fs_attach,
> >      [P9_TSTAT] = v9fs_stat,
> > diff --git a/hw/virtio-9p.h b/hw/virtio-9p.h
> > index 9773659..7e5609e 100644
> > --- a/hw/virtio-9p.h
> > +++ b/hw/virtio-9p.h
> > @@ -15,6 +15,8 @@
> >  enum {
> >      P9_TSTATFS = 8,
> >      P9_RSTATFS,
> > +    P9_TGETATTR = 24,
> > +    P9_RGETATTR,
> >      P9_TREADDIR = 40,
> >      P9_RREADDIR,
> >      P9_TVERSION = 100,
> > @@ -177,6 +179,32 @@ typedef struct V9fsStatState {
> >      struct stat stbuf;
> >  } V9fsStatState;
> > 
> > +typedef struct V9fsStatDotl {
> > +    V9fsQID qid;
> > +    uint32_t st_mode;
> > +    uint64_t st_nlink;
> > +    uint32_t st_uid;
> > +    uint32_t st_gid;
> > +    uint64_t st_rdev;
> > +    uint64_t st_size;
> > +    uint64_t st_blksize;
> > +    uint64_t st_blocks;
> > +    uint64_t st_atime_sec;
> > +    uint64_t st_atime_nsec;
> > +    uint64_t st_mtime_sec;
> > +    uint64_t st_mtime_nsec;
> > +    uint64_t st_ctime_sec;
> > +    uint64_t st_ctime_nsec;
> > +} V9fsStatDotl;
> > +
> > +typedef struct V9fsStatStateDotl {
> > +    V9fsPDU *pdu;
> > +    size_t offset;
> > +    V9fsStatDotl v9stat_dotl;
> > +    struct stat stbuf;
> > +} V9fsStatStateDotl;
> > +
> > +
> >  typedef struct V9fsWalkState {
> >      V9fsPDU *pdu;
> >      size_t offset;
> > 
> 
> 
> -aneesh