On Mon, Mar 14, 2016 at 02:53:11PM +0100, Miklos Szeredi wrote:
> On Fri, Mar 11, 2016 at 10:35:34AM -0600, Seth Forshee wrote:
> > The req member of fuse_io_priv serves two purposes. First is to
> > track the number of oustanding async requests to the server and
> > to signal that the io request is completed. The second is to be a
> > reference count on the structure to know when it can be freed.
> >
> > For sync io requests these purposes can be at odds.
> > fuse_direct_IO() wants to block until the request is done, and
> > since the signal is sent when req reaches 0 it cannot keep a
> > reference to the object. Yet it needs to use the object after the
> > userspace server has completed processing requests. This leads to
> > some handshaking and special casing that it needlessly
> > complicated and responsible for at least one race condition.
> >
> > It's much cleaner and safer to maintain a separate reference
> > count for the object lifecycle and to let req just be a count of
> > outstanding requests to the userspace server. Then we can know
> > for sure when it is safe to free the object without any
> > handshaking or special cases.
> >
> > The catch here is that most of the time these objects are stack
> > allocated and should not be freed. Initializing these objects
> > with a single reference that is never released prevents
> > accidental attempts to free the objects.
> >
> > Fixes: 9d5722b7777e ("fuse: handle synchronous iocbs internally")
> > Cc: sta...@vger.kernel.org # v4.1+
> > Signed-off-by: Seth Forshee <seth.fors...@canonical.com>
> > ---
> > fs/fuse/cuse.c | 12 ++++++++++--
> > fs/fuse/file.c | 42 ++++++++++++++++++++++++++++++++++--------
> > fs/fuse/fuse_i.h | 15 +++++++++++++++
> > 3 files changed, 59 insertions(+), 10 deletions(-)
> >
>
> [snip]
>
> > @@ -2864,6 +2882,7 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter
> > *iter, loff_t offset)
> > if (!io)
> > return -ENOMEM;
> > spin_lock_init(&io->lock);
> > + atomic_set(&io->refcnt, 1);
> > io->reqs = 1;
> > io->bytes = -1;
> > io->size = 0;
> > @@ -2887,8 +2906,15 @@ fuse_direct_IO(struct kiocb *iocb, struct iov_iter
> > *iter, loff_t offset)
> > iov_iter_rw(iter) == WRITE)
> > io->async = false;
> >
> > - if (io->async && is_sync)
> > - io->done = &wait;
> > + if (is_sync) {
> > + /*
> > + * Additional reference to keep io around after
> > + * calling fuse_aio_complete()
> > + */
> > + fuse_io_ref(io);
>
> AFAICS, the additional reference should only be needed for the io->async case,
> no?
That seems right, good catch.
> Updated, prettified patch below. Could you please test?
That looks good to me. I'll have to leave it to Robert or Martin to test
though as I could never reproduce the race.
Thanks,
Seth
