[f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Chao Yu]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Jaegeuk Kim]

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

Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

On Mon, May 18, 2020 at 12:10:16PM -0700, Jaegeuk Kim wrote:
> 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.
> 

I went through every remaining caller of f2fs_kmalloc() and f2fs_kzalloc().  I
think we're fine, except for possibly the allocation of blkz_seq in
init_blkz_info().  How many zones can we expect on a zoned block device?

Other than that, the largest fixed-size allocation is 8536 bytes
(struct discard_cmd_control).  And the variable-size allocations are all a page
or less, except for xattr buffers which maybe can be larger, but the VFS uses
kmalloc() for those too.

Anyway, f2fs used to allocate megabytes with kmalloc(), so I'm not surprised you
had issues before.  But that's not a good reason to make *every* caller
potentially get vmalloc()'ed memory, in the process introducing bugs where
vmalloc() memory isn't handled correctly.

- Eric


_______________________________________________
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: don't return vmalloc() memory from f2fs_kmalloc()

[Jaegeuk Kim]

On 06/04, Eric Biggers wrote:
> On Mon, May 18, 2020 at 12:10:16PM -0700, Jaegeuk Kim wrote:
> > 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.
> > 
> 
> I went through every remaining caller of f2fs_kmalloc() and f2fs_kzalloc().  I
> think we're fine, except for possibly the allocation of blkz_seq in
> init_blkz_info().  How many zones can we expect on a zoned block device?

In the worst case, it can be 16TB / 2MB = 8M entries.

> 
> Other than that, the largest fixed-size allocation is 8536 bytes
> (struct discard_cmd_control).  And the variable-size allocations are all a page
> or less, except for xattr buffers which maybe can be larger, but the VFS uses
> kmalloc() for those too.
> 
> Anyway, f2fs used to allocate megabytes with kmalloc(), so I'm not surprised you
> had issues before.  But that's not a good reason to make *every* caller
> potentially get vmalloc()'ed memory, in the process introducing bugs where
> vmalloc() memory isn't handled correctly.

Thank you for checking all the cases. Let me know, if this is the final version.
I may be able to merge and try to see if somebody will complain later. :P

Thanks,

> 
> - Eric


_______________________________________________
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: don't return vmalloc() memory from f2fs_kmalloc()

[Eric Biggers]

On Thu, Jun 04, 2020 at 06:15:35PM -0700, Jaegeuk Kim wrote:
> > 
> > I went through every remaining caller of f2fs_kmalloc() and f2fs_kzalloc().  I
> > think we're fine, except for possibly the allocation of blkz_seq in
> > init_blkz_info().  How many zones can we expect on a zoned block device?
> 
> In the worst case, it can be 16TB / 2MB = 8M entries.

In that case, I'll make init_blkz_info() use f2fs_kvzalloc() too.

- Eric


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