Re: [PATCH v3] f2fs: add fast symlink support
From: Wanpeng Li
Date: Mon Mar 16 2015 - 19:28:11 EST
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@xxxxxxxxxxxxxxx]
>> Sent: Friday, March 13, 2015 2:34 PM
>> To: Jaegeuk Kim
>> Cc: Changman Lee; Chao Yu; linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx;
>> linux-fsdevel@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 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@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));
>
>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
--
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/