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

[Stefan Hajnoczi]

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).

> +            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.

> @@ -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.

Stefan