On 2014-12-01 18:13, Ryusuke Konishi wrote:
> Andreas,
> On Sun, 9 Nov 2014 17:00:12 +0100, Andreas Rohner wrote:
>> This patch removes filemap_write_and_wait_range() from
>> nilfs_sync_file(), because it triggers a data segment construction by
>> calling nilfs_writepages() with WB_SYNC_ALL. A data segment construction
>> does not remove the inode from the i_dirty list and it does not clear
>> the NILFS_I_DIRTY flag. Therefore nilfs_inode_dirty() still returns
>> true, which leads to an unnecessary duplicate segment construction in
>> nilfs_sync_file().
>>
>> A call to filemap_write_and_wait_range() is not needed, because NILFS2
>> does not rely on the generic writeback mechanisms. Instead it implements
>> its own mechanism to collect all dirty pages and write them into
>> segments. It is more efficient to initiate the segment construction
>> directly in nilfs_sync_file() without the detour over
>> filemap_write_and_wait_range().
>>
>> Additionally the lock of i_mutex is not needed, because all code blocks
>> that are protected by i_mutex are also protected by a NILFS transaction:
>>
>> Function i_mutex nilfs_transaction
>> ------------------------------------------------------
>> nilfs_ioctl_setflags: yes yes
>> nilfs_fiemap: yes no
>> nilfs_write_begin: yes yes
>> nilfs_write_end: yes yes
>> nilfs_lookup: yes no
>> nilfs_create: yes yes
>> nilfs_link: yes yes
>> nilfs_mknod: yes yes
>> nilfs_symlink: yes yes
>> nilfs_mkdir: yes yes
>> nilfs_unlink: yes yes
>> nilfs_rmdir: yes yes
>> nilfs_rename: yes yes
>> nilfs_setattr: yes yes
>>
>> For nilfs_lookup() i_mutex is held for the parent directory, to protect
>> it from modification. The segment construction does not modify directory
>> inodes, so no lock is needed.
>>
>> nilfs_fiemap() reads the block layout on the disk, by using
>> nilfs_bmap_lookup_contig(). This is already protected by bmap->b_sem.
>>
>> Signed-off-by: Andreas Rohner <andreas.rohner@xxxxxxx>
>> ---
>> fs/nilfs2/file.c | 21 ++++++++-------------
>> 1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
>> index e9e3325..1ad6bdf 100644
>> --- a/fs/nilfs2/file.c
>> +++ b/fs/nilfs2/file.c
>> @@ -41,19 +41,14 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
>> struct inode *inode = file->f_mapping->host;
>> int err;
>>
>> - err = filemap_write_and_wait_range(inode->i_mapping, start, end);
>> - if (err)
>> - return err;
>> - mutex_lock(&inode->i_mutex);
>> -
>> - if (nilfs_inode_dirty(inode)) {
>> - if (datasync)
>> - err = nilfs_construct_dsync_segment(inode->i_sb, inode,
>> - 0, LLONG_MAX);
>> - else
>> - err = nilfs_construct_segment(inode->i_sb);
>> - }
>> - mutex_unlock(&inode->i_mutex);
>
>> + if (!nilfs_inode_dirty(inode))
>> + return 0;
>
> I just noticed that this transformation is not equivalent to the
> original one. With this patch, nilfs_flush_device() is not called if
> nilfs_inode_dirty() is not true, which looks to be causing another
> data integrity issue.
>
> Could you reconsider if the above check is correct or not ?
Yes you are right. I thought, that no flush would be necessary in that
case, but it clearly is. Sorry for that mistake. I will send in a fixed
version of the patch.
Regards,
Andreas Rohner
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
