Date:   Sat, 28 Oct 2017 09:56:59 +0800
From:   Chao Yu <yuchao0@...wei.com>
To:     Jaegeuk Kim <jaegeuk@...nel.org>
CC:     Chao Yu <chao@...nel.org>,
        <linux-f2fs-devel@...ts.sourceforge.net>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] f2fs: fix to keep backward compatibility of flexible
 inline xattr feature

On 2017/10/27 19:39, Chao Yu wrote:
> On 2017/10/27 19:32, Jaegeuk Kim wrote:
>> On 10/27, Chao Yu wrote:
>>> On 2017/10/27 18:56, Jaegeuk Kim wrote:
>>>> On 10/27, Chao Yu wrote:
>>>>> On 2017/10/26 22:05, Jaegeuk Kim wrote:
>>>>>> On 10/26, Chao Yu wrote:
>>>>>>> On 2017/10/26 19:52, Jaegeuk Kim wrote:
>>>>>>>> On 10/26, Chao Yu wrote:
>>>>>>>>> Hi Jaegeuk,
>>>>>>>>>
>>>>>>>>> On 2017/10/26 18:02, Jaegeuk Kim wrote:
>>>>>>>>>> Hi Chao,
>>>>>>>>>>
>>>>>>>>>> On 10/26, Jaegeuk Kim wrote:
>>>>>>>>>>> On 10/26, Chao Yu wrote:
>>>>>>>>>>>> Hi Jaegeuk,
>>>>>>>>>>>>
>>>>>>>>>>>> On 2017/10/26 16:42, Jaegeuk Kim wrote:
>>>>>>>>>>>>> Hi Chao,
>>>>>>>>>>>>>
>>>>>>>>>>>>> It seems this is a critical problem, so let me integrate this patch with your
>>>>>>>>>>>>> initial patch "f2fs: support flexible inline xattr size".
>>>>>>>>>>>>> Let me know, if you have any other concern.
>>>>>>>>>>>>
>>>>>>>>>>>> Better. ;)
>>>>>>>>>>>>
>>>>>>>>>>>> Please add commit message of this patch into initial patch "f2fs: support
>>>>>>>>>>>> flexible inline xattr size".
>>>>>>>>>>
>>>>>>>>>> BTW, I read the patch again, and couldn't catch the problem actually.
>>>>>>>>>> We didn't assign inline_xattr all the time, instead do if inline_xattr
>>>>>>>>>
>>>>>>>>> But you can see, MAX_INLINE_DATA is calculated as below, in where we will always
>>>>>>>>> reserve F2FS_INLINE_XATTR_ADDRS of 200 bytes, now we will change
>>>>>>>>> F2FS_INLINE_XATTR_ADDRS to F2FS_INLINE_XATTR_ADDRS(inode) which is an flexible
>>>>>>>>> size, so MAX_INLINE_DATA could be calculated to expand 200 bytes than before,
>>>>>>>>
>>>>>>>> That doesn't mean inline_addr is starting after 200 bytes. We're getting the
>>>>>>>> address only if inline_xattr is set, no? The below size seems reserving the
>>>>>>>
>>>>>>> This isn't about inline_xattr_addr, it is about MAX_INLINE_DATA size calculation.
>>>>>>>
>>>>>>> For example, in old image, inode is with inline_{data,dentry} flag, but without
>>>>>>> inline_xattr flag, it will only use 3688 - 200 bytes for storing inline data/dents,
>>>>>>> which means, it reserves 200 bytes.
>>>>>>>
>>>>>>> If we update kernel, get_inline_xattr_addrs will return 0 if inline_xattr flag is not
>>>>>>> set, so MAX_INLINE_DATA will be 3688, then inline_dentry page layout will change.
>>>>>>
>>>>>> Thanks. This makes much clear to me. It seems we need to handle directory only.
>>>>>> Could you please check the dev-test branches in f2fs and f2fs-tools?
>>>>>
>>>>> This is a little complicated, as for non-inline_{data,dentry} inode, we will get
>>>>> addrs number in inode by:
>>>>>
>>>>> static inline unsigned int addrs_per_inode(struct inode *inode)
>>>>> {
>>>>> 	if (f2fs_has_inline_xattr(inode))
>>>>> 		return CUR_ADDRS_PER_INODE(inode) - F2FS_INLINE_XATTR_ADDRS;
>>>>> 	return CUR_ADDRS_PER_INODE(inode);
>>>>> }
>>>>>
>>>>> It only cares about this inode is inline_xattr or not, so, if we reserve 200
>>>>> bytes for dir inode, it will cause incompatibility.
>>>>>
>>>>> So we need add this logic into i_inline_xattr_size calculation? Such as:
>>>>>
>>>>> a. new_inode:
>>>>>
>>>>> 	if (f2fs_sb_has_flexible_inline_xattr(sbi->sb)) {
>>>>> 		f2fs_bug_on(sbi, !f2fs_has_extra_attr(inode));
>>>>> 		if (f2fs_has_inline_xattr)
>>>>> 			xattr_size = sbi->inline_xattr_size;
>>>>> 	} else if (f2fs_has_inline_xattr(inode) ||
>>>>> 			(f2fs_has_inline_dentry(inode) && S_ISDIR(inode->i_mode))) {
>>>>
>>>> We don't need to check both of them. It'd be enough to check
>>>> f2fs_has_inline_dentry(inode).
>>>
>>> Agreed.
>>>
>>>>
>>>>> 		xattr = DEFAULT_INLINE_XATTR_ADDRS
>>>>> 	}
>>>>> 	F2FS_I(inode)->i_inline_xattr_size = xattr_size;
>>>>>
>>>>> b. do_read_inode
>>>>>
>>>>> 	if (f2fs_sb_has_flexible_inline_xattr(sbi->sb)) {
>>>>> 		f2fs_bug_on(sbi, !f2fs_has_extra_attr(inode));
>>>>> 		fi->i_inline_xattr_size = le16_to_cpu(ri->i_inline_xattr_size);
>>>>> 	} else if (f2fs_has_inline_xattr(inode) ||
>>>>> 			(f2fs_has_inline_dentry(inode) && S_ISDIR(inode->i_mode)) {
>>>>
>>>> Ditto.
>>>>
>>>> I merged the change in dev-test, so could you check that out again? ;)
>>>>
>>>> Thanks,
>>>>
>>>>> 		fi->i_inline_xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
>>>>> 	} else {
>>>>> 		/*
>>>>> 		 * Previous inline_data always reserved 200 bytes, even if
>>>>> 		 * inline_xattr is disabled. But only inline_data is safe
>>>
>>> Minor thing, how about:
>>>
>>> 	/*
>>> 	 * Previous inline data or directory always reserved 200 bytes in
>>> 	 * inode layout, even if inline_xattr is disabled, in order to
>>> 	 * stablize inline dentry's structure for backward compatibility,
>>> 	 * we only get back reserved space for inline data.
>>> 	 */
>>
>> I slightly changed yours and uploaded. ;)
> 
> Have checked, thanks. ;)

Could you fix f2fs-tools too, and change below out-of-update comments a bit, because
as we found, if inline_xattr is disabled, we always reserve 200 bytes only for
inline_{data,dentry}. :)

"Previously, in inode layout, we will always reserve 200 bytes for inline
xattr space no matter the inode enables inline xattr feature or not, due
to this reason, max inline size of inode is fixed, but now, if inline
xattr is not enabled, max inline size of inode will be enlarged by 200
bytes, for regular and symlink inode, it will be safe to reuse resevered
space as they are all zero, but for directory, we need to keep the
reservation for stablizing directory structure."

Thanks,

> 
> Thanks,
> 
>>
>> Thanks,
>>
>>>
>>> Otherwise code in dev-test looks good to me. ;)
>>>
>>> Thanks,
>>>
>>>>> 		 * to get back the space.
>>>>> 		 */
>>>>> 		fi->i_inline_xattr_size = 0;
>>>>> 	}
>>>>>
>>>>> How about this? IMO, it's better to keep codes similar in between new_inode
>>>>> and do_read_inode. :)
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>>
>>>>>>>> last 200 bytes which we just didn't use fully before.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>>>
>>>>>>>>> It is okay for regular or symlink inode generated in old kernel to enlarge
>>>>>>>>> MAX_INLINE_DATA in new kernel, as reserved 200 bytes in inode are zero all the
>>>>>>>>> time, for directory, it will be problematic because our layout of inline
>>>>>>>>> dentry page is fixed and calculated according to MAX_INLINE_DATA, so if
>>>>>>>>> MAX_INLINE_DATA is enlarged, fields offset in dentry structure will relocate,
>>>>>>>>> result in incompatibility.
>>>>>>>>>
>>>>>>>>> #define MAX_INLINE_DATA(inode)	(sizeof(__le32) * \
>>>>>>>>> 				(CUR_ADDRS_PER_INODE(inode) - \
>>>>>>>>> 				DEF_INLINE_RESERVED_SIZE - \
>>>>>>>>> 				F2FS_INLINE_XATTR_ADDRS))
>>>>>>>>>
>>>>>>>>>> is set. Have you done some tests with this? I'm failing tests with these
>>>>>>>>>> changes.
>>>>>>>>>
>>>>>>>>> Oh, sorry, I just tested with this patch without the modification of f2fs-tools,
>>>>>>>>> fstest didn't report any errors so far.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> And please fix related issue in f2fs-tools with below diff code:
>>>>>>>>>>>
>>>>>>>>>>> Okay, merged.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> ---
>>>>>>>>>>>>  include/f2fs_fs.h | 9 ++++++---
>>>>>>>>>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
>>>>>>>>>>>> index 2db7495804fd..a0e15ba85d97 100644
>>>>>>>>>>>> --- a/include/f2fs_fs.h
>>>>>>>>>>>> +++ b/include/f2fs_fs.h
>>>>>>>>>>>> @@ -1047,11 +1047,14 @@ static inline int __get_extra_isize(struct f2fs_inode *inode)
>>>>>>>>>>>>  extern struct f2fs_configuration c;
>>>>>>>>>>>>  static inline int get_inline_xattr_addrs(struct f2fs_inode *inode)
>>>>>>>>>>>>  {
>>>>>>>>>>>> -	if (!(inode->i_inline & F2FS_INLINE_XATTR))
>>>>>>>>>>>> +	if ((c.feature & cpu_to_le32(F2FS_FEATURE_EXTRA_ATTR)) &&
>>>>>>>>>>>> +		(c.feature & cpu_to_le32(F2FS_FEATURE_FLEXIBLE_INLINE_XATTR)))
>>>>>>>>>>>> +		return le16_to_cpu(inode->i_inline_xattr_size);
>>>>>>>>>>>> +	else if (!(inode->i_inline & F2FS_INLINE_XATTR) &&
>>>>>>>>>>>> +		(S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode)))
>>>>>>>>>>>>  		return 0;
>>>>>>>>>>>> -	if (!(c.feature & cpu_to_le32(F2FS_FEATURE_FLEXIBLE_INLINE_XATTR)))
>>>>>>>>>>>> +	else
>>>>>>>>>>>>  		return DEFAULT_INLINE_XATTR_ADDRS;
>>>>>>>>>>>> -	return le16_to_cpu(inode->i_inline_xattr_size);
>>>>>>>>>>>>  }
>>>>>>>>>>>>
>>>>>>>>>>>>  #define get_extra_isize(node)	__get_extra_isize(&node->i)
>>>>>>>>>>>> -- 
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>> Thanks,
>>>>>>>>>>>>>
>>>>>>>>>>>>> On 10/25, Chao Yu wrote:
>>>>>>>>>>>>>> Previously, in inode layout, we will always reserve 200 bytes for inline
>>>>>>>>>>>>>> xattr space no matter the inode enables inline xattr feature or not, due
>>>>>>>>>>>>>> to this reason, max inline size of inode is fixed, but now, if inline
>>>>>>>>>>>>>> xattr is not enabled, max inline size of inode will be enlarged by 200
>>>>>>>>>>>>>> bytes, for regular and symlink inode, it will be safe to reuse resevered
>>>>>>>>>>>>>> space as they are all zero, but for directory, we need to keep the
>>>>>>>>>>>>>> reservation for stablizing directory structure.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Reported-by: Sheng Yong <shengyong1@...wei.com>
>>>>>>>>>>>>>> Signed-off-by: Chao Yu <yuchao0@...wei.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>  fs/f2fs/f2fs.h  |  5 +----
>>>>>>>>>>>>>>  fs/f2fs/inode.c | 15 ++++++++++++---
>>>>>>>>>>>>>>  fs/f2fs/namei.c |  2 ++
>>>>>>>>>>>>>>  3 files changed, 15 insertions(+), 7 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>>>>>>>>>>> index 2af1d31ae74b..7ddd0d085e3b 100644
>>>>>>>>>>>>>> --- a/fs/f2fs/f2fs.h
>>>>>>>>>>>>>> +++ b/fs/f2fs/f2fs.h
>>>>>>>>>>>>>> @@ -2393,10 +2393,7 @@ static inline int get_extra_isize(struct inode *inode)
>>>>>>>>>>>>>>  static inline int f2fs_sb_has_flexible_inline_xattr(struct super_block *sb);
>>>>>>>>>>>>>>  static inline int get_inline_xattr_addrs(struct inode *inode)
>>>>>>>>>>>>>>  {
>>>>>>>>>>>>>> -	if (!f2fs_has_inline_xattr(inode))
>>>>>>>>>>>>>> -		return 0;
>>>>>>>>>>>>>> -	if (!f2fs_sb_has_flexible_inline_xattr(F2FS_I_SB(inode)->sb))
>>>>>>>>>>>>>> -		return DEFAULT_INLINE_XATTR_ADDRS;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>  	return F2FS_I(inode)->i_inline_xattr_size;
>>>>>>>>>>>>>>  }
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>>>>>>>>>>>>> index bb876737e653..7f31b22c9efa 100644
>>>>>>>>>>>>>> --- a/fs/f2fs/inode.c
>>>>>>>>>>>>>> +++ b/fs/f2fs/inode.c
>>>>>>>>>>>>>> @@ -232,10 +232,19 @@ static int do_read_inode(struct inode *inode)
>>>>>>>>>>>>>>  	fi->i_extra_isize = f2fs_has_extra_attr(inode) ?
>>>>>>>>>>>>>>  					le16_to_cpu(ri->i_extra_isize) : 0;
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>> -	if (!f2fs_has_inline_xattr(inode))
>>>>>>>>>>>>>> -		fi->i_inline_xattr_size = 0;
>>>>>>>>>>>>>> -	else if (f2fs_sb_has_flexible_inline_xattr(sbi->sb))
>>>>>>>>>>>>>> +	/*
>>>>>>>>>>>>>> +	 * Previously, we will always reserve DEFAULT_INLINE_XATTR_ADDRS size
>>>>>>>>>>>>>> +	 * space for inline xattr datas, if inline xattr is not enabled, we
>>>>>>>>>>>>>> +	 * can expect all zero in reserved area, so for regular or symlink,
>>>>>>>>>>>>>> +	 * it will be safe to reuse reserved area, but for directory, we
>>>>>>>>>>>>>> +	 * should keep the reservation for stablizing directory structure.
>>>>>>>>>>>>>> +	 */
>>>>>>>>>>>>>> +	if (f2fs_has_extra_attr(inode) &&
>>>>>>>>>>>>>> +		f2fs_sb_has_flexible_inline_xattr(sbi->sb))
>>>>>>>>>>>>>>  		fi->i_inline_xattr_size = le16_to_cpu(ri->i_inline_xattr_size);
>>>>>>>>>>>>>> +	else if (!f2fs_has_inline_xattr(inode) &&
>>>>>>>>>>>>>> +		(S_ISREG(inode->i_mode) || S_ISLNK(inode->i_mode)))
>>>>>>>>>>>>>> +		fi->i_inline_xattr_size = 0;
>>>>>>>>>>>>>>  	else
>>>>>>>>>>>>>>  		fi->i_inline_xattr_size = DEFAULT_INLINE_XATTR_ADDRS;
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>>>>>>>>>>>>>> index e6f86d5d97b9..a1c56a14c191 100644
>>>>>>>>>>>>>> --- a/fs/f2fs/namei.c
>>>>>>>>>>>>>> +++ b/fs/f2fs/namei.c
>>>>>>>>>>>>>> @@ -91,6 +91,8 @@ static struct inode *f2fs_new_inode(struct inode *dir, umode_t mode)
>>>>>>>>>>>>>>  		f2fs_sb_has_flexible_inline_xattr(sbi->sb) &&
>>>>>>>>>>>>>>  		f2fs_has_inline_xattr(inode))
>>>>>>>>>>>>>>  		F2FS_I(inode)->i_inline_xattr_size = sbi->inline_xattr_size;
>>>>>>>>>>>>>> +	else
>>>>>>>>>>>>>> +		F2FS_I(inode)->i_inline_xattr_size = 0;
>>>>>>>>>>>>>>  
>>>>>>>>>>>>>>  	if (test_opt(sbi, INLINE_DATA) && f2fs_may_inline_data(inode))
>>>>>>>>>>>>>>  		set_inode_flag(inode, FI_INLINE_DATA);
>>>>>>>>>>>>>> -- 
>>>>>>>>>>>>>> 2.13.1.388.g69e6b9b4f4a9
>>>>>>>>>>>>>
>>>>>>>>>>>>> .
>>>>>>>>>>>>>
>>>>>>
>>>>>> .
>>>>>>
> 
> .
> 

Powered by blists - more mailing lists