Re: [PATCH RFC 5/5] non-mm: discourage the usage of __GFP_NOFAIL and encourage GFP_NOFAIL

[Vlastimil Babka <vbabka@suse.cz>]

On 7/29/24 11:56 AM, Barry Song wrote:
> On Thu, Jul 25, 2024 at 1:47 PM Barry Song <21cnbao@gmail.com> wrote:
>>
>> On Thu, Jul 25, 2024 at 2:41 AM Christoph Hellwig <hch@infradead.org> wrote:
>> >
>> > On Wed, Jul 24, 2024 at 04:39:11PM +0200, Vlastimil Babka wrote:
>> > > On 7/24/24 3:55 PM, Christoph Hellwig wrote:
>> > > > On Wed, Jul 24, 2024 at 03:47:46PM +0200, Michal Hocko wrote:
>> > > >> OK, now it makes more sense ;) I have absolutely no objections to
>> > > >> prefering scoped NO{FS,IO} interfaces of course. And that would indeed
>> > > >> eliminate a need for defining GFP_NO{FS,IO}_NOFAIL alternatives.
>> > > >
>> > > > Yes.  My proposal would be:
>> > > >
>> > > > GFP_NOFAIL without any modifiers it the only valid nofail API.
>> > >
>> > > Where GFP_NOFAIL is GFP_KERNEL | __GFP_NOFAIL (and not the more limited one
>> > > as defined in patch 4/5).
>> >
>> > Yes.
>> >
>> > > > File systems / drivers can combine іt with the scoped nofs/noio if
>> > > > needed.
>> > >
>> > > Sounds good, how quickly we can convert existing __GFP_NOFAIL users remains
>> > > to be seen...
>> >
>> > I took a quick look at the file system ones and they look pretty easy.  I
>> > think it would be good to a quick scriped run for everything that does
>> > GFP_KERNEL | __GFP_NOFAIL right now, and then spend a little time on
>> > the rest.
> 
> I assume you mean something as the below?

This would work but looks too much like a workaround to fit with the new
rules without actually fulfiling the purpose of the scopes. I.e. it's
possible this allocation is in fact part of a larger NOIO scope that should
be marked accordingly, and not just wrap this single kmalloc.

> diff --git a/drivers/md/dm-region-hash.c b/drivers/md/dm-region-hash.c
> index a4550975c27d..b90ef94b1a09 100644
> --- a/drivers/md/dm-region-hash.c
> +++ b/drivers/md/dm-region-hash.c
> @@ -291,10 +291,13 @@ static void __rh_insert(struct dm_region_hash
> *rh, struct dm_region *reg)
>  static struct dm_region *__rh_alloc(struct dm_region_hash *rh, region_t region)
>  {
>         struct dm_region *reg, *nreg;
> +       int orig_flags;
> 
>         nreg = mempool_alloc(&rh->region_pool, GFP_ATOMIC);
> +       orig_flags = memalloc_noio_save();
>         if (unlikely(!nreg))
> -               nreg = kmalloc(sizeof(*nreg), GFP_NOIO | __GFP_NOFAIL);
> +               nreg = kmalloc(sizeof(*nreg), GFP_NOFAIL);
> +       memalloc_noio_restore(orig_flags);
> 
>         nreg->state = rh->log->type->in_sync(rh->log, region, 1) ?
>                       DM_RH_CLEAN : DM_RH_NOSYNC;