linux-kernel - Re: [PATCH] fs: push i_mutex and filemap_write_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4E0B63B5.5070404@redhat.com>
Date:	Wed, 29 Jun 2011 13:41:09 -0400
From:	Josef Bacik <josef@...hat.com>
To:	Jan Kara <jack@...e.cz>
CC:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	viro@...IV.linux.org.uk, hch@...radead.org
Subject: Re: [PATCH] fs: push i_mutex and filemap_write_and_wait down into
 ->fsync() handlers

On 06/29/2011 12:38 PM, Jan Kara wrote:
> On Tue 28-06-11 11:35:10, Josef Bacik wrote:
>> Btrfs needs to be able to control how filemap_write_and_wait_range() is called
>> in fsync to make it less of a painful operation, so push down taking i_mutex and
>> the calling of filemap_write_and_wait() down into the ->fsync() handlers.  Some
>> file systems can drop taking the i_mutex altogether it seems, like ext3 and
>> ocfs2.  For correctness sake I just pushed everything down in all cases to make
>> sure that we keep the current behavior the same for everybody, and then each
>> individual fs maintainer can make up their mind about what to do from there.
>> Thanks,
>>
>> Signed-off-by: Josef Bacik <josef@...hat.com>
> ...
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 1921392..fa44df8 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
>> @@ -1758,7 +1758,7 @@ extern int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
>>  extern void ext4_htree_free_dir_info(struct dir_private_info *p);
>>  
>>  /* fsync.c */
>> -extern int ext4_sync_file(struct file *, int);
>> +extern int ext4_sync_file(struct file *, loff_t, loff_t, int);
>>  extern int ext4_flush_completed_IO(struct inode *);
>>  
>>  /* hash.c */
>> diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
>> index ce66d2f..afab2e4 100644
>> --- a/fs/ext4/fsync.c
>> +++ b/fs/ext4/fsync.c
>> @@ -165,7 +165,7 @@ static int ext4_sync_parent(struct inode *inode)
>>   * i_mutex lock is held when entering and exiting this function
>>   */
>>  
>> -int ext4_sync_file(struct file *file, int datasync)
>> +int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>>  {
>>  	struct inode *inode = file->f_mapping->host;
>>  	struct ext4_inode_info *ei = EXT4_I(inode);
>> @@ -178,15 +178,22 @@ int ext4_sync_file(struct file *file, int datasync)
>>  
>>  	trace_ext4_sync_file_enter(file, datasync);
>>  
>> +	ret = filemap_write_and_wait_range(inode->i_mapping, start, end);
>> +	if (ret)
>> +		return ret;
>> +	mutex_lock(&inode->i_mutex);
>> +
>>  	if (inode->i_sb->s_flags & MS_RDONLY)
>> -		return 0;
>> +		goto out;
>>  
>>  	ret = ext4_flush_completed_IO(inode);
>>  	if (ret < 0)
>>  		goto out;
>>  
>>  	if (!journal) {
>> -		ret = generic_file_fsync(file, datasync);
>> +		if (inode->i_state & I_DIRTY ||
>> +		    (datasync && (inode->i_state & I_DIRTY_DATASYNC)))
>> +			ret = sync_inode_metadata(inode, 1);
>   Umm, how did you get to this change? It is actually wrong - ext4 in
> nojournal mode really seems to need to call generic_file_fsync() (so that
> sync_buffers_list() is called).
> 

Ah oops sorry about that, I will fix that, I was trying to be clever and
encapsulate generic_file_fsync()'s behavior into here but I missed the
sync_mapping_buffers() call.  Thanks,

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