Date: Mon, 14 Mar 2016 10:12:54 -0500
From: Seth Forshee <seth.forshee@canonical.com>
To: Miklos Szeredi <miklos@szeredi.hu>
Cc: m.loschwitz@syseleven.de, robert@quobyte.com,
        fuse-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 2/2] fuse: Add reference counting for fuse_io_priv
Message-ID: <20160314151254.GA84964@ubuntu-hedt>
References: <1457714135-50289-1-git-send-email-seth.forshee@canonical.com>
 <1457714135-50289-3-git-send-email-seth.forshee@canonical.com>
 <20160314135311.GN8655@tucsk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20160314135311.GN8655@tucsk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2779
Lines: 71

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: stable@vger.kernel.org # v4.1+
> > Signed-off-by: Seth Forshee <seth.forshee@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