Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 14 Nov 2022 21:45:45 +0100
From:   David Sterba <dsterba@suse.cz>
To:     Qu Wenruo <quwenruo.btrfs@gmx.com>
Cc:     Li zeming <zeming@nfschina.com>, clm@fb.com, josef@toxicpanda.com,
        dsterba@suse.com, linux-btrfs@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] btrfs: volumes: Increase bioc pointer check
Message-ID: <20221114204545.GB5824@twin.jikos.cz>
Reply-To: dsterba@suse.cz
References: <20221026013611.2900-1-zeming@nfschina.com>
 <26b65420-2609-26a5-9cc2-c12cabd310e0@gmx.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <26b65420-2609-26a5-9cc2-c12cabd310e0@gmx.com>
User-Agent: Mutt/1.5.23.1-rc1 (2014-03-12)
Precedence: bulk

On Wed, Oct 26, 2022 at 02:42:51PM +0800, Qu Wenruo wrote:
> On 2022/10/26 09:36, Li zeming wrote:
> > The __GFP_NOFAIL flag will cause memory to be allocated an infinite
> > number of times until the allocation is successful, but it is best to
> > use it only for very necessary code, and try not to use it.
> >
> > The alloc_btrfs_io_context function looks a little closer to normal
> > (excuse my analysis), but I think we can remove __GFP_NOFAIL from it and
> > add a bioc pointer allocation check that returns NULL if the allocation
> > fails.
> >
> > Signed-off-by: Li zeming <zeming@nfschina.com>
> > ---
> >   v2: Add annotation vocabulary modify, remove __GFP_NOFAIL flag.
> >   v3: Modifying the Description.
> >
> >   fs/btrfs/volumes.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 064ab2a79c80..b8d901f58995 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -5891,7 +5891,9 @@ static struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_
> >   		 * and the stripes.
> >   		 */
> >   		sizeof(u64) * (total_stripes),
> > -		GFP_NOFS|__GFP_NOFAIL);
> > +		GFP_NOFS);
> > +	if (!bioc)
> > +		return NULL;
> >
> >   	atomic_set(&bioc->error, 0);
> >   	refcount_set(&bioc->refs, 1);
> > @@ -6071,7 +6073,7 @@ struct btrfs_discard_stripe *btrfs_map_discard(struct btrfs_fs_info *fs_info,
> >    * array of stripes.
> >    * For READ, it also needs to be supported using the same mirror number.
> >    *
> > - * If the requested block is not left of the left cursor, EIO is returned. This
> > + * If the requested block is not left of the left cursor, EIO should be returned. This
> 
> Is there any need for this change?
> 
> I don't think your patch has even touched the call chain of
> get_extra_mirror_from_replace().

That's maybe an accidental change, not relevant to the NOFAIL removal.