Re: [PATCH v1 07/14] fs: Add aop_flags parameter to create_page_buffers()

[Stefan Roesch <shr@fb.com>]

On 2/16/22 10:34 AM, Matthew Wilcox wrote:
> On Wed, Feb 16, 2022 at 10:30:33AM -0800, Stefan Roesch wrote:
>> On 2/14/22 10:14 AM, Matthew Wilcox wrote:
>>> On Mon, Feb 14, 2022 at 09:43:56AM -0800, Stefan Roesch wrote:
>>>> This adds the aop_flags parameter to the create_page_buffers function.
>>>> When AOP_FLAGS_NOWAIT parameter is set, the atomic allocation flag is
>>>> set. The AOP_FLAGS_NOWAIT flag is set, when async buffered writes are
>>>> enabled.
>>>
>>> Why is this better than passing in gfp flags directly?
>>>
>>
>> I don't think that gfp flags are a great fit here. We only want to pass in
>> a nowait flag and this does not map nicely to a gfp flag. 
> 
> ... what?  The only thing you do with this flag is use it to choose
> some gfp flags.  Pass those gfp flags in directly.
> 
>>>> +		gfp_t gfp = GFP_NOFS | __GFP_ACCOUNT;
>>>> +
>>>> +		if (aop_flags & AOP_FLAGS_NOWAIT) {
>>>> +			gfp |= GFP_ATOMIC | __GFP_NOWARN;
>>>> +			gfp &= ~__GFP_DIRECT_RECLAIM;
>>>> +		} else {
>>>> +			gfp |= __GFP_NOFAIL;
>>>> +		}
> 
> The flags you've chosen here are also bonkers, but I'm not sure that
> it's worth explaining to you why if you're this resistant to making
> obvious corrections to your patches.


Sorry my comment was for patch 1 not patch 7.