Re: [PATCH v3] f2fs: add fast symlink support

[Jaegeuk Kim]

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@xxxxxxxxxxxxxxx>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/