Re: [Qemu-devel] [RFC PATCH -V1 4/4] hw/9pfs: Add open flag to fid

[Aneesh Kumar K. V]

On Mon, 28 Feb 2011 18:00:56 -0800, "Venkateswararao Jujjuri (JV)" 
<address@hidden> wrote:
> On 2/5/2011 10:08 AM, Aneesh Kumar K.V wrote:
> > We use this flag when we reopen the file. We need
> > to track open flag because if the open request have
> > flags like O_SYNC, we want to open the file with same flag
> > in host too
> > 
> > Signed-off-by: Aneesh Kumar K.V <address@hidden>
> > ---
> >  hw/9pfs/virtio-9p.c |   57 
> > ++++++++++++++++++++++++++++++++++++++++++---------
> >  hw/9pfs/virtio-9p.h |    1 +
> >  2 files changed, 48 insertions(+), 10 deletions(-)
> > 
> > diff --git a/hw/9pfs/virtio-9p.c b/hw/9pfs/virtio-9p.c
> > index 1518e00..cbdf705 100644
> > --- a/hw/9pfs/virtio-9p.c
> > +++ b/hw/9pfs/virtio-9p.c
> > @@ -22,6 +22,8 @@
> >  int debug_9p_pdu;
> >  static void v9fs_reclaim_fd(V9fsState *s);
> > 
> > +#define PASS_OPEN_FLAG (O_SYNC | O_DSYNC | O_RSYNC |  \
> > +                        O_EXCL)
> >  enum {
> >      Oread   = 0x00,
> >      Owrite  = 0x01,
> > @@ -68,6 +70,24 @@ static int omode_to_uflags(int8_t mode)
> >      return ret;
> >  }
> > 
> > +static int get_dotl_openflags(int oflags)
> > +{
> > +    int flags;
> > +    /*
> > +     * Since we can share the fd between multiple fids,
> > +     * open the file in read write mode
> > +     */
> > +    flags = O_RDWR;
> > +    /*
> > +     * If the client asked for any of the below flags we
> > +     * should open the file with same open flag
> > +     */
> > +    if (oflags & PASS_OPEN_FLAG) {
> > +        flags =  oflags & PASS_OPEN_FLAG;
> Isn't it
> 
>  flags |= oflags & PASS_OPEN_FLAG; ?

Already fixed in my local tree. 

> > +    }
> > +    return flags;
> > +}
> > +
> >  void cred_init(FsCred *credp)
> >  {
> >      credp->fc_uid = -1;
> > @@ -452,9 +472,9 @@ static V9fsFidState *lookup_fid(V9fsState *s, int32_t 
> > fid)
> >               * descriptors.
> >               */
> >              if (f->fsmap.fid_type == P9_FID_FILE) {
> > -                /* FIXME!! should we remember the open flags ?*/
> >                  if (f->fsmap.fs.fd == -1) {
> > -                    f->fsmap.fs.fd = v9fs_do_open(s, &f->fsmap.path, 
> > O_RDWR);
> > +                    f->fsmap.fs.fd = v9fs_do_open(s, &f->fsmap.path,
> > +                                                  f->fsmap.open_flags);
> >                  }
> >              }
> >              /*
> > @@ -1811,14 +1831,19 @@ static void v9fs_open_post_lstat(V9fsState *s, 
> > V9fsOpenState *vs, int err)
> >          v9fs_open_post_opendir(s, vs, err);
> >      } else {
> >          if (s->proto_version == V9FS_PROTO_2000L) {
> > -            flags = vs->mode;
> > -            flags &= ~(O_NOCTTY | O_ASYNC | O_CREAT);
> > -            /* Ignore direct disk access hint until the server supports 
> > it. */
> > -            flags &= ~O_DIRECT;
> 
> This is more drastic change. So far we are ignoring only few flags..but with
> this we are allowing only few flags. Need to understand the impact of this on
> dotl protocol.

The goal is to make sure we use only those flags that make sense for the
host file system. That would enable us to share the fd between multiple
fids. We cannot share fd with different open flags. 

> 
> Also I would put this patch ahead of introducing fid reclaim patch.
> 
> > +            flags = get_dotl_openflags(vs->mode);
> >          } else {
> >              flags = omode_to_uflags(vs->mode);
> >          }
> >          vs->fidp->fsmap.fs.fd = v9fs_do_open(s, &vs->fidp->fsmap.path, 
> > flags);
> > +        vs->fidp->fsmap.open_flags = flags;
> > +        if (flags & O_EXCL) {
> > +            /*
> > +             * We let the host file system do O_EXCL check
> > +             * We should not reclaim such fd
> > +             */
> > +            vs->fidp->fsmap.flags  |= FID_NON_RECLAIMABLE;
> > +        }
> >          v9fs_open_post_open(s, vs, err);
> >      }
> >      return;
> > @@ -1937,11 +1962,22 @@ static void v9fs_lcreate(V9fsState *s, V9fsPDU *pdu)
> >      v9fs_string_sprintf(&vs->fullname, "%s/%s", vs->fidp->fsmap.path.data,
> >               vs->name.data);
> > 
> > -    /* Ignore direct disk access hint until the server supports it. */
> > -    flags &= ~O_DIRECT;
> > -
> > +    flags = get_dotl_openflags(flags);
> >      vs->fidp->fsmap.fs.fd = v9fs_do_open2(s, vs->fullname.data, 
> > vs->fidp->uid,
> > -            gid, flags, mode);
> > +                                          gid, flags, mode);
> > +    /*
> > +     * We don't want to recreate the in reclaim path. So remove
> > +     * create flag
> > +     */
> > +    flags &= ~O_CREAT;
> > +    vs->fidp->fsmap.open_flags = flags;
> > +    if (flags & O_EXCL) {
> > +        /*
> > +         * We let the host file system do O_EXCL check
> > +         * We should not reclaim such fd
> > +         */
> > +        vs->fidp->fsmap.flags  |= FID_NON_RECLAIMABLE;
> > +    }
> You can move above two steps to here by...
> vs->fidp->fsmap.open_flags = flags & ~O_CREAT
> >      v9fs_lcreate_post_do_open2(s, vs, err);
> >      return;
> > 
> > @@ -2653,6 +2689,7 @@ static void v9fs_create_post_lstat(V9fsState *s, 
> > V9fsCreateState *vs, int err)
> >                                                -1,
> >                                                
> > omode_to_uflags(vs->mode)|O_CREAT,
> >                                                vs->perm);
> > +        vs->fidp->fsmap.open_flags = omode_to_uflags(vs->mode);
> 
> why are we not doing ~O_CREAT here?
> 

We don't have O_CREATE in open_flags. I explicitly pass that to the
above call. I also have another fix to make it work correct for dotl.

May be i should post the new version i have

-aneesh