Re: [Qemu-devel] [PATCH -V3 2/8] hw/9pfs: Add file descriptor reclaim support

[Aneesh Kumar K. V]

On Sun, 13 Mar 2011 16:08:29 +0000, Stefan Hajnoczi <address@hidden> wrote:
> On Sat, Mar 5, 2011 at 5:52 PM, Aneesh Kumar K.V
> <address@hidden> wrote:
> > @@ -107,7 +108,12 @@ static int v9fs_do_closedir(V9fsState *s, DIR *dir)
> >
> >  static int v9fs_do_open(V9fsState *s, V9fsString *path, int flags)
> >  {
> > -    return s->ops->open(&s->ctx, path->data, flags);
> > +    int fd;
> > +    fd = s->ops->open(&s->ctx, path->data, flags);
> > +    if (fd > P9_FD_RECLAIM_THRES) {
> > +        v9fs_reclaim_fd(s);
> > +    }
> 
> I think the threshold should depend on the file descriptor ulimit.
> The hardcoded constant doesn't work if the ulimit is set to 1000 or
> less (it would cause other users in QEMU to hit EMFILE errors).

Yes. That is suppose to be a follow up patch. I had that set to 100 for
all the early testing.

> 
> > +            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);
> > +                }
> 
> Please address the FIXME.  I think the case where O_RDWR breaks is if
> QEMU has permissions to open the file for read only.  The the client
> is able to open the file for read but when the file descriptor is
> resurrected we'll get EPERM here.

The FIXME is fixed in the follow up patch  (patch 5)

> 
> > @@ -516,7 +600,10 @@ static int free_fid(V9fsState *s, int32_t fid)
> >     *fidpp = fidp->next;
> >
> >     if (fidp->fsmap.fid_type == P9_FID_FILE) {
> > -        v9fs_do_close(s, fidp->fsmap.fs.fd);
> > +        /* I we reclaimed the fd no need to close */
> 
> s/I //
> 
> > +        if (fidp->fsmap.fs.fd != -1) {
> > +            v9fs_do_close(s, fidp->fsmap.fs.fd);
> > +        }
> >     } else if (fidp->fsmap.fid_type == P9_FID_DIR) {
> >         v9fs_do_closedir(s, fidp->fsmap.fs.dir);
> >     } else if (fidp->fsmap.fid_type == P9_FID_XATTR) {
> > @@ -2719,7 +2806,11 @@ static void v9fs_remove(V9fsState *s, V9fsPDU *pdu)
> >         err = -EINVAL;
> >         goto out;
> >     }
> > -
> > +    /*
> > +     * IF the file is unlinked, we cannot reopen
> > +     * the file later. So don't reclaim fd
> > +     */
> > +    v9fs_mark_fids_unreclaim(s, &vs->fidp->fsmap.path);
> 
> This poses a problem for the case where guest and host are both
> accessing the file system.  If the fd is reclaimed and the host
> deletes the file, then the guest cannot access its open file anymore.
> 
> The same issue also affects rename and has not been covered by this patch.
> 

Currently virtFS don't handle the host rename/unlink. That we walk
a name and get the fid and then use the fid to open the file. In between
if the file get removed/renamed we will get an EINVAL.

All that will go away once we switch to handle based open.

-aneesh