From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8A218C433DF for ; Thu, 4 Jun 2020 21:06:58 +0000 (UTC) Received: from lists.sourceforge.net (lists.sourceforge.net [216.105.38.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 513AC2067B; Thu, 4 Jun 2020 21:06:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=sourceforge.net header.i=@sourceforge.net header.b="L59xuAdc"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=sf.net header.i=@sf.net header.b="ihW0xSxe"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="Tw1VGIo2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 513AC2067B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=kernel.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linux-f2fs-devel-bounces@lists.sourceforge.net Received: from [127.0.0.1] (helo=sfs-ml-4.v29.lw.sourceforge.com) by sfs-ml-4.v29.lw.sourceforge.com with esmtp (Exim 4.90_1) (envelope-from ) id 1jgx4j-0002oY-5u; Thu, 04 Jun 2020 21:06:57 +0000 Received: from [172.30.20.202] (helo=mx.sourceforge.net) by sfs-ml-4.v29.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.90_1) (envelope-from ) id 1jgx4i-0002oS-MG for linux-f2fs-devel@lists.sourceforge.net; Thu, 04 Jun 2020 21:06:56 +0000 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sourceforge.net; s=x; h=In-Reply-To:Content-Type:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To:Content-Transfer-Encoding: Content-ID:Content-Description:Resent-Date:Resent-From:Resent-Sender: Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help:List-Unsubscribe: List-Subscribe:List-Post:List-Owner:List-Archive; bh=xgWYnP0wIAvtZ3thPLVdLkBQNwxihvirLTJpvp94MLI=; b=L59xuAdcqSX7bj8ERNmp2IJbE+ Rhv6hXVp9JGMVwxCnEpsuRk8zVxOpId1AR+5vlize/H+7lc0pGJiJfHvm+93yiZbMF4PNVk7jbFjf 3alNvyEilVtncbQhCuJ0qFkhbXKO+Db/DXmu0uRpFNqFi+Oo9tH3m8PhrGkwTc9CiiYc=; DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sf.net; s=x ; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To :From:Date:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=xgWYnP0wIAvtZ3thPLVdLkBQNwxihvirLTJpvp94MLI=; b=ihW0xSxeeglxXVXQXsdTuLj6w4 0a0DYdAjF31P8qcAeZeVKRKgoLXTf+gT6fTIuUxsAh4PdEFMXmp/ICn0yhxQpYClGnR0x9AaJxvcq 6d1PGJ9ka5XGe5Hf5RHC1e29AcVUVagoONH7emnghKCLIftGdywm2z5Rtyf9isX0yPKI=; Received: from mail.kernel.org ([198.145.29.99]) by sfi-mx-4.v28.lw.sourceforge.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.92.2) id 1jgx4f-00C7FM-0s for linux-f2fs-devel@lists.sourceforge.net; Thu, 04 Jun 2020 21:06:56 +0000 Received: from sol.localdomain (c-107-3-166-239.hsd1.ca.comcast.net [107.3.166.239]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 8C5DC2067B; Thu, 4 Jun 2020 21:06:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1591304807; bh=g/gdyeFpBHokLKrQe6xCW/yCnhqXNIihjrHwcCcMn3s=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=Tw1VGIo2Hu5KJlkvMxls7SKhk++Yml4QtUfWxudQ6SxE+xOokTNjw4YDhZSlrNEDU 8Qp0rN8tf3RwpsitWIwnHLSnvlXMyx73SPCQqq4JzhfPZQGRQQ7gcIjvlmqW4UPmbC WNfaFT63HnoKtnNSv35MCJdmQW5wAKWMX5CQKpx4= Date: Thu, 4 Jun 2020 14:06:46 -0700 From: Eric Biggers To: Jaegeuk Kim Message-ID: <20200604210646.GA855@sol.localdomain> References: <20200505204823.211214-1-ebiggers@kernel.org> <20200518180648.GB218254@google.com> <20200518183542.GA121709@gmail.com> <20200518191016.GA182581@google.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200518191016.GA182581@google.com> X-Headers-End: 1jgx4f-00C7FM-0s Subject: Re: [f2fs-dev] [PATCH] f2fs: don't return vmalloc() memory from f2fs_kmalloc() X-BeenThere: linux-f2fs-devel@lists.sourceforge.net X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-f2fs-devel@lists.sourceforge.net Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-f2fs-devel-bounces@lists.sourceforge.net 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 > > > > > > > > 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 > > > > --- > > > > 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