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

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 07/12, Ju Hyung Park wrote:
> 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)?)

How about adding two paths? One is kzalloc() for normal case, and the other
is slab alloc for inline_xattr case?

> 
> 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


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