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

Stefan Roesch
2022-02-16T18:35:44Z

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.