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. ;)
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 <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
> >>>>>>>>>>
> >>>>>>>>>> .
> >>>>>>>>>>
> >>>
> >>> .
> >>>
