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;
Message-ID: <26b65420-2609-26a5-9cc2-c12cabd310e0@gmx.com>
Date:   Wed, 26 Oct 2022 14:42:51 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.0
Subject: Re: [PATCH v3] btrfs: volumes: Increase bioc pointer check
To:     Li zeming <zeming@nfschina.com>, clm@fb.com, josef@toxicpanda.com,
        dsterba@suse.com
Cc:     linux-btrfs@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20221026013611.2900-1-zeming@nfschina.com>
Content-Language: en-US
From:   Qu Wenruo <quwenruo.btrfs@gmx.com>
In-Reply-To: <20221026013611.2900-1-zeming@nfschina.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
Precedence: bulk


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_con=
text(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(str=
uct btrfs_fs_info *fs_info,
>    * array of stripes.
>    * For READ, it also needs to be supported using the same mirror numbe=
r.
>    *
> - * If the requested block is not left of the left cursor, EIO is return=
ed. 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().

Thanks,
Qu
>    * can happen because btrfs_num_copies() returns one more in the dev-r=
eplace
>    * case.
>    */