Re: [PATCH 3/5] fuse: wait for end of IO on release

[Maxim V. Patlasov]

Hi Brian,

01/03/2013 12:35 AM, Brian Foster ÐÐÑÐÑ:
> On 12/20/2012 07:31 AM, Maxim Patlasov wrote:
> > There are two types of I/O activity that can be "in progress" at the time
> > of fuse_release() execution: asynchronous read-ahead and write-back. The
> > patch ensures that they are completed before fuse_release_common sends
> > FUSE_RELEASE to userspace.
> >
> > So far as fuse_release() waits for end of async I/O, its callbacks
> > (fuse_readpages_end and fuse_writepage_finish) calling fuse_file_put cannot
> > be the last holders of fuse file anymore. To emphasize the fact, the patch
> > replaces fuse_file_put with __fuse_file_put there.
> >
> > Signed-off-by: Maxim Patlasov <mpatlasov@xxxxxxxxxxxxx>
> > ---
> >   fs/fuse/file.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
> >   1 files changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 4f23134..aed9be2 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -137,6 +137,12 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
> >   	}
> >   }
> >   
> > +static void __fuse_file_put(struct fuse_file *ff)
> > +{
> > +	if (atomic_dec_and_test(&ff->count))
> > +		BUG();
> > +}
> > +
> I think a comment in or before this function to explain the reasoning
> for the BUG would be helpful.
>
> >   int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
> >   		 bool isdir)
> >   {
> > @@ -260,7 +266,12 @@ void fuse_release_common(struct file *file, int opcode)
> >   	 * Make the release synchronous if this is a fuseblk mount,
> >   	 * synchronous RELEASE is allowed (and desirable) in this case
> >   	 * because the server can be trusted not to screw up.
> > +	 *
> > +	 * We might wait for them (asynchronous READ or WRITE requests), so:
> >   	 */
> > +	if (ff->fc->close_wait)
> > +		BUG_ON(atomic_read(&ff->count) != 1);
> > +
> It might be cleaner to pull the new part of the comment and the BUG_ON()
> check to before the existing comment and fuse_file_put (e.g., create a
> new comment).
>
> >   	fuse_file_put(ff, ff->fc->destroy_req != NULL);
> >   }
> >   
> > @@ -271,6 +282,31 @@ static int fuse_open(struct inode *inode, struct file *file)
> >   
> >   static int fuse_release(struct inode *inode, struct file *file)
> >   {
> > +	struct fuse_file *ff = file->private_data;
> > +
> > +	if (ff->fc->close_wait) {
> > +		struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > +		/*
> > +		 * Must remove file from write list. Otherwise it is possible
> > +		 * this file will get more writeback from another files
> > +		 * rerouted via write_files.
> > +		 */
> > +		spin_lock(&ff->fc->lock);
> > +		list_del_init(&ff->write_entry);
> > +		spin_unlock(&ff->fc->lock);
> > +
> > +		wait_event(fi->page_waitq, atomic_read(&ff->count) == 1);
> > +
> > +		/*
> > +		 * Wait for threads just released ff to leave their critical
> > +		 * sections. Taking spinlock is the first thing
> > +		 * fuse_release_common does, so that this is unnecessary, but
> > +		 * it is still good to emphasize right here, that we need this.
> > +		 */
> > +		spin_unlock_wait(&ff->fc->lock);
> I'm all for clarity, but if the wait is unnecessary, perhaps just leave
> the comment..? Just my .02.
>
> Aside from the few nits here, the set looks pretty good to me.

Thanks for review, the suggestions look reasonable. I'll resend 
corrected patch soon.

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