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

From: Chao Yu
Date: Tue Feb 25 2020 - 03:44:59 EST

Next message: Johan Hovold: "Re: [PATCH] USB: Replace zero-length array with flexible-array member"
Previous message: Wanpeng Li: "Re: [RFC] Question about async TLB flush and KVM pv tlb improvements"
In reply to: Ju Hyung Park: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Next in thread: kbuild test robot: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Ju Hyung,

Thanks for the reply.

On 2020/2/25 14:37, Ju Hyung Park wrote:
> 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.

Thanks for providing statistic patch, let me check this latter.

Thanks,

>
> 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: Johan Hovold: "Re: [PATCH] USB: Replace zero-length array with flexible-array member"
Previous message: Wanpeng Li: "Re: [RFC] Question about async TLB flush and KVM pv tlb improvements"
In reply to: Ju Hyung Park: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Next in thread: kbuild test robot: "Re: [PATCH] f2fs: use kmem_cache pool during inline xattr lookups"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]