Messages in this thread | | | Date | Tue, 15 Jan 2013 18:04:12 +0400 | From | "Maxim V. Patlasov" <> | Subject | Re: [PATCH 3/5] fuse: wait for end of IO on release |
| |
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@parallels.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |