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

[Ju Hyung Park <qkrwngud825@gmail.com>]

Hi everyone.

This is a RFC patch.

This patch introduces an even bigger problem, which is forcing all
xattr lookup memory allocations to be made in 4076B, when in reality,
4076B allocations are only made during initial mounts and the rests
are made in 204B, unnecessarily wasting memory.

In my testing, 4076B allocations are only done 4 times during mount
and the rests(millions) are in 204B.

I'd like to ask the maintainers to suggest some bright ideas on how to
tackle this correctly.
(e.g. Use kmem pool only for 204B allocations and fallback to regular
kzalloc() if (*base_size != 204)?)

Thanks.

On Fri, Jul 12, 2019 at 12:06 AM Park Ju Hyung <qkrwngud825@gmail.com> 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@gmail.com>
> ---
>  fs/f2fs/f2fs.h  |  6 ++++++
>  fs/f2fs/super.c |  8 +++++++-
>  fs/f2fs/xattr.c | 33 ++++++++++++++++++++++++---------
>  3 files changed, 37 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 9c6388253c9d2..3046ca2ebd121 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3510,6 +3510,12 @@ void f2fs_exit_sysfs(void);
>  int f2fs_register_sysfs(struct f2fs_sb_info *sbi);
>  void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi);
>
> +/*
> + * xattr.c
> + */
> +int __init f2fs_init_xattr_caches(void);
> +void f2fs_destroy_xattr_caches(void);
> +
>  /*
>   * crypto support
>   */
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 6d262d13251cf..abb59d9e25848 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -3614,9 +3614,12 @@ static int __init init_f2fs_fs(void)
>         err = init_inodecache();
>         if (err)
>                 goto fail;
> -       err = f2fs_create_node_manager_caches();
> +       err = f2fs_init_xattr_caches();
>         if (err)
>                 goto free_inodecache;
> +       err = f2fs_create_node_manager_caches();
> +       if (err)
> +               goto fail_xattr_caches;
>         err = f2fs_create_segment_manager_caches();
>         if (err)
>                 goto free_node_manager_caches;
> @@ -3656,6 +3659,8 @@ static int __init init_f2fs_fs(void)
>         f2fs_destroy_segment_manager_caches();
>  free_node_manager_caches:
>         f2fs_destroy_node_manager_caches();
> +fail_xattr_caches:
> +       f2fs_destroy_xattr_caches();
>  free_inodecache:
>         destroy_inodecache();
>  fail:
> @@ -3673,6 +3678,7 @@ static void __exit exit_f2fs_fs(void)
>         f2fs_destroy_checkpoint_caches();
>         f2fs_destroy_segment_manager_caches();
>         f2fs_destroy_node_manager_caches();
> +       f2fs_destroy_xattr_caches();
>         destroy_inodecache();
>         f2fs_destroy_trace_ios();
>  }
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index e791741d193b8..635b50ea3e5e8 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -22,6 +22,23 @@
>  #include "f2fs.h"
>  #include "xattr.h"
>
> +static struct kmem_cache *f2fs_xattr_cachep;
> +
> +int __init f2fs_init_xattr_caches(void)
> +{
> +       f2fs_xattr_cachep = f2fs_kmem_cache_create("xattr_entry",
> +                       VALID_XATTR_BLOCK_SIZE + XATTR_PADDING_SIZE);
> +       if (!f2fs_xattr_cachep)
> +               return -ENOMEM;
> +
> +       return 0;
> +}
> +
> +void f2fs_destroy_xattr_caches(void)
> +{
> +       kmem_cache_destroy(f2fs_xattr_cachep);
> +}
> +
>  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)
> @@ -312,7 +329,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
>                 return -ENODATA;
>
>         *base_size = XATTR_SIZE(xnid, inode) + XATTR_PADDING_SIZE;
> -       txattr_addr = f2fs_kzalloc(F2FS_I_SB(inode), *base_size, GFP_NOFS);
> +       txattr_addr = kmem_cache_zalloc(f2fs_xattr_cachep, GFP_NOFS);
>         if (!txattr_addr)
>                 return -ENOMEM;
>
> @@ -358,7 +375,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
>         *base_addr = txattr_addr;
>         return 0;
>  out:
> -       kvfree(txattr_addr);
> +       kmem_cache_free(f2fs_xattr_cachep, txattr_addr);
>         return err;
>  }
>
> @@ -367,13 +384,11 @@ static int read_all_xattrs(struct inode *inode, struct page *ipage,
>  {
>         struct f2fs_xattr_header *header;
>         nid_t xnid = F2FS_I(inode)->i_xattr_nid;
> -       unsigned int size = VALID_XATTR_BLOCK_SIZE;
>         unsigned int inline_size = inline_xattr_size(inode);
>         void *txattr_addr;
>         int err;
>
> -       txattr_addr = f2fs_kzalloc(F2FS_I_SB(inode),
> -                       inline_size + size + XATTR_PADDING_SIZE, GFP_NOFS);
> +       txattr_addr = kmem_cache_zalloc(f2fs_xattr_cachep, GFP_NOFS);
>         if (!txattr_addr)
>                 return -ENOMEM;
>
> @@ -401,7 +416,7 @@ static int read_all_xattrs(struct inode *inode, struct page *ipage,
>         *base_addr = txattr_addr;
>         return 0;
>  fail:
> -       kvfree(txattr_addr);
> +       kmem_cache_free(f2fs_xattr_cachep, txattr_addr);
>         return err;
>  }
>
> @@ -528,7 +543,7 @@ int f2fs_getxattr(struct inode *inode, int index, const char *name,
>         }
>         error = size;
>  out:
> -       kvfree(base_addr);
> +       kmem_cache_free(f2fs_xattr_cachep, base_addr);
>         return error;
>  }
>
> @@ -574,7 +589,7 @@ ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer, size_t buffer_size)
>         }
>         error = buffer_size - rest;
>  cleanup:
> -       kvfree(base_addr);
> +       kmem_cache_free(f2fs_xattr_cachep, base_addr);
>         return error;
>  }
>
> @@ -712,7 +727,7 @@ static int __f2fs_setxattr(struct inode *inode, int index,
>         if (!error && S_ISDIR(inode->i_mode))
>                 set_sbi_flag(F2FS_I_SB(inode), SBI_NEED_CP);
>  exit:
> -       kvfree(base_addr);
> +       kmem_cache_free(f2fs_xattr_cachep, base_addr);
>         return error;
>  }
>
> --
> 2.21.0
>


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel