From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao2.yu@samsung.com>
Cc: "'Wanpeng Li'" <wanpeng.li@linux.intel.com>,
"'Changman Lee'" <cm224.lee@samsung.com>,
linux-f2fs-devel@lists.sourceforge.net,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] f2fs: add fast symlink support
Date: Fri, 20 Mar 2015 13:13:39 -0700 [thread overview]
Message-ID: <20150320190501.GA19413@jaegeuk-mac02.mot.com> (raw)
In-Reply-To: <008701d061f3$55a0e7a0$00e2b6e0$@samsung.com>
Hi Wanpeng,
The Chao's patch is what I intended.
Could anyone of you enhance that patch?
If we use inline_data by default, we need another option to disable it.
Like "noinline_data"?
Thanks,
On Thu, Mar 19, 2015 at 11:17:17AM +0800, Chao Yu wrote:
> Hi Wanpeng,
>
> > -----Original Message-----
> > From: Wanpeng Li [mailto:wanpeng.li@linux.intel.com]
> > Sent: Thursday, March 19, 2015 7:02 AM
> > To: Jaegeuk Kim
> > Cc: Wanpeng Li; Changman Lee; Chao Yu; linux-f2fs-devel@lists.sourceforge.net;
> > linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v3] f2fs: add fast symlink support
> >
> > Hi Jaegeuk,
> > On Wed, Mar 18, 2015 at 11:05:28AM -0700, Jaegeuk Kim wrote:
> > >Hi,
> > >
> > >On Wed, Mar 18, 2015 at 04:58:52PM +0800, Wanpeng Li wrote:
> > >> Hi Jaegeuk,
> > >> On Tue, Mar 17, 2015 at 10:21:27AM -0700, Jaegeuk Kim wrote:
> > >> >> - err = page_symlink(inode, symname, symlen);
> > >> >> +
> > >> >> + if (symlen > MAX_FAST_SYMLINK_SIZE) {
> > >> >> + /* slow symlink */
> > >> >> + inode->i_op = &f2fs_symlink_inode_operations;
> > >> >> + inode->i_mapping->a_ops = &f2fs_dblock_aops;
> > >> >> + err = page_symlink(inode, symname, symlen);
> > >> >> + } else {
> > >> >> + /* fast symlink */
> > >> >> + struct page *node_page;
> > >> >> +
> > >> >> + inode->i_op = &f2fs_fast_symlink_inode_operations;
> > >> >> + node_page = get_node_page(sbi, inode->i_ino);
> > >> >> + memcpy((char *)&F2FS_INODE(node_page)->i_addr, symname, symlen);
> > >> >
> > >> >This is mostly likewise the inline_data flow.
> > >> >As of now, I can't recommend using any i_addr region, since we need to handle
> > >> >many cases such as truncation, roll-forward recovery, and fsck/dump tools.
> > >> >
> > >> >It is more sensible to enable inline_data by default, isn't it?
> > >>
> > >> Do you mean to replace the codes above by something like
> > >> f2fs_write_inline_data()?
> > >
> > >I meant the mount option, inline_data.
> > >Currently, f2fs doesn't set that option at mount time, but I thought that we
> > >can set it by default since it becomes stable.
> >
> > So I think you mean my codes should memcpy i_addr[1~872] instead of i_addr[0~872], right?
>
> I think what Jaegeuk means is that we can use inline_data in f2fs by default with
> patch like below firstly:
>
> >From a7320bfe94239ea4ceb193621a3a1a4d11a40d07 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao2.yu@samsung.com>
> Date: Thu, 19 Mar 2015 10:02:08 +0800
> Subject: [PATCH] f2fs: use inline_data by default
>
> Use inline_data feature by default since it brings us better performance & space
> utilization and now it is already stable.
>
> Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
> fs/f2fs/super.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index fc6857f..8772d91 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -991,6 +991,7 @@ try_onemore:
> sbi->active_logs = NR_CURSEG_TYPE;
>
> set_opt(sbi, BG_GC);
> + set_opt(sbi, INLINE_DATA);
>
> #ifdef CONFIG_F2FS_FS_XATTR
> set_opt(sbi, XATTR_USER);
> --
> 2.3.1
>
> Then we enable inline_data feature for symlink in f2fs_may_inline, after that
> by default our symlink created has inline_data flag in it, we treat small size
> symlink as an inline regular file, we can write its data page which is written
> in page_symlink into inode page like normal inline data procedure.
>
> Thanks,
>
> >
> > Regards,
> > Wanpeng Li
> >
> > >
> > >Thanks,
> > >
> > >>
> > >> Regards,
> > >> Wanpeng Li
> > >>
> > >> >
> > >> >Thanks,
> > >> >
> > >> >> + set_page_dirty(node_page);
> > >> >> + f2fs_put_page(node_page, 1);
> > >> >> + inode->i_size = symlen-1;
> > >> >> + set_inode_flag(F2FS_I(inode), FI_FAST_SYMLINK);
> > >> >> + mark_inode_dirty(inode);
> > >> >> + }
> > >> >> alloc_nid_done(sbi, inode->i_ino);
> > >> >>
> > >> >> d_instantiate(dentry, inode);
> > >> >> @@ -743,6 +774,20 @@ const struct inode_operations f2fs_symlink_inode_operations = {
> > >> >> #endif
> > >> >> };
> > >> >>
> > >> >> +const struct inode_operations f2fs_fast_symlink_inode_operations = {
> > >> >> + .readlink = generic_readlink,
> > >> >> + .follow_link = f2fs_follow_link,
> > >> >> + .put_link = page_put_link,
> > >> >> + .getattr = f2fs_getattr,
> > >> >> + .setattr = f2fs_setattr,
> > >> >> +#ifdef CONFIG_F2FS_FS_XATTR
> > >> >> + .setxattr = generic_setxattr,
> > >> >> + .getxattr = generic_getxattr,
> > >> >> + .listxattr = f2fs_listxattr,
> > >> >> + .removexattr = generic_removexattr,
> > >> >> +#endif
> > >> >> +};
> > >> >> +
> > >> >> const struct inode_operations f2fs_special_inode_operations = {
> > >> >> .getattr = f2fs_getattr,
> > >> >> .setattr = f2fs_setattr,
> > >> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > >> >> index 35a9117..efe28a2b 100644
> > >> >> --- a/fs/f2fs/node.c
> > >> >> +++ b/fs/f2fs/node.c
> > >> >> @@ -908,8 +908,9 @@ void remove_inode_page(struct inode *inode)
> > >> >> }
> > >> >>
> > >> >> /* remove potential inline_data blocks */
> > >> >> - if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > >> >> - S_ISLNK(inode->i_mode))
> > >> >> + if ((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > >> >> + S_ISLNK(inode->i_mode)) &&
> > >> >> + !f2fs_inode_is_fast_symlink(inode))
> > >> >> truncate_data_blocks_range(&dn, 1);
> > >> >>
> > >> >> /* 0 is possible, after f2fs_new_inode() has failed */
> > >> >> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> > >> >> index 502f28c..834880c 100644
> > >> >> --- a/include/linux/f2fs_fs.h
> > >> >> +++ b/include/linux/f2fs_fs.h
> > >> >> @@ -178,9 +178,11 @@ struct f2fs_extent {
> > >> >> #define F2FS_INLINE_DATA 0x02 /* file inline data flag */
> > >> >> #define F2FS_INLINE_DENTRY 0x04 /* file inline dentry flag */
> > >> >> #define F2FS_DATA_EXIST 0x08 /* file inline data exist flag */
> > >> >> +#define F2FS_FAST_SYMLINK 0x10 /* file fast symlink flag */
> > >> >>
> > >> >> #define MAX_INLINE_DATA (sizeof(__le32) * (DEF_ADDRS_PER_INODE - \
> > >> >> F2FS_INLINE_XATTR_ADDRS - 1))
> > >> >> +#define MAX_FAST_SYMLINK_SIZE (MAX_INLINE_DATA + 1)
> > >> >>
> > >> >> struct f2fs_inode {
> > >> >> __le16 i_mode; /* file mode */
> > >> >> --
> > >> >> 1.9.1
WARNING: multiple messages have this Message-ID (diff)
From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao2.yu@samsung.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [PATCH v3] f2fs: add fast symlink support
Date: Fri, 20 Mar 2015 13:13:39 -0700 [thread overview]
Message-ID: <20150320190501.GA19413@jaegeuk-mac02.mot.com> (raw)
In-Reply-To: <008701d061f3$55a0e7a0$00e2b6e0$@samsung.com>
Hi Wanpeng,
The Chao's patch is what I intended.
Could anyone of you enhance that patch?
If we use inline_data by default, we need another option to disable it.
Like "noinline_data"?
Thanks,
On Thu, Mar 19, 2015 at 11:17:17AM +0800, Chao Yu wrote:
> Hi Wanpeng,
>
> > -----Original Message-----
> > From: Wanpeng Li [mailto:wanpeng.li@linux.intel.com]
> > Sent: Thursday, March 19, 2015 7:02 AM
> > To: Jaegeuk Kim
> > Cc: Wanpeng Li; Changman Lee; Chao Yu; linux-f2fs-devel@lists.sourceforge.net;
> > linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH v3] f2fs: add fast symlink support
> >
> > Hi Jaegeuk,
> > On Wed, Mar 18, 2015 at 11:05:28AM -0700, Jaegeuk Kim wrote:
> > >Hi,
> > >
> > >On Wed, Mar 18, 2015 at 04:58:52PM +0800, Wanpeng Li wrote:
> > >> Hi Jaegeuk,
> > >> On Tue, Mar 17, 2015 at 10:21:27AM -0700, Jaegeuk Kim wrote:
> > >> >> - err = page_symlink(inode, symname, symlen);
> > >> >> +
> > >> >> + if (symlen > MAX_FAST_SYMLINK_SIZE) {
> > >> >> + /* slow symlink */
> > >> >> + inode->i_op = &f2fs_symlink_inode_operations;
> > >> >> + inode->i_mapping->a_ops = &f2fs_dblock_aops;
> > >> >> + err = page_symlink(inode, symname, symlen);
> > >> >> + } else {
> > >> >> + /* fast symlink */
> > >> >> + struct page *node_page;
> > >> >> +
> > >> >> + inode->i_op = &f2fs_fast_symlink_inode_operations;
> > >> >> + node_page = get_node_page(sbi, inode->i_ino);
> > >> >> + memcpy((char *)&F2FS_INODE(node_page)->i_addr, symname, symlen);
> > >> >
> > >> >This is mostly likewise the inline_data flow.
> > >> >As of now, I can't recommend using any i_addr region, since we need to handle
> > >> >many cases such as truncation, roll-forward recovery, and fsck/dump tools.
> > >> >
> > >> >It is more sensible to enable inline_data by default, isn't it?
> > >>
> > >> Do you mean to replace the codes above by something like
> > >> f2fs_write_inline_data()?
> > >
> > >I meant the mount option, inline_data.
> > >Currently, f2fs doesn't set that option at mount time, but I thought that we
> > >can set it by default since it becomes stable.
> >
> > So I think you mean my codes should memcpy i_addr[1~872] instead of i_addr[0~872], right?
>
> I think what Jaegeuk means is that we can use inline_data in f2fs by default with
> patch like below firstly:
>
> >From a7320bfe94239ea4ceb193621a3a1a4d11a40d07 Mon Sep 17 00:00:00 2001
> From: Chao Yu <chao2.yu@samsung.com>
> Date: Thu, 19 Mar 2015 10:02:08 +0800
> Subject: [PATCH] f2fs: use inline_data by default
>
> Use inline_data feature by default since it brings us better performance & space
> utilization and now it is already stable.
>
> Suggested-by: Jaegeuk Kim <jaegeuk@kernel.org>
> Signed-off-by: Chao Yu <chao2.yu@samsung.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
> fs/f2fs/super.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index fc6857f..8772d91 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -991,6 +991,7 @@ try_onemore:
> sbi->active_logs = NR_CURSEG_TYPE;
>
> set_opt(sbi, BG_GC);
> + set_opt(sbi, INLINE_DATA);
>
> #ifdef CONFIG_F2FS_FS_XATTR
> set_opt(sbi, XATTR_USER);
> --
> 2.3.1
>
> Then we enable inline_data feature for symlink in f2fs_may_inline, after that
> by default our symlink created has inline_data flag in it, we treat small size
> symlink as an inline regular file, we can write its data page which is written
> in page_symlink into inode page like normal inline data procedure.
>
> Thanks,
>
> >
> > Regards,
> > Wanpeng Li
> >
> > >
> > >Thanks,
> > >
> > >>
> > >> Regards,
> > >> Wanpeng Li
> > >>
> > >> >
> > >> >Thanks,
> > >> >
> > >> >> + set_page_dirty(node_page);
> > >> >> + f2fs_put_page(node_page, 1);
> > >> >> + inode->i_size = symlen-1;
> > >> >> + set_inode_flag(F2FS_I(inode), FI_FAST_SYMLINK);
> > >> >> + mark_inode_dirty(inode);
> > >> >> + }
> > >> >> alloc_nid_done(sbi, inode->i_ino);
> > >> >>
> > >> >> d_instantiate(dentry, inode);
> > >> >> @@ -743,6 +774,20 @@ const struct inode_operations f2fs_symlink_inode_operations = {
> > >> >> #endif
> > >> >> };
> > >> >>
> > >> >> +const struct inode_operations f2fs_fast_symlink_inode_operations = {
> > >> >> + .readlink = generic_readlink,
> > >> >> + .follow_link = f2fs_follow_link,
> > >> >> + .put_link = page_put_link,
> > >> >> + .getattr = f2fs_getattr,
> > >> >> + .setattr = f2fs_setattr,
> > >> >> +#ifdef CONFIG_F2FS_FS_XATTR
> > >> >> + .setxattr = generic_setxattr,
> > >> >> + .getxattr = generic_getxattr,
> > >> >> + .listxattr = f2fs_listxattr,
> > >> >> + .removexattr = generic_removexattr,
> > >> >> +#endif
> > >> >> +};
> > >> >> +
> > >> >> const struct inode_operations f2fs_special_inode_operations = {
> > >> >> .getattr = f2fs_getattr,
> > >> >> .setattr = f2fs_setattr,
> > >> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> > >> >> index 35a9117..efe28a2b 100644
> > >> >> --- a/fs/f2fs/node.c
> > >> >> +++ b/fs/f2fs/node.c
> > >> >> @@ -908,8 +908,9 @@ void remove_inode_page(struct inode *inode)
> > >> >> }
> > >> >>
> > >> >> /* remove potential inline_data blocks */
> > >> >> - if (S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > >> >> - S_ISLNK(inode->i_mode))
> > >> >> + if ((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> > >> >> + S_ISLNK(inode->i_mode)) &&
> > >> >> + !f2fs_inode_is_fast_symlink(inode))
> > >> >> truncate_data_blocks_range(&dn, 1);
> > >> >>
> > >> >> /* 0 is possible, after f2fs_new_inode() has failed */
> > >> >> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> > >> >> index 502f28c..834880c 100644
> > >> >> --- a/include/linux/f2fs_fs.h
> > >> >> +++ b/include/linux/f2fs_fs.h
> > >> >> @@ -178,9 +178,11 @@ struct f2fs_extent {
> > >> >> #define F2FS_INLINE_DATA 0x02 /* file inline data flag */
> > >> >> #define F2FS_INLINE_DENTRY 0x04 /* file inline dentry flag */
> > >> >> #define F2FS_DATA_EXIST 0x08 /* file inline data exist flag */
> > >> >> +#define F2FS_FAST_SYMLINK 0x10 /* file fast symlink flag */
> > >> >>
> > >> >> #define MAX_INLINE_DATA (sizeof(__le32) * (DEF_ADDRS_PER_INODE - \
> > >> >> F2FS_INLINE_XATTR_ADDRS - 1))
> > >> >> +#define MAX_FAST_SYMLINK_SIZE (MAX_INLINE_DATA + 1)
> > >> >>
> > >> >> struct f2fs_inode {
> > >> >> __le16 i_mode; /* file mode */
> > >> >> --
> > >> >> 1.9.1
------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the
conversation now. http://goparallel.sourceforge.net/
next prev parent reply other threads:[~2015-03-20 20:13 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-03-13 6:33 [PATCH v3] f2fs: add fast symlink support Wanpeng Li
2015-03-16 13:03 ` Chao Yu
2015-03-16 23:09 ` Wanpeng Li
2015-03-16 23:08 ` Wanpeng Li
2015-03-17 17:21 ` Jaegeuk Kim
2015-03-18 8:58 ` Wanpeng Li
2015-03-18 8:58 ` Wanpeng Li
2015-03-18 18:05 ` Jaegeuk Kim
2015-03-18 23:02 ` Wanpeng Li
2015-03-19 3:17 ` Chao Yu
2015-03-19 3:17 ` Chao Yu
2015-03-20 20:13 ` Jaegeuk Kim [this message]
2015-03-20 20:13 ` Jaegeuk Kim
2015-03-22 23:10 ` Wanpeng Li
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20150320190501.GA19413@jaegeuk-mac02.mot.com \
--to=jaegeuk@kernel.org \
--cc=chao2.yu@samsung.com \
--cc=cm224.lee@samsung.com \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=wanpeng.li@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.