[PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[JunChao Sun]

Replace kmem_cache_create with KMEM_CACHE macro that
guaranteed struct alignment

Signed-off-by: JunChao Sun <sunjunchao2870@gmail.com>
---
 fs/ext4/extents_status.c | 8 ++------
 fs/ext4/readpage.c       | 5 ++---
 2 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index cd0a861853e3..97eccc0028a1 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -155,9 +155,7 @@ static void __revise_pending(struct inode *inode, ext4_lblk_t lblk,
 
 int __init ext4_init_es(void)
 {
-	ext4_es_cachep = kmem_cache_create("ext4_extent_status",
-					   sizeof(struct extent_status),
-					   0, (SLAB_RECLAIM_ACCOUNT), NULL);
+	ext4_es_cachep = KMEM_CACHE(extent_status, SLAB_RECLAIM_ACCOUNT);
 	if (ext4_es_cachep == NULL)
 		return -ENOMEM;
 	return 0;
@@ -1807,9 +1805,7 @@ static void ext4_print_pending_tree(struct inode *inode)
 
 int __init ext4_init_pending(void)
 {
-	ext4_pending_cachep = kmem_cache_create("ext4_pending_reservation",
-					   sizeof(struct pending_reservation),
-					   0, (SLAB_RECLAIM_ACCOUNT), NULL);
+	ext4_pending_cachep = KMEM_CACHE(pending_reservation, SLAB_RECLAIM_ACCOUNT);
 	if (ext4_pending_cachep == NULL)
 		return -ENOMEM;
 	return 0;
diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
index 3d21eae267fc..773176e7f9f5 100644
--- a/fs/ext4/readpage.c
+++ b/fs/ext4/readpage.c
@@ -410,9 +410,8 @@ int ext4_mpage_readpages(struct inode *inode,
 
 int __init ext4_init_post_read_processing(void)
 {
-	bio_post_read_ctx_cache =
-		kmem_cache_create("ext4_bio_post_read_ctx",
-				  sizeof(struct bio_post_read_ctx), 0, 0, NULL);
+	bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, SLAB_RECLAIM_ACCOUNT);
+
 	if (!bio_post_read_ctx_cache)
 		goto fail;
 	bio_post_read_ctx_pool =
-- 
2.17.1

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[Jan Kara]

On Wed 09-11-22 07:38:22, JunChao Sun wrote:
> Replace kmem_cache_create with KMEM_CACHE macro that
> guaranteed struct alignment
> 
> Signed-off-by: JunChao Sun <sunjunchao2870@gmail.com>

Yeah, nice cleanups. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  fs/ext4/extents_status.c | 8 ++------
>  fs/ext4/readpage.c       | 5 ++---
>  2 files changed, 4 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index cd0a861853e3..97eccc0028a1 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -155,9 +155,7 @@ static void __revise_pending(struct inode *inode, ext4_lblk_t lblk,
>  
>  int __init ext4_init_es(void)
>  {
> -	ext4_es_cachep = kmem_cache_create("ext4_extent_status",
> -					   sizeof(struct extent_status),
> -					   0, (SLAB_RECLAIM_ACCOUNT), NULL);
> +	ext4_es_cachep = KMEM_CACHE(extent_status, SLAB_RECLAIM_ACCOUNT);
>  	if (ext4_es_cachep == NULL)
>  		return -ENOMEM;
>  	return 0;
> @@ -1807,9 +1805,7 @@ static void ext4_print_pending_tree(struct inode *inode)
>  
>  int __init ext4_init_pending(void)
>  {
> -	ext4_pending_cachep = kmem_cache_create("ext4_pending_reservation",
> -					   sizeof(struct pending_reservation),
> -					   0, (SLAB_RECLAIM_ACCOUNT), NULL);
> +	ext4_pending_cachep = KMEM_CACHE(pending_reservation, SLAB_RECLAIM_ACCOUNT);
>  	if (ext4_pending_cachep == NULL)
>  		return -ENOMEM;
>  	return 0;
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index 3d21eae267fc..773176e7f9f5 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -410,9 +410,8 @@ int ext4_mpage_readpages(struct inode *inode,
>  
>  int __init ext4_init_post_read_processing(void)
>  {
> -	bio_post_read_ctx_cache =
> -		kmem_cache_create("ext4_bio_post_read_ctx",
> -				  sizeof(struct bio_post_read_ctx), 0, 0, NULL);
> +	bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, SLAB_RECLAIM_ACCOUNT);
> +
>  	if (!bio_post_read_ctx_cache)
>  		goto fail;
>  	bio_post_read_ctx_pool =
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[Eric Biggers]

On Wed, Nov 09, 2022 at 07:38:22AM -0800, JunChao Sun wrote:
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index 3d21eae267fc..773176e7f9f5 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -410,9 +410,8 @@ int ext4_mpage_readpages(struct inode *inode,
>  
>  int __init ext4_init_post_read_processing(void)
>  {
> -	bio_post_read_ctx_cache =
> -		kmem_cache_create("ext4_bio_post_read_ctx",
> -				  sizeof(struct bio_post_read_ctx), 0, 0, NULL);
> +	bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, SLAB_RECLAIM_ACCOUNT);
> +

Why use SLAB_RECLAIM_ACCOUNT here?

- Eric

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[JunChao Sun]

Yeah, maybe we should remove the SLAB_RECLAIM_ACCOUNT flag for static
slab, and 16828088f9e51815 ("ext4: use KMEM_CACHE instead of
kmem_cache_create") have done so. But should we remove
SLAB_RECLAIM_ACCOUNT in this patch or belong to a separate patch?

Eric Biggers <ebiggers@kernel.org> 于2022年11月10日周四 02:00写道：
>
> On Wed, Nov 09, 2022 at 07:38:22AM -0800, JunChao Sun wrote:
> > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> > index 3d21eae267fc..773176e7f9f5 100644
> > --- a/fs/ext4/readpage.c
> > +++ b/fs/ext4/readpage.c
> > @@ -410,9 +410,8 @@ int ext4_mpage_readpages(struct inode *inode,
> >
> >  int __init ext4_init_post_read_processing(void)
> >  {
> > -     bio_post_read_ctx_cache =
> > -             kmem_cache_create("ext4_bio_post_read_ctx",
> > -                               sizeof(struct bio_post_read_ctx), 0, 0, NULL);
> > +     bio_post_read_ctx_cache = KMEM_CACHE(bio_post_read_ctx, SLAB_RECLAIM_ACCOUNT);
> > +
>
> Why use SLAB_RECLAIM_ACCOUNT here?
>
> - Eric

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[Eric Biggers]

On Thu, Nov 10, 2022 at 08:53:26AM +0800, JunChao Sun wrote:
> Yeah, maybe we should remove the SLAB_RECLAIM_ACCOUNT flag for static
> slab, and 16828088f9e51815 ("ext4: use KMEM_CACHE instead of
> kmem_cache_create") have done so. But should we remove
> SLAB_RECLAIM_ACCOUNT in this patch or belong to a separate patch?

I'd just keep the slab flags the same in this patch.  If any flags do need to be
changed, that should be a separate patch.

I think SLAB_RECLAIM_ACCOUNT is meant for for things that are directly
reclaimable, such as struct ext4_inode_info.  Inodes are evictable, and when
that happens, the corresponding struct ext4_inode_info gets freed.

bio_post_read_ctx_cache probably should use SLAB_TEMPORARY instead, since it is
only used for temporary structures during I/O.

That being said, SLAB_TEMPORARY is currently #define'd to SLAB_RECLAIM_ACCOUNT,
so currently it makes no difference in practice...

- Eric

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[JunChao Sun]

Eric Biggers <ebiggers@kernel.org> 于2022年11月10日周四 10:47写道：
>
> On Thu, Nov 10, 2022 at 08:53:26AM +0800, JunChao Sun wrote:
> > Yeah, maybe we should remove the SLAB_RECLAIM_ACCOUNT flag for static
> > slab, and 16828088f9e51815 ("ext4: use KMEM_CACHE instead of
> > kmem_cache_create") have done so. But should we remove
> > SLAB_RECLAIM_ACCOUNT in this patch or belong to a separate patch?
>
>
> > I'd just keep the slab flags the same in this patch.  If any flags do need to be
> > changed, that should be a separate patch.
> >
> > I think SLAB_RECLAIM_ACCOUNT is meant for for things that are directly
> > reclaimable, such as struct ext4_inode_info.  Inodes are evictable, and when
> > that happens, the corresponding struct ext4_inode_info gets freed.
> >
> > bio_post_read_ctx_cache probably should use SLAB_TEMPORARY instead, since it is
> > only used for temporary structures during I/O.
> >
> > That being said, SLAB_TEMPORARY is currently #define'd to SLAB_RECLAIM_ACCOUNT,
> > so currently it makes no difference in practice...

Thanks for clarifying. I will send a separate patch to remove
SLAB_RECLAIM_ACCOUNT.

>
> - Eric

Re: [PATCH] ext4: replace kmem_cache_create with KMEM_CACHE

[JunChao Sun]

Eric Biggers <ebiggers@kernel.org> 于2022年11月10日周四 10:47写道：
>
> On Thu, Nov 10, 2022 at 08:53:26AM +0800, JunChao Sun wrote:
> > Yeah, maybe we should remove the SLAB_RECLAIM_ACCOUNT flag for static
> > slab, and 16828088f9e51815 ("ext4: use KMEM_CACHE instead of
> > kmem_cache_create") have done so. But should we remove
> > SLAB_RECLAIM_ACCOUNT in this patch or belong to a separate patch?
>
> I'd just keep the slab flags the same in this patch.  If any flags do need to be
> changed, that should be a separate patch.
>
>
> > I think SLAB_RECLAIM_ACCOUNT is meant for for things that are directly
> > reclaimable, such as struct ext4_inode_info.  Inodes are evictable, and when
> > that happens, the corresponding struct ext4_inode_info gets freed.
> >
> > bio_post_read_ctx_cache probably should use SLAB_TEMPORARY instead, since it is
> > only used for temporary structures during I/O.
How to decide whether to use SLAB_RECLAIM_ACCOUNT or not when creating
a slab cache? Is it based on whether the object is reclaimable or
evictable, or the amount of memory the object may use? If the first,
how to know whether an object is reclaimable or evictable?
Btw, I saw that ext4 called ext4_es_register_shrinker() to register
shrinker for struct extent_status, so SLAB_RECLAIM_ACCOUNT should be
used when creating ext4_es_cachep, right?
>
> That being said, SLAB_TEMPORARY is currently #define'd to SLAB_RECLAIM_ACCOUNT,
> so currently it makes no difference in practice...
>
> - Eric