From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751712AbdJZKDE (ORCPT ); Thu, 26 Oct 2017 06:03:04 -0400 Received: from mail.kernel.org ([198.145.29.99]:50008 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751160AbdJZKDB (ORCPT ); Thu, 26 Oct 2017 06:03:01 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A40B521913 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=jaegeuk@kernel.org Date: Thu, 26 Oct 2017 12:02:57 +0200 From: Jaegeuk Kim To: Chao Yu Cc: linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org, chao@kernel.org Subject: Re: [PATCH] f2fs: fix to keep backward compatibility of flexible inline xattr feature Message-ID: <20171026100257.GA60038@jaegeuk-macbookpro.roam.corp.google.com> References: <20171025103124.63162-1-yuchao0@huawei.com> <20171026084243.GA54331@jaegeuk-macbookpro.roam.corp.google.com> <37f397bd-fa5b-c7f0-bb20-ea3be98437cd@huawei.com> <20171026091108.GC54331@jaegeuk-macbookpro.roam.corp.google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20171026091108.GC54331@jaegeuk-macbookpro.roam.corp.google.com> User-Agent: Mutt/1.8.2 (2017-04-18) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 is set. Have you done some tests with this? I'm failing tests with these changes. > > > > 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 > > >> Signed-off-by: Chao Yu > > >> --- > > >> 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 > > > > > > . > > >