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

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

[Park Ju Hyung]

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

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

[Ju Hyung Park]

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

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

[Jaegeuk Kim]

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

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

[Chao Yu]

On 2019/7/12 1:06, Jaegeuk Kim wrote:
> 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?

Agreed,

Something like this:

if (!xnid)
	addr = f2fs_kmem_cache_alloc();
else
	addr = f2fs_kzalloc();

....

if (!xnid)
	kmem_cache_free(, addr);
else
	kvfree(addr);

Thanks,

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


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

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

[Chao Yu]

Hi Jaegeuk, Ju Hyung, Yaroslav,

I can see "f2fs: xattr: reserve cache for xattr allocations" has been merged in
dev-test branch, however, it doesn't exist in f2fs mailing list, so I can not
comment on it.... Can anyone send it to the list?

Thanks,


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

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

[Ju Hyung Park]

Hi Chao and Jaegeuk,

I have no idea how that patch got merged.

We(me and Yaro) were supposed to work on doing some finishing touches
to the patch before sending it to upstream.

I'll personally check with Yaro.

Jaegeuk, please remove the patch.
That patch has numerous issues, biggest one being hardcoded size(SZ_256).

Also, I need to figure out how to allocate kmem cache per mounts.

Thanks.

On Mon, Jul 29, 2019 at 4:28 PM Chao Yu <yuchao0@huawei.com> wrote:
>
> Hi Jaegeuk, Ju Hyung, Yaroslav,
>
> I can see "f2fs: xattr: reserve cache for xattr allocations" has been merged in
> dev-test branch, however, it doesn't exist in f2fs mailing list, so I can not
> comment on it.... Can anyone send it to the list?
>
> Thanks,


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

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

[Chao Yu]

Hi Ju Hyung,

On 2019/7/29 15:48, Ju Hyung Park wrote:
> Hi Chao and Jaegeuk,
> 
> I have no idea how that patch got merged.
> 
> We(me and Yaro) were supposed to work on doing some finishing touches
> to the patch before sending it to upstream.
> 
> I'll personally check with Yaro.
> 
> Jaegeuk, please remove the patch.
> That patch has numerous issues, biggest one being hardcoded size(SZ_256).
> 
> Also, I need to figure out how to allocate kmem cache per mounts.

That would need a per sbi kmem cache, I guess it's reasonable and worth when we
mount multiple f2fs partitions with different inline xattr size.

However, IIRC, inline_xattr_size can be changed after remount, so how can we
handle different inline xattr size in one mount... that's also a problem.

BTW, in old patch I found kvfree() should not be replaced as f2fs_kzalloc() can
use vmalloc to allocate memroy.

-	kvfree(txattr_addr);
+	if (!inline_size)
+		kfree(txattr_addr);

Thanks,

> 
> Thanks.
> 
> On Mon, Jul 29, 2019 at 4:28 PM Chao Yu <yuchao0@huawei.com> wrote:
>>
>> Hi Jaegeuk, Ju Hyung, Yaroslav,
>>
>> I can see "f2fs: xattr: reserve cache for xattr allocations" has been merged in
>> dev-test branch, however, it doesn't exist in f2fs mailing list, so I can not
>> comment on it.... Can anyone send it to the list?
>>
>> Thanks,
> .
> 


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

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

[Jaegeuk Kim]

On 07/29, Ju Hyung Park wrote:
> Hi Chao and Jaegeuk,
> 
> I have no idea how that patch got merged.
> 
> We(me and Yaro) were supposed to work on doing some finishing touches
> to the patch before sending it to upstream.
> 
> I'll personally check with Yaro.
> 
> Jaegeuk, please remove the patch.
> That patch has numerous issues, biggest one being hardcoded size(SZ_256).

That branch is for testing purpose and kinda TODO list for me. :)
Anyway, I'll remove it, so please post it later with better shape.

Thanks,

> 
> Also, I need to figure out how to allocate kmem cache per mounts.
> 
> Thanks.
> 
> On Mon, Jul 29, 2019 at 4:28 PM Chao Yu <yuchao0@huawei.com> wrote:
> >
> > Hi Jaegeuk, Ju Hyung, Yaroslav,
> >
> > I can see "f2fs: xattr: reserve cache for xattr allocations" has been merged in
> > dev-test branch, however, it doesn't exist in f2fs mailing list, so I can not
> > comment on it.... Can anyone send it to the list?
> >
> > Thanks,


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