[v3] btrfs: volumes: Increase bioc pointer check

  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(-)
  

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().

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

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.
  

On Wed, Oct 26, 2022 at 09:36:11AM +0800, 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>

I've reworded the subject and changelog and patch is now in misc-next,
thanks.