Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Chao Yu <yuchao0@huawei.com>]

On 2018/4/3 13:23, Jaegeuk Kim wrote:
> On 04/03, Chao Yu wrote:
>> On 2018/3/31 0:30, Jaegeuk Kim wrote:
>>> Change log from v1:
>>>  - add more description
>>>
>>> This fixes xfstests/generic/392.
>>>
>>> The failure was caused by different times between 1) one marked in the last
>>> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
>>> The reason was that we skipped updating inode block at 1), since its i_size
>>> was recoverable along with 4KB-aligned data writes, which was fixed by:
>>>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
>>>  fs/f2fs/inode.c |  4 ++++
>>>  2 files changed, 19 insertions(+)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 000f93f6767e..675c39d85111 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>>>  	kprojid_t i_projid;		/* id for project quota */
>>>  	int i_inline_xattr_size;	/* inline xattr size */
>>>  	struct timespec i_crtime;	/* inode creation time */
>>> +	struct timespec i_disk_time[4];	/* inode disk times */
>>>  };
>>>  
>>>  static inline void get_extent_info(struct extent_info *ext,
>>> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
>>>  	f2fs_mark_inode_dirty_sync(inode, true);
>>>  }
>>>  
>>> +static inline bool time_equal(struct timespec a, struct timespec b)
>>> +{
>>> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
>>> +}
>>
>> We can use existing timespec_equal in <linux/time.h> instead of defining a
>> duplicated one?
> 
> It is defined with const parameters.

I didn't get it, const keyword can used to protect parameter not to be changed
in that function, that will be more safe, so it will be better to use that one?

Thanks,

> 
>>
>>> +
>>>  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>>>  {
>>>  	bool ret;
>>> @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>>>  			i_size_read(inode) & ~PAGE_MASK)
>>>  		return false;
>>>  
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
>>> +		return false;
>>> +
>>>  	down_read(&F2FS_I(inode)->i_sem);
>>>  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
>>>  	up_read(&F2FS_I(inode)->i_sem);
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 401f09ccce7e..70aba580f4b0 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
>>>  	if (inode->i_nlink == 0)
>>>  		clear_inline_node(node_page);
>>>  
>>> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
>>> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
>>> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
>>> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
>>
>> We need initialize them in do_read_inode?
> 
> Done.
> Thanks,
> 
>>
>> Thanks,
>>
>>>  }
>>>  
>>>  void update_inode_page(struct inode *inode)
>>>
> 
> .
>