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