Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200225014257.83903-1-yuchao0@huawei.com>
In-Reply-To: <20200225014257.83903-1-yuchao0@huawei.com>
From:   Ju Hyung Park <qkrwngud825@gmail.com>
Date:   Tue, 25 Feb 2020 15:37:15 +0900
Message-ID: <CAD14+f0e5RsntWQk+0=_6f0rJt=0GNBQBZZ3BqjbQzZqJ7f4iw@mail.gmail.com>
Subject: Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups
To:     Chao Yu <yuchao0@huawei.com>
Cc:     Jaegeuk Kim <jaegeuk@kernel.org>,
        linux-f2fs-devel@lists.sourceforge.net,
        linux-kernel@vger.kernel.org, Chao Yu <chao@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@huawei.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>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>
> 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
>