LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v3] f2fs: add fast symlink support
@ 2015-03-13  6:33 Wanpeng Li
  2015-03-16 13:03 ` Chao Yu
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Wanpeng Li @ 2015-03-13  6:33 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Changman Lee, Chao Yu, linux-f2fs-devel, linux-fsdevel,
	linux-kernel, Wanpeng Li

This patch introduces the improvement fast symlinks to allow storage of
the target path within inode, thus symlinks with short target paths are
more accessed quickly. It will fall back to using the original slow
symlink if the target path exceeds the available inode space.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
v2 -> v3:
 * handle fast symlink in remove_inode_page
v1 -> v2:
 * set max size of fast symlink according to inline_xattr flag

 fs/f2fs/f2fs.h          | 15 ++++++++++++++
 fs/f2fs/file.c          |  2 ++
 fs/f2fs/inode.c         |  9 +++++++--
 fs/f2fs/namei.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++----
 fs/f2fs/node.c          |  5 +++--
 include/linux/f2fs_fs.h |  2 ++
 6 files changed, 78 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 511d6cd..3012ea0 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1226,6 +1226,7 @@ enum {
 	FI_VOLATILE_FILE,	/* indicate volatile file */
 	FI_DROP_CACHE,		/* drop dirty page cache */
 	FI_DATA_EXIST,		/* indicate data exists */
+	FI_FAST_SYMLINK,        /* indicate fast symlink */
 };
 
 static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
@@ -1262,6 +1263,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
 		set_inode_flag(fi, FI_INLINE_DENTRY);
 	if (ri->i_inline & F2FS_DATA_EXIST)
 		set_inode_flag(fi, FI_DATA_EXIST);
+	if (ri->i_inline & F2FS_FAST_SYMLINK)
+		set_inode_flag(fi, FI_FAST_SYMLINK);
 }
 
 static inline void set_raw_inline(struct f2fs_inode_info *fi,
@@ -1277,6 +1280,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,
 		ri->i_inline |= F2FS_INLINE_DENTRY;
 	if (is_inode_flag_set(fi, FI_DATA_EXIST))
 		ri->i_inline |= F2FS_DATA_EXIST;
+	if (is_inode_flag_set(fi, FI_FAST_SYMLINK))
+		ri->i_inline |= F2FS_FAST_SYMLINK;
 }
 
 static inline int f2fs_has_inline_xattr(struct inode *inode)
@@ -1370,6 +1375,15 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
 	sbi->sb->s_flags |= MS_RDONLY;
 }
 
+/*
+ * Test whether an inode is a fast symlink.
+ */
+static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
+{
+	return (S_ISLNK(inode->i_mode) &&
+			is_inode_flag_set(F2FS_I(inode), FI_FAST_SYMLINK));
+}
+
 #define get_inode_mode(i) \
 	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
 	 (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
@@ -1746,6 +1760,7 @@ extern const struct address_space_operations f2fs_node_aops;
 extern const struct address_space_operations f2fs_meta_aops;
 extern const struct inode_operations f2fs_dir_inode_operations;
 extern const struct inode_operations f2fs_symlink_inode_operations;
+extern const struct inode_operations f2fs_fast_symlink_inode_operations;
 extern const struct inode_operations f2fs_special_inode_operations;
 extern struct kmem_cache *inode_entry_slab;
 
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index f1341c7..05a3d4f 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -546,6 +546,8 @@ void f2fs_truncate(struct inode *inode)
 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
 				S_ISLNK(inode->i_mode)))
 		return;
+	if (f2fs_inode_is_fast_symlink(inode))
+		return;
 
 	trace_f2fs_truncate(inode);
 
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index b508744..ec9ad22 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -13,6 +13,7 @@
 #include <linux/buffer_head.h>
 #include <linux/writeback.h>
 #include <linux/bitops.h>
+#include <linux/namei.h>
 
 #include "f2fs.h"
 #include "node.h"
@@ -188,8 +189,12 @@ make_now:
 		inode->i_mapping->a_ops = &f2fs_dblock_aops;
 		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
 	} else if (S_ISLNK(inode->i_mode)) {
-		inode->i_op = &f2fs_symlink_inode_operations;
-		inode->i_mapping->a_ops = &f2fs_dblock_aops;
+		if (f2fs_inode_is_fast_symlink(inode))
+			inode->i_op = &f2fs_fast_symlink_inode_operations;
+		else {
+			inode->i_op = &f2fs_symlink_inode_operations;
+			inode->i_mapping->a_ops = &f2fs_dblock_aops;
+		}
 	} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
 			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
 		inode->i_op = &f2fs_special_inode_operations;
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 1e2ae21..0619909 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -14,6 +14,7 @@
 #include <linux/sched.h>
 #include <linux/ctype.h>
 #include <linux/dcache.h>
+#include <linux/namei.h>
 
 #include "f2fs.h"
 #include "node.h"
@@ -247,6 +248,21 @@ fail:
 	return err;
 }
 
+static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
+{
+	struct page *node_page;
+
+	node_page = get_node_page(F2FS_I_SB(dentry->d_inode),
+				dentry->d_inode->i_ino);
+	if (IS_ERR(node_page)) {
+		nd_set_link(nd, NULL);
+		return NULL;
+	}
+	nd_set_link(nd, (char *)(F2FS_INODE(node_page)->i_addr));
+	f2fs_put_page(node_page, 1);
+	return NULL;
+}
+
 static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 					const char *symname)
 {
@@ -261,16 +277,31 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
 	if (IS_ERR(inode))
 		return PTR_ERR(inode);
 
-	inode->i_op = &f2fs_symlink_inode_operations;
-	inode->i_mapping->a_ops = &f2fs_dblock_aops;
-
 	f2fs_lock_op(sbi);
 	err = f2fs_add_link(dentry, inode);
 	if (err)
 		goto out;
 	f2fs_unlock_op(sbi);
 
-	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);
+		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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v3] f2fs: add fast symlink support
  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
  2 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2015-03-16 13:03 UTC (permalink / raw)
  To: 'Wanpeng Li', 'Jaegeuk Kim'
  Cc: 'Changman Lee', linux-f2fs-devel, linux-fsdevel, linux-kernel

Hi Wanpeng, Jaegeuk,

> -----Original Message-----
> From: Wanpeng Li [mailto:wanpeng.li@linux.intel.com]
> Sent: Friday, March 13, 2015 2:34 PM
> To: Jaegeuk Kim
> Cc: Changman Lee; Chao Yu; linux-f2fs-devel@lists.sourceforge.net;
> linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; Wanpeng Li
> Subject: [PATCH v3] f2fs: add fast symlink support
> 
> This patch introduces the improvement fast symlinks to allow storage of
> the target path within inode, thus symlinks with short target paths are
> more accessed quickly. It will fall back to using the original slow
> symlink if the target path exceeds the available inode space.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
> v2 -> v3:
>  * handle fast symlink in remove_inode_page
> v1 -> v2:
>  * set max size of fast symlink according to inline_xattr flag
> 
>  fs/f2fs/f2fs.h          | 15 ++++++++++++++
>  fs/f2fs/file.c          |  2 ++
>  fs/f2fs/inode.c         |  9 +++++++--
>  fs/f2fs/namei.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++----
>  fs/f2fs/node.c          |  5 +++--
>  include/linux/f2fs_fs.h |  2 ++
>  6 files changed, 78 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 511d6cd..3012ea0 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1226,6 +1226,7 @@ enum {
>  	FI_VOLATILE_FILE,	/* indicate volatile file */
>  	FI_DROP_CACHE,		/* drop dirty page cache */
>  	FI_DATA_EXIST,		/* indicate data exists */
> +	FI_FAST_SYMLINK,        /* indicate fast symlink */
>  };
> 
>  static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
> @@ -1262,6 +1263,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
>  		set_inode_flag(fi, FI_INLINE_DENTRY);
>  	if (ri->i_inline & F2FS_DATA_EXIST)
>  		set_inode_flag(fi, FI_DATA_EXIST);
> +	if (ri->i_inline & F2FS_FAST_SYMLINK)
> +		set_inode_flag(fi, FI_FAST_SYMLINK);
>  }
> 
>  static inline void set_raw_inline(struct f2fs_inode_info *fi,
> @@ -1277,6 +1280,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,
>  		ri->i_inline |= F2FS_INLINE_DENTRY;
>  	if (is_inode_flag_set(fi, FI_DATA_EXIST))
>  		ri->i_inline |= F2FS_DATA_EXIST;
> +	if (is_inode_flag_set(fi, FI_FAST_SYMLINK))
> +		ri->i_inline |= F2FS_FAST_SYMLINK;
>  }
> 
>  static inline int f2fs_has_inline_xattr(struct inode *inode)
> @@ -1370,6 +1375,15 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
>  	sbi->sb->s_flags |= MS_RDONLY;
>  }
> 
> +/*
> + * Test whether an inode is a fast symlink.
> + */
> +static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
> +{
> +	return (S_ISLNK(inode->i_mode) &&
> +			is_inode_flag_set(F2FS_I(inode), FI_FAST_SYMLINK));

Look at this again, it seems that it's not necessary to introduce FI_FAST_SYMLINK
& F2FS_FAST_SYNLINK as we waste one bit in i_inline flag, for fast symlink, using
your original design will be enough until we consider xattr node block case.

static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
{
	return (S_ISLNK(inode->i_mode) && !F2FS_HAS_BLOCKS(inode));
}

Wanpeng, how about waiting for Jaegeuk's opinion?

> +}
> +
>  #define get_inode_mode(i) \
>  	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
>  	 (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
> @@ -1746,6 +1760,7 @@ extern const struct address_space_operations f2fs_node_aops;
>  extern const struct address_space_operations f2fs_meta_aops;
>  extern const struct inode_operations f2fs_dir_inode_operations;
>  extern const struct inode_operations f2fs_symlink_inode_operations;
> +extern const struct inode_operations f2fs_fast_symlink_inode_operations;
>  extern const struct inode_operations f2fs_special_inode_operations;
>  extern struct kmem_cache *inode_entry_slab;
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f1341c7..05a3d4f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -546,6 +546,8 @@ void f2fs_truncate(struct inode *inode)
>  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
>  				S_ISLNK(inode->i_mode)))
>  		return;
> +	if (f2fs_inode_is_fast_symlink(inode))
> +		return;
> 
>  	trace_f2fs_truncate(inode);
> 
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index b508744..ec9ad22 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -13,6 +13,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/writeback.h>
>  #include <linux/bitops.h>
> +#include <linux/namei.h>
> 
>  #include "f2fs.h"
>  #include "node.h"
> @@ -188,8 +189,12 @@ make_now:
>  		inode->i_mapping->a_ops = &f2fs_dblock_aops;
>  		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>  	} else if (S_ISLNK(inode->i_mode)) {
> -		inode->i_op = &f2fs_symlink_inode_operations;
> -		inode->i_mapping->a_ops = &f2fs_dblock_aops;
> +		if (f2fs_inode_is_fast_symlink(inode))
> +			inode->i_op = &f2fs_fast_symlink_inode_operations;
> +		else {
> +			inode->i_op = &f2fs_symlink_inode_operations;
> +			inode->i_mapping->a_ops = &f2fs_dblock_aops;
> +		}
>  	} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
>  			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
>  		inode->i_op = &f2fs_special_inode_operations;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 1e2ae21..0619909 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched.h>
>  #include <linux/ctype.h>
>  #include <linux/dcache.h>
> +#include <linux/namei.h>
> 
>  #include "f2fs.h"
>  #include "node.h"
> @@ -247,6 +248,21 @@ fail:
>  	return err;
>  }
> 
> +static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
> +{
> +	struct page *node_page;
> +
> +	node_page = get_node_page(F2FS_I_SB(dentry->d_inode),
> +				dentry->d_inode->i_ino);
> +	if (IS_ERR(node_page)) {
> +		nd_set_link(nd, NULL);
> +		return NULL;
> +	}
> +	nd_set_link(nd, (char *)(F2FS_INODE(node_page)->i_addr));
> +	f2fs_put_page(node_page, 1);
> +	return NULL;
> +}

The only different of i_op for fast symlink and normal symlink is that
we use different callback function for ->follow_link, if we merge
page_follow_link_light & f2fs_follow_link into one, we can remove duplicated
f2fs_fast_symlink_inode_operations. How do you think of it?

> +
>  static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>  					const char *symname)
>  {
> @@ -261,16 +277,31 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>  	if (IS_ERR(inode))
>  		return PTR_ERR(inode);
> 
> -	inode->i_op = &f2fs_symlink_inode_operations;
> -	inode->i_mapping->a_ops = &f2fs_dblock_aops;
> -
>  	f2fs_lock_op(sbi);
>  	err = f2fs_add_link(dentry, inode);
>  	if (err)
>  		goto out;
>  	f2fs_unlock_op(sbi);
> 
> -	err = page_symlink(inode, symname, symlen);
> +
> +	if (symlen > MAX_FAST_SYMLINK_SIZE) {

I think we should be pick up max_fast_symlink_size which is introduced in v2.

> +		/* 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);
> +		set_page_dirty(node_page);
> +		f2fs_put_page(node_page, 1);
> +		inode->i_size = symlen-1;

trivial coding style issue.
inode->i_size = symlen - 1;

Thanks,

> +		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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  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:08 ` Wanpeng Li
  2015-03-17 17:21 ` Jaegeuk Kim
  2 siblings, 0 replies; 11+ messages in thread
From: Wanpeng Li @ 2015-03-16 23:08 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Changman Lee, Chao Yu, linux-f2fs-devel, linux-fsdevel,
	linux-kernel, Wanpeng Li

Ping Jaegeuk,
On Fri, Mar 13, 2015 at 02:33:39PM +0800, Wanpeng Li wrote:
>This patch introduces the improvement fast symlinks to allow storage of
>the target path within inode, thus symlinks with short target paths are
>more accessed quickly. It will fall back to using the original slow
>symlink if the target path exceeds the available inode space.
>
>Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>---
>v2 -> v3:
> * handle fast symlink in remove_inode_page
>v1 -> v2:
> * set max size of fast symlink according to inline_xattr flag
>
> fs/f2fs/f2fs.h          | 15 ++++++++++++++
> fs/f2fs/file.c          |  2 ++
> fs/f2fs/inode.c         |  9 +++++++--
> fs/f2fs/namei.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++----
> fs/f2fs/node.c          |  5 +++--
> include/linux/f2fs_fs.h |  2 ++
> 6 files changed, 78 insertions(+), 8 deletions(-)
>
>diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>index 511d6cd..3012ea0 100644
>--- a/fs/f2fs/f2fs.h
>+++ b/fs/f2fs/f2fs.h
>@@ -1226,6 +1226,7 @@ enum {
> 	FI_VOLATILE_FILE,	/* indicate volatile file */
> 	FI_DROP_CACHE,		/* drop dirty page cache */
> 	FI_DATA_EXIST,		/* indicate data exists */
>+	FI_FAST_SYMLINK,        /* indicate fast symlink */
> };
> 
> static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
>@@ -1262,6 +1263,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
> 		set_inode_flag(fi, FI_INLINE_DENTRY);
> 	if (ri->i_inline & F2FS_DATA_EXIST)
> 		set_inode_flag(fi, FI_DATA_EXIST);
>+	if (ri->i_inline & F2FS_FAST_SYMLINK)
>+		set_inode_flag(fi, FI_FAST_SYMLINK);
> }
> 
> static inline void set_raw_inline(struct f2fs_inode_info *fi,
>@@ -1277,6 +1280,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,
> 		ri->i_inline |= F2FS_INLINE_DENTRY;
> 	if (is_inode_flag_set(fi, FI_DATA_EXIST))
> 		ri->i_inline |= F2FS_DATA_EXIST;
>+	if (is_inode_flag_set(fi, FI_FAST_SYMLINK))
>+		ri->i_inline |= F2FS_FAST_SYMLINK;
> }
> 
> static inline int f2fs_has_inline_xattr(struct inode *inode)
>@@ -1370,6 +1375,15 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
> 	sbi->sb->s_flags |= MS_RDONLY;
> }
> 
>+/*
>+ * Test whether an inode is a fast symlink.
>+ */
>+static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
>+{
>+	return (S_ISLNK(inode->i_mode) &&
>+			is_inode_flag_set(F2FS_I(inode), FI_FAST_SYMLINK));
>+}
>+
> #define get_inode_mode(i) \
> 	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
> 	 (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
>@@ -1746,6 +1760,7 @@ extern const struct address_space_operations f2fs_node_aops;
> extern const struct address_space_operations f2fs_meta_aops;
> extern const struct inode_operations f2fs_dir_inode_operations;
> extern const struct inode_operations f2fs_symlink_inode_operations;
>+extern const struct inode_operations f2fs_fast_symlink_inode_operations;
> extern const struct inode_operations f2fs_special_inode_operations;
> extern struct kmem_cache *inode_entry_slab;
> 
>diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>index f1341c7..05a3d4f 100644
>--- a/fs/f2fs/file.c
>+++ b/fs/f2fs/file.c
>@@ -546,6 +546,8 @@ void f2fs_truncate(struct inode *inode)
> 	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
> 				S_ISLNK(inode->i_mode)))
> 		return;
>+	if (f2fs_inode_is_fast_symlink(inode))
>+		return;
> 
> 	trace_f2fs_truncate(inode);
> 
>diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>index b508744..ec9ad22 100644
>--- a/fs/f2fs/inode.c
>+++ b/fs/f2fs/inode.c
>@@ -13,6 +13,7 @@
> #include <linux/buffer_head.h>
> #include <linux/writeback.h>
> #include <linux/bitops.h>
>+#include <linux/namei.h>
> 
> #include "f2fs.h"
> #include "node.h"
>@@ -188,8 +189,12 @@ make_now:
> 		inode->i_mapping->a_ops = &f2fs_dblock_aops;
> 		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
> 	} else if (S_ISLNK(inode->i_mode)) {
>-		inode->i_op = &f2fs_symlink_inode_operations;
>-		inode->i_mapping->a_ops = &f2fs_dblock_aops;
>+		if (f2fs_inode_is_fast_symlink(inode))
>+			inode->i_op = &f2fs_fast_symlink_inode_operations;
>+		else {
>+			inode->i_op = &f2fs_symlink_inode_operations;
>+			inode->i_mapping->a_ops = &f2fs_dblock_aops;
>+		}
> 	} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
> 			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
> 		inode->i_op = &f2fs_special_inode_operations;
>diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>index 1e2ae21..0619909 100644
>--- a/fs/f2fs/namei.c
>+++ b/fs/f2fs/namei.c
>@@ -14,6 +14,7 @@
> #include <linux/sched.h>
> #include <linux/ctype.h>
> #include <linux/dcache.h>
>+#include <linux/namei.h>
> 
> #include "f2fs.h"
> #include "node.h"
>@@ -247,6 +248,21 @@ fail:
> 	return err;
> }
> 
>+static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
>+{
>+	struct page *node_page;
>+
>+	node_page = get_node_page(F2FS_I_SB(dentry->d_inode),
>+				dentry->d_inode->i_ino);
>+	if (IS_ERR(node_page)) {
>+		nd_set_link(nd, NULL);
>+		return NULL;
>+	}
>+	nd_set_link(nd, (char *)(F2FS_INODE(node_page)->i_addr));
>+	f2fs_put_page(node_page, 1);
>+	return NULL;
>+}
>+
> static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> 					const char *symname)
> {
>@@ -261,16 +277,31 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
> 	if (IS_ERR(inode))
> 		return PTR_ERR(inode);
> 
>-	inode->i_op = &f2fs_symlink_inode_operations;
>-	inode->i_mapping->a_ops = &f2fs_dblock_aops;
>-
> 	f2fs_lock_op(sbi);
> 	err = f2fs_add_link(dentry, inode);
> 	if (err)
> 		goto out;
> 	f2fs_unlock_op(sbi);
> 
>-	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);
>+		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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-16 13:03 ` Chao Yu
@ 2015-03-16 23:09   ` Wanpeng Li
  0 siblings, 0 replies; 11+ messages in thread
From: Wanpeng Li @ 2015-03-16 23:09 UTC (permalink / raw)
  To: Chao Yu
  Cc: 'Wanpeng Li', 'Jaegeuk Kim',
	'Changman Lee',
	linux-f2fs-devel, linux-fsdevel, linux-kernel

Hi Chao,
On Mon, Mar 16, 2015 at 09:03:30PM +0800, Chao Yu wrote:
>Hi Wanpeng, Jaegeuk,
>
>> -----Original Message-----
>> From: Wanpeng Li [mailto:wanpeng.li@linux.intel.com]
>> Sent: Friday, March 13, 2015 2:34 PM
>> To: Jaegeuk Kim
>> Cc: Changman Lee; Chao Yu; linux-f2fs-devel@lists.sourceforge.net;
>> linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; Wanpeng Li
>> Subject: [PATCH v3] f2fs: add fast symlink support
>> 
>> This patch introduces the improvement fast symlinks to allow storage of
>> the target path within inode, thus symlinks with short target paths are
>> more accessed quickly. It will fall back to using the original slow
>> symlink if the target path exceeds the available inode space.
>> 
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>> ---
>> v2 -> v3:
>>  * handle fast symlink in remove_inode_page
>> v1 -> v2:
>>  * set max size of fast symlink according to inline_xattr flag
>> 
>>  fs/f2fs/f2fs.h          | 15 ++++++++++++++
>>  fs/f2fs/file.c          |  2 ++
>>  fs/f2fs/inode.c         |  9 +++++++--
>>  fs/f2fs/namei.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++----
>>  fs/f2fs/node.c          |  5 +++--
>>  include/linux/f2fs_fs.h |  2 ++
>>  6 files changed, 78 insertions(+), 8 deletions(-)
>> 
>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>> index 511d6cd..3012ea0 100644
>> --- a/fs/f2fs/f2fs.h
>> +++ b/fs/f2fs/f2fs.h
>> @@ -1226,6 +1226,7 @@ enum {
>>  	FI_VOLATILE_FILE,	/* indicate volatile file */
>>  	FI_DROP_CACHE,		/* drop dirty page cache */
>>  	FI_DATA_EXIST,		/* indicate data exists */
>> +	FI_FAST_SYMLINK,        /* indicate fast symlink */
>>  };
>> 
>>  static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
>> @@ -1262,6 +1263,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
>>  		set_inode_flag(fi, FI_INLINE_DENTRY);
>>  	if (ri->i_inline & F2FS_DATA_EXIST)
>>  		set_inode_flag(fi, FI_DATA_EXIST);
>> +	if (ri->i_inline & F2FS_FAST_SYMLINK)
>> +		set_inode_flag(fi, FI_FAST_SYMLINK);
>>  }
>> 
>>  static inline void set_raw_inline(struct f2fs_inode_info *fi,
>> @@ -1277,6 +1280,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,
>>  		ri->i_inline |= F2FS_INLINE_DENTRY;
>>  	if (is_inode_flag_set(fi, FI_DATA_EXIST))
>>  		ri->i_inline |= F2FS_DATA_EXIST;
>> +	if (is_inode_flag_set(fi, FI_FAST_SYMLINK))
>> +		ri->i_inline |= F2FS_FAST_SYMLINK;
>>  }
>> 
>>  static inline int f2fs_has_inline_xattr(struct inode *inode)
>> @@ -1370,6 +1375,15 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
>>  	sbi->sb->s_flags |= MS_RDONLY;
>>  }
>> 
>> +/*
>> + * Test whether an inode is a fast symlink.
>> + */
>> +static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
>> +{
>> +	return (S_ISLNK(inode->i_mode) &&
>> +			is_inode_flag_set(F2FS_I(inode), FI_FAST_SYMLINK));
>
>Look at this again, it seems that it's not necessary to introduce FI_FAST_SYMLINK
>& F2FS_FAST_SYNLINK as we waste one bit in i_inline flag, for fast symlink, using
>your original design will be enough until we consider xattr node block case.
>
>static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
>{
>	return (S_ISLNK(inode->i_mode) && !F2FS_HAS_BLOCKS(inode));
>}
>
>Wanpeng, how about waiting for Jaegeuk's opinion?
>
>> +}
>> +
>>  #define get_inode_mode(i) \
>>  	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
>>  	 (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
>> @@ -1746,6 +1760,7 @@ extern const struct address_space_operations f2fs_node_aops;
>>  extern const struct address_space_operations f2fs_meta_aops;
>>  extern const struct inode_operations f2fs_dir_inode_operations;
>>  extern const struct inode_operations f2fs_symlink_inode_operations;
>> +extern const struct inode_operations f2fs_fast_symlink_inode_operations;
>>  extern const struct inode_operations f2fs_special_inode_operations;
>>  extern struct kmem_cache *inode_entry_slab;
>> 
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index f1341c7..05a3d4f 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -546,6 +546,8 @@ void f2fs_truncate(struct inode *inode)
>>  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
>>  				S_ISLNK(inode->i_mode)))
>>  		return;
>> +	if (f2fs_inode_is_fast_symlink(inode))
>> +		return;
>> 
>>  	trace_f2fs_truncate(inode);
>> 
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index b508744..ec9ad22 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -13,6 +13,7 @@
>>  #include <linux/buffer_head.h>
>>  #include <linux/writeback.h>
>>  #include <linux/bitops.h>
>> +#include <linux/namei.h>
>> 
>>  #include "f2fs.h"
>>  #include "node.h"
>> @@ -188,8 +189,12 @@ make_now:
>>  		inode->i_mapping->a_ops = &f2fs_dblock_aops;
>>  		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>>  	} else if (S_ISLNK(inode->i_mode)) {
>> -		inode->i_op = &f2fs_symlink_inode_operations;
>> -		inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> +		if (f2fs_inode_is_fast_symlink(inode))
>> +			inode->i_op = &f2fs_fast_symlink_inode_operations;
>> +		else {
>> +			inode->i_op = &f2fs_symlink_inode_operations;
>> +			inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> +		}
>>  	} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
>>  			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
>>  		inode->i_op = &f2fs_special_inode_operations;
>> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
>> index 1e2ae21..0619909 100644
>> --- a/fs/f2fs/namei.c
>> +++ b/fs/f2fs/namei.c
>> @@ -14,6 +14,7 @@
>>  #include <linux/sched.h>
>>  #include <linux/ctype.h>
>>  #include <linux/dcache.h>
>> +#include <linux/namei.h>
>> 
>>  #include "f2fs.h"
>>  #include "node.h"
>> @@ -247,6 +248,21 @@ fail:
>>  	return err;
>>  }
>> 
>> +static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
>> +{
>> +	struct page *node_page;
>> +
>> +	node_page = get_node_page(F2FS_I_SB(dentry->d_inode),
>> +				dentry->d_inode->i_ino);
>> +	if (IS_ERR(node_page)) {
>> +		nd_set_link(nd, NULL);
>> +		return NULL;
>> +	}
>> +	nd_set_link(nd, (char *)(F2FS_INODE(node_page)->i_addr));
>> +	f2fs_put_page(node_page, 1);
>> +	return NULL;
>> +}
>
>The only different of i_op for fast symlink and normal symlink is that
>we use different callback function for ->follow_link, if we merge
>page_follow_link_light & f2fs_follow_link into one, we can remove duplicated
>f2fs_fast_symlink_inode_operations. How do you think of it?

I found that ext2/3/4 do the same as mine. ;-)

Regards,
Wanpeng Li 

>
>> +
>>  static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>>  					const char *symname)
>>  {
>> @@ -261,16 +277,31 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>>  	if (IS_ERR(inode))
>>  		return PTR_ERR(inode);
>> 
>> -	inode->i_op = &f2fs_symlink_inode_operations;
>> -	inode->i_mapping->a_ops = &f2fs_dblock_aops;
>> -
>>  	f2fs_lock_op(sbi);
>>  	err = f2fs_add_link(dentry, inode);
>>  	if (err)
>>  		goto out;
>>  	f2fs_unlock_op(sbi);
>> 
>> -	err = page_symlink(inode, symname, symlen);
>> +
>> +	if (symlen > MAX_FAST_SYMLINK_SIZE) {
>
>I think we should be pick up max_fast_symlink_size which is introduced in v2.
>
>> +		/* 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);
>> +		set_page_dirty(node_page);
>> +		f2fs_put_page(node_page, 1);
>> +		inode->i_size = symlen-1;
>
>trivial coding style issue.
>inode->i_size = symlen - 1;
>
>Thanks,
>
>> +		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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  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:08 ` Wanpeng Li
@ 2015-03-17 17:21 ` Jaegeuk Kim
  2015-03-18  8:58   ` Wanpeng Li
  2 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2015-03-17 17:21 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Changman Lee, Chao Yu, linux-f2fs-devel, linux-fsdevel, linux-kernel

Hi guys,

Sorry for the delay due to travel.

On Fri, Mar 13, 2015 at 02:33:39PM +0800, Wanpeng Li wrote:
> This patch introduces the improvement fast symlinks to allow storage of
> the target path within inode, thus symlinks with short target paths are
> more accessed quickly. It will fall back to using the original slow
> symlink if the target path exceeds the available inode space.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
> v2 -> v3:
>  * handle fast symlink in remove_inode_page
> v1 -> v2:
>  * set max size of fast symlink according to inline_xattr flag
> 
>  fs/f2fs/f2fs.h          | 15 ++++++++++++++
>  fs/f2fs/file.c          |  2 ++
>  fs/f2fs/inode.c         |  9 +++++++--
>  fs/f2fs/namei.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++----
>  fs/f2fs/node.c          |  5 +++--
>  include/linux/f2fs_fs.h |  2 ++
>  6 files changed, 78 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 511d6cd..3012ea0 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1226,6 +1226,7 @@ enum {
>  	FI_VOLATILE_FILE,	/* indicate volatile file */
>  	FI_DROP_CACHE,		/* drop dirty page cache */
>  	FI_DATA_EXIST,		/* indicate data exists */
> +	FI_FAST_SYMLINK,        /* indicate fast symlink */
>  };
>  
>  static inline void set_inode_flag(struct f2fs_inode_info *fi, int flag)
> @@ -1262,6 +1263,8 @@ static inline void get_inline_info(struct f2fs_inode_info *fi,
>  		set_inode_flag(fi, FI_INLINE_DENTRY);
>  	if (ri->i_inline & F2FS_DATA_EXIST)
>  		set_inode_flag(fi, FI_DATA_EXIST);
> +	if (ri->i_inline & F2FS_FAST_SYMLINK)
> +		set_inode_flag(fi, FI_FAST_SYMLINK);
>  }
>  
>  static inline void set_raw_inline(struct f2fs_inode_info *fi,
> @@ -1277,6 +1280,8 @@ static inline void set_raw_inline(struct f2fs_inode_info *fi,
>  		ri->i_inline |= F2FS_INLINE_DENTRY;
>  	if (is_inode_flag_set(fi, FI_DATA_EXIST))
>  		ri->i_inline |= F2FS_DATA_EXIST;
> +	if (is_inode_flag_set(fi, FI_FAST_SYMLINK))
> +		ri->i_inline |= F2FS_FAST_SYMLINK;
>  }
>  
>  static inline int f2fs_has_inline_xattr(struct inode *inode)
> @@ -1370,6 +1375,15 @@ static inline void f2fs_stop_checkpoint(struct f2fs_sb_info *sbi)
>  	sbi->sb->s_flags |= MS_RDONLY;
>  }
>  
> +/*
> + * Test whether an inode is a fast symlink.
> + */
> +static inline int f2fs_inode_is_fast_symlink(struct inode *inode)
> +{
> +	return (S_ISLNK(inode->i_mode) &&
> +			is_inode_flag_set(F2FS_I(inode), FI_FAST_SYMLINK));
> +}
> +
>  #define get_inode_mode(i) \
>  	((is_inode_flag_set(F2FS_I(i), FI_ACL_MODE)) ? \
>  	 (F2FS_I(i)->i_acl_mode) : ((i)->i_mode))
> @@ -1746,6 +1760,7 @@ extern const struct address_space_operations f2fs_node_aops;
>  extern const struct address_space_operations f2fs_meta_aops;
>  extern const struct inode_operations f2fs_dir_inode_operations;
>  extern const struct inode_operations f2fs_symlink_inode_operations;
> +extern const struct inode_operations f2fs_fast_symlink_inode_operations;
>  extern const struct inode_operations f2fs_special_inode_operations;
>  extern struct kmem_cache *inode_entry_slab;
>  
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index f1341c7..05a3d4f 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -546,6 +546,8 @@ void f2fs_truncate(struct inode *inode)
>  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
>  				S_ISLNK(inode->i_mode)))
>  		return;
> +	if (f2fs_inode_is_fast_symlink(inode))
> +		return;
>  
>  	trace_f2fs_truncate(inode);
>  
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index b508744..ec9ad22 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -13,6 +13,7 @@
>  #include <linux/buffer_head.h>
>  #include <linux/writeback.h>
>  #include <linux/bitops.h>
> +#include <linux/namei.h>
>  
>  #include "f2fs.h"
>  #include "node.h"
> @@ -188,8 +189,12 @@ make_now:
>  		inode->i_mapping->a_ops = &f2fs_dblock_aops;
>  		mapping_set_gfp_mask(inode->i_mapping, GFP_F2FS_HIGH_ZERO);
>  	} else if (S_ISLNK(inode->i_mode)) {
> -		inode->i_op = &f2fs_symlink_inode_operations;
> -		inode->i_mapping->a_ops = &f2fs_dblock_aops;
> +		if (f2fs_inode_is_fast_symlink(inode))
> +			inode->i_op = &f2fs_fast_symlink_inode_operations;
> +		else {
> +			inode->i_op = &f2fs_symlink_inode_operations;
> +			inode->i_mapping->a_ops = &f2fs_dblock_aops;
> +		}
>  	} else if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode) ||
>  			S_ISFIFO(inode->i_mode) || S_ISSOCK(inode->i_mode)) {
>  		inode->i_op = &f2fs_special_inode_operations;
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 1e2ae21..0619909 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -14,6 +14,7 @@
>  #include <linux/sched.h>
>  #include <linux/ctype.h>
>  #include <linux/dcache.h>
> +#include <linux/namei.h>
>  
>  #include "f2fs.h"
>  #include "node.h"
> @@ -247,6 +248,21 @@ fail:
>  	return err;
>  }
>  
> +static void *f2fs_follow_link(struct dentry *dentry, struct nameidata *nd)
> +{
> +	struct page *node_page;
> +
> +	node_page = get_node_page(F2FS_I_SB(dentry->d_inode),
> +				dentry->d_inode->i_ino);
> +	if (IS_ERR(node_page)) {
> +		nd_set_link(nd, NULL);
> +		return NULL;
> +	}
> +	nd_set_link(nd, (char *)(F2FS_INODE(node_page)->i_addr));
> +	f2fs_put_page(node_page, 1);
> +	return NULL;
> +}
> +
>  static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>  					const char *symname)
>  {
> @@ -261,16 +277,31 @@ static int f2fs_symlink(struct inode *dir, struct dentry *dentry,
>  	if (IS_ERR(inode))
>  		return PTR_ERR(inode);
>  
> -	inode->i_op = &f2fs_symlink_inode_operations;
> -	inode->i_mapping->a_ops = &f2fs_dblock_aops;
> -
>  	f2fs_lock_op(sbi);
>  	err = f2fs_add_link(dentry, inode);
>  	if (err)
>  		goto out;
>  	f2fs_unlock_op(sbi);
>  
> -	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?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-17 17:21 ` Jaegeuk Kim
@ 2015-03-18  8:58   ` Wanpeng Li
  2015-03-18 18:05     ` Jaegeuk Kim
  0 siblings, 1 reply; 11+ messages in thread
From: Wanpeng Li @ 2015-03-18  8:58 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Changman Lee, Chao Yu, linux-f2fs-devel, linux-fsdevel, linux-kernel

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()?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-18  8:58   ` Wanpeng Li
@ 2015-03-18 18:05     ` Jaegeuk Kim
  2015-03-18 23:02       ` Wanpeng Li
  0 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2015-03-18 18:05 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Changman Lee, Chao Yu, linux-f2fs-devel, linux-fsdevel, linux-kernel

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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-18 18:05     ` Jaegeuk Kim
@ 2015-03-18 23:02       ` Wanpeng Li
  2015-03-19  3:17         ` Chao Yu
  0 siblings, 1 reply; 11+ messages in thread
From: Wanpeng Li @ 2015-03-18 23:02 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Wanpeng Li, Changman Lee, Chao Yu, linux-f2fs-devel,
	linux-fsdevel, linux-kernel

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?

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v3] f2fs: add fast symlink support
  2015-03-18 23:02       ` Wanpeng Li
@ 2015-03-19  3:17         ` Chao Yu
  2015-03-20 20:13           ` Jaegeuk Kim
  0 siblings, 1 reply; 11+ messages in thread
From: Chao Yu @ 2015-03-19  3:17 UTC (permalink / raw)
  To: 'Wanpeng Li', 'Jaegeuk Kim'
  Cc: 'Changman Lee', linux-f2fs-devel, linux-fsdevel, linux-kernel

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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-19  3:17         ` Chao Yu
@ 2015-03-20 20:13           ` Jaegeuk Kim
  2015-03-22 23:10             ` Wanpeng Li
  0 siblings, 1 reply; 11+ messages in thread
From: Jaegeuk Kim @ 2015-03-20 20:13 UTC (permalink / raw)
  To: Chao Yu
  Cc: 'Wanpeng Li', 'Changman Lee',
	linux-f2fs-devel, linux-fsdevel, linux-kernel

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v3] f2fs: add fast symlink support
  2015-03-20 20:13           ` Jaegeuk Kim
@ 2015-03-22 23:10             ` Wanpeng Li
  0 siblings, 0 replies; 11+ messages in thread
From: Wanpeng Li @ 2015-03-22 23:10 UTC (permalink / raw)
  To: Jaegeuk Kim
  Cc: Chao Yu, 'Wanpeng Li', 'Changman Lee',
	linux-f2fs-devel, linux-fsdevel, linux-kernel

Hi Jaegeuk,
On Fri, Mar 20, 2015 at 01:13:39PM -0700, Jaegeuk Kim wrote:
>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"?

Will do, thanks for you and Chao's help.

Regards,
Wanpeng Li 

>
>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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2015-03-22 23:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 18:05     ` Jaegeuk Kim
2015-03-18 23:02       ` Wanpeng Li
2015-03-19  3:17         ` Chao Yu
2015-03-20 20:13           ` Jaegeuk Kim
2015-03-22 23:10             ` Wanpeng Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).