Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups

From: Ju Hyung Park
Date: Tue Feb 25 2020 - 01:37:34 EST

Next message: Xiaoyao Li: "Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT"
Previous message: Vasily Averin: "Re: [PATCH 2/7] gcov_seq_next should increase position index"
In reply to: Chao Yu: "[PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Next in thread: Chao Yu: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Chao.

Sorry for the late reply, I've been busy with my uni stuffs.

It's nice to see this issue getting some attentions back, but
unfortunately, I can't test it at the moment.

Here's the patch we used to see the alloc statistics:
https://pastebin.com/bhBh2dgs by Sultan Alsawaf.

Hope it helps.

On Tue, Feb 25, 2020 at 10:43 AM Chao Yu <yuchao0@xxxxxxxxxx> wrote:
>
> It's been observed that kzalloc() on lookup_all_xattrs() are called millions
> of times on Android, quickly becoming the top abuser of slub memory allocator.
>
> Use a dedicated kmem cache pool for xattr lookups to mitigate this.
>
> Signed-off-by: Park Ju Hyung <qkrwngud825@xxxxxxxxx>
> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>
> ---
>
> Hi, Ju Hyung,
>
> Let me know if you have any objection on this patch. :)
>
> fs/f2fs/f2fs.h | 3 +++
> fs/f2fs/super.c | 10 ++++++++-
> fs/f2fs/xattr.c | 54 ++++++++++++++++++++++++++++++++++++++++++++-----
> fs/f2fs/xattr.h | 4 ++++
> 4 files changed, 65 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 12a197e89a3e..23b93a116c73 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1486,6 +1486,9 @@ struct f2fs_sb_info {
> __u32 s_chksum_seed;
>
> struct workqueue_struct *post_read_wq; /* post read workqueue */
> +
> + struct kmem_cache *inline_xattr_slab; /* inline xattr entry */
> + unsigned int inline_xattr_slab_size; /* default inline xattr slab size */
> };
>
> struct f2fs_private_dio {
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index d241e07c0bfa..0b16204d3b7d 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -1201,6 +1201,7 @@ static void f2fs_put_super(struct super_block *sb)
> kvfree(sbi->raw_super);
>
> destroy_device_list(sbi);
> + f2fs_destroy_xattr_caches(sbi);
> mempool_destroy(sbi->write_io_dummy);
> #ifdef CONFIG_QUOTA
> for (i = 0; i < MAXQUOTAS; i++)
> @@ -3457,12 +3458,17 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> }
> }
>
> + /* init per sbi slab cache */
> + err = f2fs_init_xattr_caches(sbi);
> + if (err)
> + goto free_io_dummy;
> +
> /* get an inode for meta space */
> sbi->meta_inode = f2fs_iget(sb, F2FS_META_INO(sbi));
> if (IS_ERR(sbi->meta_inode)) {
> f2fs_err(sbi, "Failed to read F2FS meta data inode");
> err = PTR_ERR(sbi->meta_inode);
> - goto free_io_dummy;
> + goto free_xattr_cache;
> }
>
> err = f2fs_get_valid_checkpoint(sbi);
> @@ -3735,6 +3741,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> make_bad_inode(sbi->meta_inode);
> iput(sbi->meta_inode);
> sbi->meta_inode = NULL;
> +free_xattr_cache:
> + f2fs_destroy_xattr_caches(sbi);
> free_io_dummy:
> mempool_destroy(sbi->write_io_dummy);
> free_percpu:
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index a3360a97e624..e46a10eb0e42 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -23,6 +23,25 @@
> #include "xattr.h"
> #include "segment.h"
>
> +static void *xattr_alloc(struct f2fs_sb_info *sbi, int size, bool *is_inline)
> +{
> + *is_inline = (size == sbi->inline_xattr_slab_size);
> +
> + if (*is_inline)
> + return kmem_cache_zalloc(sbi->inline_xattr_slab, GFP_NOFS);
> +
> + return f2fs_kzalloc(sbi, size, GFP_NOFS);
> +}
> +
> +static void xattr_free(struct f2fs_sb_info *sbi, void *xattr_addr,
> + bool is_inline)
> +{
> + if (is_inline)
> + kmem_cache_free(sbi->inline_xattr_slab, xattr_addr);
> + else
> + kvfree(xattr_addr);
> +}
> +
> static int f2fs_xattr_generic_get(const struct xattr_handler *handler,
> struct dentry *unused, struct inode *inode,
> const char *name, void *buffer, size_t size)
> @@ -301,7 +320,8 @@ static int read_xattr_block(struct inode *inode, void *txattr_addr)
> static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
> unsigned int index, unsigned int len,
> const char *name, struct f2fs_xattr_entry **xe,
> - void **base_addr, int *base_size)
> + void **base_addr, int *base_size,
> + bool *is_inline)
> {
> void *cur_addr, *txattr_addr, *last_txattr_addr;
> void *last_addr = NULL;
> @@ -313,7 +333,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
> return -ENODATA;
>
> *base_size = XATTR_SIZE(inode) + XATTR_PADDING_SIZE;
> - txattr_addr = f2fs_kzalloc(F2FS_I_SB(inode), *base_size, GFP_NOFS);
> + txattr_addr = xattr_alloc(F2FS_I_SB(inode), *base_size, is_inline);
> if (!txattr_addr)
> return -ENOMEM;
>
> @@ -362,7 +382,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
> *base_addr = txattr_addr;
> return 0;
> out:
> - kvfree(txattr_addr);
> + xattr_free(F2FS_I_SB(inode), txattr_addr, *is_inline);
> return err;
> }
>
> @@ -499,6 +519,7 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> unsigned int size, len;
> void *base_addr = NULL;
> int base_size;
> + bool is_inline;
>
> if (name == NULL)
> return -EINVAL;
> @@ -509,7 +530,7 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>
> down_read(&F2FS_I(inode)->i_xattr_sem);
> error = lookup_all_xattrs(inode, ipage, index, len, name,
> - &entry, &base_addr, &base_size);
> + &entry, &base_addr, &base_size, &is_inline);
> up_read(&F2FS_I(inode)->i_xattr_sem);
> if (error)
> return error;
> @@ -532,7 +553,7 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
> }
> error = size;
> out:
> - kvfree(base_addr);
> + xattr_free(F2FS_I_SB(inode), base_addr, is_inline);
> return error;
> }
>
> @@ -767,3 +788,26 @@ int f2fs_setxattr(struct inode *inode, int index, const char *name,
> f2fs_update_time(sbi, REQ_TIME);
> return err;
> }
> +
> +int f2fs_init_xattr_caches(struct f2fs_sb_info *sbi)
> +{
> + dev_t dev = sbi->sb->s_bdev->bd_dev;
> + char slab_name[32];
> +
> + sprintf(slab_name, "f2fs_xattr_entry-%u:%u", MAJOR(dev), MINOR(dev));
> +
> + sbi->inline_xattr_slab_size = F2FS_OPTION(sbi).inline_xattr_size *
> + sizeof(__le32) + XATTR_PADDING_SIZE;
> +
> + sbi->inline_xattr_slab = f2fs_kmem_cache_create(slab_name,
> + sbi->inline_xattr_slab_size);
> + if (!sbi->inline_xattr_slab)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi)
> +{
> + kmem_cache_destroy(sbi->inline_xattr_slab);
> +}
> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
> index 574beea46494..7345dfa7b7a9 100644
> --- a/fs/f2fs/xattr.h
> +++ b/fs/f2fs/xattr.h
> @@ -131,6 +131,8 @@ extern int f2fs_setxattr(struct inode *, int, const char *,
> extern int f2fs_getxattr(struct inode *, int, const char *, void *,
> size_t, struct page *);
> extern ssize_t f2fs_listxattr(struct dentry *, char *, size_t);
> +extern int f2fs_init_xattr_caches(struct f2fs_sb_info *);
> +extern void f2fs_destroy_xattr_caches(struct f2fs_sb_info *);
> #else
>
> #define f2fs_xattr_handlers NULL
> @@ -151,6 +153,8 @@ static inline ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer,
> {
> return -EOPNOTSUPP;
> }
> +static int f2fs_init_xattr_caches(struct f2fs_sb_info *sbi) { return 0; }
> +static void f2fs_destroy_xattr_caches(struct f2fs_sb_info *sbi) { return 0; }
> #endif
>
> #ifdef CONFIG_F2FS_FS_SECURITY
> --
> 2.18.0.rc1
>

Next message: Xiaoyao Li: "Re: [PATCH 1/2] kvm: vmx: Use basic exit reason to check if it's the specific VM EXIT"
Previous message: Vasily Averin: "Re: [PATCH 2/7] gcov_seq_next should increase position index"
In reply to: Chao Yu: "[PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Next in thread: Chao Yu: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]