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?
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 <shengyo...@huawei.com>
> >>>>>>> Signed-off-by: Chao Yu <yuch...@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
> >>>>>>
> >>>>>> .
> >>>>>>
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
