Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail

From: Gao Xiang <hsiangkao@aol.com>
To: Jaegeuk Kim <jaegeuk@kernel.org>
Cc: "Theodore Y. Ts'o" <tytso@mit.edu>,
	Gao Xiang <gaoxiang25@huawei.com>,
	Jonathan Corbet <corbet@lwn.net>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [f2fs-dev] [PATCH] f2fs: bio_alloc should never fail
Date: Thu, 31 Oct 2019 00:52:39 +0800	[thread overview]
Message-ID: <20191030165226.GC3953@hsiangkao-HP-ZHAN-66-Pro-G1> (raw)
In-Reply-To: <20191030163313.GB34056@jaegeuk-macbookpro.roam.corp.google.com>

On Wed, Oct 30, 2019 at 09:33:13AM -0700, Jaegeuk Kim wrote:
> On 10/30, Theodore Y. Ts'o wrote:
> > On Wed, Oct 30, 2019 at 11:50:37PM +0800, Gao Xiang wrote:
> > > 
> > > So I'm curious about the original issue in commit 740432f83560
> > > ("f2fs: handle failed bio allocation"). Since f2fs manages multiple write
> > > bios with its internal fio but it seems the commit is not helpful to
> > > resolve potential mempool deadlock (I'm confused since no calltrace,
> > > maybe I'm wrong)...
> > 
> > Two possibilities come to mind.  (a) It may be that on older kernels
> > (when f2fs is backported to older Board Support Package kernels from
> > the SOC vendors) didn't have the bio_alloc() guarantee, so it was
> > necessary on older kernels, but not on upstream, or (b) it wasn't
> > *actually* possible for bio_alloc() to fail and someone added the
> > error handling in 740432f83560 out of paranoia.
> 
> Yup, I was checking old device kernels but just stopped digging it out.
> Instead, I hesitate to apply this patch since I can't get why we need to
> get rid of this code for clean-up purpose. This may be able to bring
> some hassles when backporting to android/device kernels.

Yes, got you concern. As I said in other patches for many times, since
you're the maintainer of f2fs, it's all up to you (I'm not paranoia).
However, I think there are 2 valid reasons:

 1) As a newbie of Linux filesystem. When I study or work on f2fs,
    and I saw these misleading code, I think I will produce similar
    code in the future (not everyone refers comments above bio_alloc),
    so such usage will spread (since one could refer some sample code
    from exist code);

 2) Since it's upstream, I personally think appropriate cleanup is ok (anyway
    it kills net 20+ line dead code), and this patch I think isn't so harmful
    for backporting.

Thanks,
Gao Xiang

> 
> > 
> > (Hence my suggestion that in the ext4 version of the patch, we add a
> > code comment justifying why there was no error checking, to make it
> > clear that this was a deliberate choice.  :-)
> > 
> > 						- Ted