lkml.org 
[lkml]   [2013]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2013-01-15 15:41    [W:6.393 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site