Linux-f2fs-devel Archive on lore.kernel.org
 help / color / Atom feed
* [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
@ 2020-05-05 20:48 Eric Biggers
  2020-05-06  7:43 ` Chao Yu
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Eric Biggers @ 2020-05-05 20:48 UTC (permalink / raw)
  To: linux-f2fs-devel

From: Eric Biggers <ebiggers@google.com>

kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
and f2fs_kvmalloc(), both return both kinds of memory.

It's redundant to have two functions that do the same thing, and also
breaking the standard naming convention is causing bugs since people
assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
e.g. the various allocations in fs/f2fs/compress.c.

Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
re-introducing the allocation failures that the vmalloc fallback was
intended to fix, convert the largest allocations to use f2fs_kvmalloc().

Signed-off-by: Eric Biggers <ebiggers@google.com>
---
 fs/f2fs/checkpoint.c | 4 ++--
 fs/f2fs/f2fs.h       | 8 +-------
 fs/f2fs/node.c       | 8 ++++----
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 97b6378554b406..ac5b47f15f5e77 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
 	int i;
 	int err;
 
-	sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
-				 GFP_KERNEL);
+	sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
+				  GFP_KERNEL);
 	if (!sbi->ckpt)
 		return -ENOMEM;
 	/*
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index d036a31a97e84e..bc4c5b9b1bf14c 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode *inode)
 static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
 					size_t size, gfp_t flags)
 {
-	void *ret;
-
 	if (time_to_inject(sbi, FAULT_KMALLOC)) {
 		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
 		return NULL;
 	}
 
-	ret = kmalloc(size, flags);
-	if (ret)
-		return ret;
-
-	return kvmalloc(size, flags);
+	return kmalloc(size, flags);
 }
 
 static inline void *f2fs_kzalloc(struct f2fs_sb_info *sbi,
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 4da0d8713df5cb..e660af55565c61 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -2934,7 +2934,7 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
 		return 0;
 
 	nm_i->nat_bits_blocks = F2FS_BLK_ALIGN((nat_bits_bytes << 1) + 8);
-	nm_i->nat_bits = f2fs_kzalloc(sbi,
+	nm_i->nat_bits = f2fs_kvzalloc(sbi,
 			nm_i->nat_bits_blocks << F2FS_BLKSIZE_BITS, GFP_KERNEL);
 	if (!nm_i->nat_bits)
 		return -ENOMEM;
@@ -3067,9 +3067,9 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi)
 	int i;
 
 	nm_i->free_nid_bitmap =
-		f2fs_kzalloc(sbi, array_size(sizeof(unsigned char *),
-					     nm_i->nat_blocks),
-			     GFP_KERNEL);
+		f2fs_kvzalloc(sbi, array_size(sizeof(unsigned char *),
+					      nm_i->nat_blocks),
+			      GFP_KERNEL);
 	if (!nm_i->free_nid_bitmap)
 		return -ENOMEM;
 
-- 
2.26.2



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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-05 20:48 [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc() Eric Biggers
@ 2020-05-06  7:43 ` Chao Yu
  2020-05-06 18:43   ` Eric Biggers
  2020-05-15 19:13 ` Eric Biggers
  2020-05-18 18:06 ` Jaegeuk Kim
  2 siblings, 1 reply; 8+ messages in thread
From: Chao Yu @ 2020-05-06  7:43 UTC (permalink / raw)
  To: Eric Biggers, linux-f2fs-devel

On 2020/5/6 4:48, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> and f2fs_kvmalloc(), both return both kinds of memory.
> 
> It's redundant to have two functions that do the same thing, and also
> breaking the standard naming convention is causing bugs since people
> assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> e.g. the various allocations in fs/f2fs/compress.c.
> 
> Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> re-introducing the allocation failures that the vmalloc fallback was
> intended to fix, convert the largest allocations to use f2fs_kvmalloc().

I've submitted one patch since you suggested when commented in compression
support patch.

I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(),
and keep the order of kmalloc - failed - kvmalloc.

Thanks,

> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/f2fs/checkpoint.c | 4 ++--
>  fs/f2fs/f2fs.h       | 8 +-------
>  fs/f2fs/node.c       | 8 ++++----
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 97b6378554b406..ac5b47f15f5e77 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
>  	int i;
>  	int err;
>  
> -	sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
> -				 GFP_KERNEL);
> +	sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
> +				  GFP_KERNEL);
>  	if (!sbi->ckpt)
>  		return -ENOMEM;
>  	/*
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d036a31a97e84e..bc4c5b9b1bf14c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode *inode)
>  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
>  					size_t size, gfp_t flags)
>  {
> -	void *ret;
> -
>  	if (time_to_inject(sbi, FAULT_KMALLOC)) {
>  		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
>  		return NULL;
>  	}
>  
> -	ret = kmalloc(size, flags);
> -	if (ret)
> -		return ret;
> -
> -	return kvmalloc(size, flags);
> +	return kmalloc(size, flags);
>  }
>  
>  static inline void *f2fs_kzalloc(struct f2fs_sb_info *sbi,
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 4da0d8713df5cb..e660af55565c61 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2934,7 +2934,7 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>  		return 0;
>  
>  	nm_i->nat_bits_blocks = F2FS_BLK_ALIGN((nat_bits_bytes << 1) + 8);
> -	nm_i->nat_bits = f2fs_kzalloc(sbi,
> +	nm_i->nat_bits = f2fs_kvzalloc(sbi,
>  			nm_i->nat_bits_blocks << F2FS_BLKSIZE_BITS, GFP_KERNEL);
>  	if (!nm_i->nat_bits)
>  		return -ENOMEM;
> @@ -3067,9 +3067,9 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi)
>  	int i;
>  
>  	nm_i->free_nid_bitmap =
> -		f2fs_kzalloc(sbi, array_size(sizeof(unsigned char *),
> -					     nm_i->nat_blocks),
> -			     GFP_KERNEL);
> +		f2fs_kvzalloc(sbi, array_size(sizeof(unsigned char *),
> +					      nm_i->nat_blocks),
> +			      GFP_KERNEL);
>  	if (!nm_i->free_nid_bitmap)
>  		return -ENOMEM;
>  
> 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-06  7:43 ` Chao Yu
@ 2020-05-06 18:43   ` Eric Biggers
  2020-05-07  3:13     ` Chao Yu
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-05-06 18:43 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-f2fs-devel

On Wed, May 06, 2020 at 03:43:36PM +0800, Chao Yu wrote:
> On 2020/5/6 4:48, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> > kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> > and f2fs_kvmalloc(), both return both kinds of memory.
> > 
> > It's redundant to have two functions that do the same thing, and also
> > breaking the standard naming convention is causing bugs since people
> > assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> > e.g. the various allocations in fs/f2fs/compress.c.
> > 
> > Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> > re-introducing the allocation failures that the vmalloc fallback was
> > intended to fix, convert the largest allocations to use f2fs_kvmalloc().
> 
> I've submitted one patch since you suggested when commented in compression
> support patch.
> 
> I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(),
> and keep the order of kmalloc - failed - kvmalloc.
> 

I think you're talking about
https://lkml.kernel.org/r/20191105030451.GA55090@jaegeuk-macbookpro.roam.corp.google.com?

I think that making the large allocations use kvmalloc(), as this patch does, is
good enough to address any memory allocation failures that may have been
encountered in the past.  We don't need to switch all allocations to use
kvmalloc(), as there's no benefit for small allocations.

- Eric


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-06 18:43   ` Eric Biggers
@ 2020-05-07  3:13     ` Chao Yu
  0 siblings, 0 replies; 8+ messages in thread
From: Chao Yu @ 2020-05-07  3:13 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-f2fs-devel

On 2020/5/7 2:43, Eric Biggers wrote:
> On Wed, May 06, 2020 at 03:43:36PM +0800, Chao Yu wrote:
>> On 2020/5/6 4:48, Eric Biggers wrote:
>>> From: Eric Biggers <ebiggers@google.com>
>>>
>>> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
>>> kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
>>> and f2fs_kvmalloc(), both return both kinds of memory.
>>>
>>> It's redundant to have two functions that do the same thing, and also
>>> breaking the standard naming convention is causing bugs since people
>>> assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
>>> e.g. the various allocations in fs/f2fs/compress.c.
>>>
>>> Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
>>> re-introducing the allocation failures that the vmalloc fallback was
>>> intended to fix, convert the largest allocations to use f2fs_kvmalloc().
>>
>> I've submitted one patch since you suggested when commented in compression
>> support patch.
>>
>> I remember Jaegeuk prefer to use f2fs_kvmalloc() to instead f2fs_kmalloc(),
>> and keep the order of kmalloc - failed - kvmalloc.
>>
> 
> I think you're talking about
> https://lkml.kernel.org/r/20191105030451.GA55090@jaegeuk-macbookpro.roam.corp.google.com?
> 
> I think that making the large allocations use kvmalloc(), as this patch does, is
> good enough to address any memory allocation failures that may have been
> encountered in the past.  We don't need to switch all allocations to use
> kvmalloc(), as there's no benefit for small allocations.

Yeah, I agreed, and in ENOMEM case, small-sized kvmalloc increases allocating path
length unnecessarily.

Thanks,

> 
> - Eric
> .
> 


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-05 20:48 [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc() Eric Biggers
  2020-05-06  7:43 ` Chao Yu
@ 2020-05-15 19:13 ` Eric Biggers
  2020-05-18 18:06 ` Jaegeuk Kim
  2 siblings, 0 replies; 8+ messages in thread
From: Eric Biggers @ 2020-05-15 19:13 UTC (permalink / raw)
  To: linux-f2fs-devel, Jaegeuk Kim

On Tue, May 05, 2020 at 01:48:23PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> and f2fs_kvmalloc(), both return both kinds of memory.
> 
> It's redundant to have two functions that do the same thing, and also
> breaking the standard naming convention is causing bugs since people
> assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> e.g. the various allocations in fs/f2fs/compress.c.
> 
> Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> re-introducing the allocation failures that the vmalloc fallback was
> intended to fix, convert the largest allocations to use f2fs_kvmalloc().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>

Jaegeuk, are you planning to apply this patch?

- Eric


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-05 20:48 [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc() Eric Biggers
  2020-05-06  7:43 ` Chao Yu
  2020-05-15 19:13 ` Eric Biggers
@ 2020-05-18 18:06 ` Jaegeuk Kim
  2020-05-18 18:35   ` Eric Biggers
  2 siblings, 1 reply; 8+ messages in thread
From: Jaegeuk Kim @ 2020-05-18 18:06 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-f2fs-devel

On 05/05, Eric Biggers wrote:
> From: Eric Biggers <ebiggers@google.com>
> 
> kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> and f2fs_kvmalloc(), both return both kinds of memory.
> 
> It's redundant to have two functions that do the same thing, and also
> breaking the standard naming convention is causing bugs since people
> assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> e.g. the various allocations in fs/f2fs/compress.c.
> 
> Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> re-introducing the allocation failures that the vmalloc fallback was
> intended to fix, convert the largest allocations to use f2fs_kvmalloc().
> 
> Signed-off-by: Eric Biggers <ebiggers@google.com>
> ---
>  fs/f2fs/checkpoint.c | 4 ++--
>  fs/f2fs/f2fs.h       | 8 +-------
>  fs/f2fs/node.c       | 8 ++++----
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 97b6378554b406..ac5b47f15f5e77 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
>  	int i;
>  	int err;
>  
> -	sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
> -				 GFP_KERNEL);
> +	sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
> +				  GFP_KERNEL);
>  	if (!sbi->ckpt)
>  		return -ENOMEM;
>  	/*
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index d036a31a97e84e..bc4c5b9b1bf14c 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode *inode)
>  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
>  					size_t size, gfp_t flags)
>  {
> -	void *ret;
> -
>  	if (time_to_inject(sbi, FAULT_KMALLOC)) {
>  		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
>  		return NULL;
>  	}
>  
> -	ret = kmalloc(size, flags);
> -	if (ret)
> -		return ret;
> -
> -	return kvmalloc(size, flags);
> +	return kmalloc(size, flags);

IIRC, sometimes, we suffered from ENOMEM from kmalloc, as some structures
depended on the disk capacity. I can't remember exactly which structure tho.

>  }
>  
>  static inline void *f2fs_kzalloc(struct f2fs_sb_info *sbi,
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 4da0d8713df5cb..e660af55565c61 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -2934,7 +2934,7 @@ static int __get_nat_bitmaps(struct f2fs_sb_info *sbi)
>  		return 0;
>  
>  	nm_i->nat_bits_blocks = F2FS_BLK_ALIGN((nat_bits_bytes << 1) + 8);
> -	nm_i->nat_bits = f2fs_kzalloc(sbi,
> +	nm_i->nat_bits = f2fs_kvzalloc(sbi,
>  			nm_i->nat_bits_blocks << F2FS_BLKSIZE_BITS, GFP_KERNEL);
>  	if (!nm_i->nat_bits)
>  		return -ENOMEM;
> @@ -3067,9 +3067,9 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi)
>  	int i;
>  
>  	nm_i->free_nid_bitmap =
> -		f2fs_kzalloc(sbi, array_size(sizeof(unsigned char *),
> -					     nm_i->nat_blocks),
> -			     GFP_KERNEL);
> +		f2fs_kvzalloc(sbi, array_size(sizeof(unsigned char *),
> +					      nm_i->nat_blocks),
> +			      GFP_KERNEL);
>  	if (!nm_i->free_nid_bitmap)
>  		return -ENOMEM;
>  
> -- 
> 2.26.2
> 
> 
> 
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-18 18:06 ` Jaegeuk Kim
@ 2020-05-18 18:35   ` Eric Biggers
  2020-05-18 19:10     ` Jaegeuk Kim
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Biggers @ 2020-05-18 18:35 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-f2fs-devel

On Mon, May 18, 2020 at 11:06:48AM -0700, Jaegeuk Kim wrote:
> On 05/05, Eric Biggers wrote:
> > From: Eric Biggers <ebiggers@google.com>
> > 
> > kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> > kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> > and f2fs_kvmalloc(), both return both kinds of memory.
> > 
> > It's redundant to have two functions that do the same thing, and also
> > breaking the standard naming convention is causing bugs since people
> > assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> > e.g. the various allocations in fs/f2fs/compress.c.
> > 
> > Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> > re-introducing the allocation failures that the vmalloc fallback was
> > intended to fix, convert the largest allocations to use f2fs_kvmalloc().
> > 
> > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > ---
> >  fs/f2fs/checkpoint.c | 4 ++--
> >  fs/f2fs/f2fs.h       | 8 +-------
> >  fs/f2fs/node.c       | 8 ++++----
> >  3 files changed, 7 insertions(+), 13 deletions(-)
> > 
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 97b6378554b406..ac5b47f15f5e77 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
> >  	int i;
> >  	int err;
> >  
> > -	sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
> > -				 GFP_KERNEL);
> > +	sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
> > +				  GFP_KERNEL);
> >  	if (!sbi->ckpt)
> >  		return -ENOMEM;
> >  	/*
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index d036a31a97e84e..bc4c5b9b1bf14c 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode *inode)
> >  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
> >  					size_t size, gfp_t flags)
> >  {
> > -	void *ret;
> > -
> >  	if (time_to_inject(sbi, FAULT_KMALLOC)) {
> >  		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
> >  		return NULL;
> >  	}
> >  
> > -	ret = kmalloc(size, flags);
> > -	if (ret)
> > -		return ret;
> > -
> > -	return kvmalloc(size, flags);
> > +	return kmalloc(size, flags);
> 
> IIRC, sometimes, we suffered from ENOMEM from kmalloc, as some structures
> depended on the disk capacity. I can't remember exactly which structure tho.
> 

I think this patch already addresses that, by changing the large allocations to
use f2fs_kvmalloc().

- Eric


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()
  2020-05-18 18:35   ` Eric Biggers
@ 2020-05-18 19:10     ` Jaegeuk Kim
  0 siblings, 0 replies; 8+ messages in thread
From: Jaegeuk Kim @ 2020-05-18 19:10 UTC (permalink / raw)
  To: Eric Biggers; +Cc: linux-f2fs-devel

On 05/18, Eric Biggers wrote:
> On Mon, May 18, 2020 at 11:06:48AM -0700, Jaegeuk Kim wrote:
> > On 05/05, Eric Biggers wrote:
> > > From: Eric Biggers <ebiggers@google.com>
> > > 
> > > kmalloc() returns kmalloc'ed memory, and kvmalloc() returns either
> > > kmalloc'ed or vmalloc'ed memory.  But the f2fs wrappers, f2fs_kmalloc()
> > > and f2fs_kvmalloc(), both return both kinds of memory.
> > > 
> > > It's redundant to have two functions that do the same thing, and also
> > > breaking the standard naming convention is causing bugs since people
> > > assume it's safe to kfree() memory allocated by f2fs_kmalloc().  See
> > > e.g. the various allocations in fs/f2fs/compress.c.
> > > 
> > > Fix this by making f2fs_kmalloc() just use kmalloc().  And to avoid
> > > re-introducing the allocation failures that the vmalloc fallback was
> > > intended to fix, convert the largest allocations to use f2fs_kvmalloc().
> > > 
> > > Signed-off-by: Eric Biggers <ebiggers@google.com>
> > > ---
> > >  fs/f2fs/checkpoint.c | 4 ++--
> > >  fs/f2fs/f2fs.h       | 8 +-------
> > >  fs/f2fs/node.c       | 8 ++++----
> > >  3 files changed, 7 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > > index 97b6378554b406..ac5b47f15f5e77 100644
> > > --- a/fs/f2fs/checkpoint.c
> > > +++ b/fs/f2fs/checkpoint.c
> > > @@ -895,8 +895,8 @@ int f2fs_get_valid_checkpoint(struct f2fs_sb_info *sbi)
> > >  	int i;
> > >  	int err;
> > >  
> > > -	sbi->ckpt = f2fs_kzalloc(sbi, array_size(blk_size, cp_blks),
> > > -				 GFP_KERNEL);
> > > +	sbi->ckpt = f2fs_kvzalloc(sbi, array_size(blk_size, cp_blks),
> > > +				  GFP_KERNEL);
> > >  	if (!sbi->ckpt)
> > >  		return -ENOMEM;
> > >  	/*
> > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > index d036a31a97e84e..bc4c5b9b1bf14c 100644
> > > --- a/fs/f2fs/f2fs.h
> > > +++ b/fs/f2fs/f2fs.h
> > > @@ -2953,18 +2953,12 @@ static inline bool f2fs_may_extent_tree(struct inode *inode)
> > >  static inline void *f2fs_kmalloc(struct f2fs_sb_info *sbi,
> > >  					size_t size, gfp_t flags)
> > >  {
> > > -	void *ret;
> > > -
> > >  	if (time_to_inject(sbi, FAULT_KMALLOC)) {
> > >  		f2fs_show_injection_info(sbi, FAULT_KMALLOC);
> > >  		return NULL;
> > >  	}
> > >  
> > > -	ret = kmalloc(size, flags);
> > > -	if (ret)
> > > -		return ret;
> > > -
> > > -	return kvmalloc(size, flags);
> > > +	return kmalloc(size, flags);
> > 
> > IIRC, sometimes, we suffered from ENOMEM from kmalloc, as some structures
> > depended on the disk capacity. I can't remember exactly which structure tho.
> > 
> 
> I think this patch already addresses that, by changing the large allocations to
> use f2fs_kvmalloc().

Hmm, I worried a bit whether it covers every cases.

> 
> - Eric


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-05 20:48 [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc() Eric Biggers
2020-05-06  7:43 ` Chao Yu
2020-05-06 18:43   ` Eric Biggers
2020-05-07  3:13     ` Chao Yu
2020-05-15 19:13 ` Eric Biggers
2020-05-18 18:06 ` Jaegeuk Kim
2020-05-18 18:35   ` Eric Biggers
2020-05-18 19:10     ` Jaegeuk Kim

Linux-f2fs-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-f2fs-devel/0 linux-f2fs-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-f2fs-devel linux-f2fs-devel/ https://lore.kernel.org/linux-f2fs-devel \
		linux-f2fs-devel@lists.sourceforge.net
	public-inbox-index linux-f2fs-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/net.sourceforge.lists.linux-f2fs-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git