Re: [PATCH] Add O_ASYNC support to FUSE

[Miklos Szeredi <miklos@szeredi.hu>]

> This adds asynchronous notification to FUSE - a FUSE server can
> request O_ASYNC on a /dev/fuse file descriptor and receive SIGIO
> when there is input available.
> 
> One subtlety - fuse_dev_fasync, which is called when O_ASYNC is
> requested, does no locking, unlike the other methods.  I think it's
> unnecessary, as the fuse_conn.fasync list is manipulated only by
> fasync_helper and kill_fasync, which provide their own locking.

Yes, but you need to use fuse_get_conn() and check the result (see
fuse_dev_poll()).

> It would also be wrong to use fuse_lock, as it's a spin lock and
> fasync_helper can sleep.  My one concern with this is the fuse_conn
> going away underneath fuse_dev_fasync - sys_fcntl takes a reference
> on the file struct, so this seems not to be a problem.

Yes, file holds a reference to fuse_conn.

> One possible bug - there is a wakeup in inode.c:fuse_put_super, but
> sticking a kill_fasync there causes BUGs because the /dev/fuse
> file struct seems to have gone away.

I don't see how that could happen.  fuse_dev_release() removes the
file from fc->fasync, and fasync_helper() and kill_fasync() are
protected with fasync_lock against racing with each other.

Although it may be possible that kill_fasync() races with the final
fput(), but as I see kill_fasync() only uses the file->f_owner field
which should still be valid before and during file->release().

Which BUG is triggered?

> So, this may be lacking when a FUSE mount is unmounted from
> underneath the server (is that possible, or must the server be
> killed in order to do the umount?).

Yes it's possible, it is how a normal unmount happens:

  - umount() called on the filesystem, filesystem unmounted
  - read on /dev/fuse returns -ENODEV
  - userspace filesystem exits gracefuly

> @@ -894,6 +895,7 @@ void fuse_abort_conn(struct fuse_conn *f
>  		end_requests(fc, &fc->pending);
>  		end_requests(fc, &fc->processing);
>  		wake_up_all(&fc->waitq);
> +		kill_fasync(&fc->fasync, SIGIO, POLLIN);

This should be POLL_IN too, no?

> +static int fuse_dev_fasync(int fd, struct file *file, int on)
> +{
> +	struct fuse_conn *fc;
> +
> +	/* No locking - fasync_helper does its own locking */
> +	fc = file->private_data;
> +	return(fasync_helper(fd, file, on, &fc->fasync));

I like this style better:

	return fasync_helper(fd, file, on, &fc->fasync);

Thanks,
Miklos
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/