| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAD14+f0e5RsntWQk+0=_6f0rJt=0GNBQBZZ3BqjbQzZqJ7f4iw@mail.gmail.com>
Date: Tue, 25 Feb 2020 15:37:15 +0900
From: Ju Hyung Park <qkrwngud825@...il.com>
To: Chao Yu <yuchao0@...wei.com>
Cc: Jaegeuk Kim <jaegeuk@...nel.org>,
linux-f2fs-devel@...ts.sourceforge.net,
linux-kernel@...r.kernel.org, Chao Yu <chao@...nel.org>
Subject: Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups
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@...wei.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@...il.com>
> Signed-off-by: Chao Yu <yuchao0@...wei.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
>
Powered by blists - more mailing lists