Messages in this thread Patch in this message | | | Subject | Re: [PATCH] f2fs: fix to keep backward compatibility of flexible inline xattr feature | From | Chao Yu <> | Date | Fri, 27 Oct 2017 10:13:47 +0800 |
| |
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))) { 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)) { 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 * 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@huawei.com> >>>>>>>>> Signed-off-by: Chao Yu <yuchao0@huawei.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 >>>>>>>> >>>>>>>> . >>>>>>>> > > . >
| |