Useless test in alloc_multiple_bios

Useless test in alloc_multiple_bios

[Mikulas Patocka]

Hi

I found this piece of code in alloc_multiple_bios:
int try = (gfp_flag & GFP_NOWAIT) ? 0 : 1;

The problem is that GFP_NOWAIT includes __GFP_KSWAPD_RECLAIM and GFP_IO 
also includes __GFP_KSWAPD_RECLAIM - so the test always returns true and 
we always start with try = 0. This code was introduced by the commit 
4a2fe2960891f1ccd7805d0973284fd44c2f12b4.

I am inclined to remove this logic at all and always start with try = 0; 
- trying to allocate bios first with GFP_NOWAIT makes no harm (and it 
improves performance because we don't need to grab the lock) and if the 
allocation fails, it is retried with GFP_NOIO.

I'd like to ask - if you think that this piece of code has some purpose, 
please describe the purpose.

Mikulas

Re: Useless test in alloc_multiple_bios

[Ming Lei]

On Wed, May 15, 2024 at 08:15:20PM +0200, Mikulas Patocka wrote:
> Hi
> 
> I found this piece of code in alloc_multiple_bios:
> int try = (gfp_flag & GFP_NOWAIT) ? 0 : 1;

I guess the above check is supposed to be (gfp_flag == GFP_NOWAIT)?

> 
> The problem is that GFP_NOWAIT includes __GFP_KSWAPD_RECLAIM and GFP_IO 
> also includes __GFP_KSWAPD_RECLAIM - so the test always returns true and 
> we always start with try = 0. This code was introduced by the commit 
> 4a2fe2960891f1ccd7805d0973284fd44c2f12b4.
> 
> I am inclined to remove this logic at all and always start with try = 0; 
> - trying to allocate bios first with GFP_NOWAIT makes no harm (and it 
> improves performance because we don't need to grab the lock) and if the 
> allocation fails, it is retried with GFP_NOIO.

I think it is fine to simplify the logic in this way, and now GFP_NOWAIT is
only for sending empty flush, which could be from fast path, and abnormal IO
is supposed to be in slow path.

Thanks,
Ming