lkml.org 
[lkml]   [2021]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/6] MM: improve documentation for __GFP_NOFAIL
On Thu 14-10-21 13:32:01, David Sterba wrote:
> On Wed, Oct 13, 2021 at 10:26:58AM +0200, Michal Hocko wrote:
> > > crap like this (found in btrfs):
> > >
> > > /*
> > > * We're holding a transaction handle, so use a NOFS memory
> > > * allocation context to avoid deadlock if reclaim happens.
> > > */
> > > nofs_flag = memalloc_nofs_save();
> > > value = kmalloc(size, GFP_KERNEL);
> > > memalloc_nofs_restore(nofs_flag);
> >
> > Yes this looks wrong indeed! If I were to review such a code I would ask
> > why the scope cannot match the transaction handle context. IIRC jbd does
> > that.
>
> Adding the transaction start/end as the NOFS scope is a long term plan
> and going on for years, because it's not a change we would need in
> btrfs, but rather a favor to MM to switch away from "GFP_NOFS everywhere
> because it's easy".
>
> The first step was to convert the easy cases. Almost all safe cases
> switching GFP_NOFS to GFP_KERNEL have happened. Another step is to
> convert GFP_NOFS to memalloc_nofs_save/GFP_KERNEL/memalloc_nofs_restore
> in contexts where we know we'd rely on the transaction NOFS scope in the
> future. Once this is implemented, the memalloc_nofs_* calls are deleted
> and it works as expected. Now you may argue that the switch could be
> changing GFP_NOFS to GFP_KERNEL at that time but that is not that easy
> to review or reason about in the whole transaction context in all
> allocations.
>
> This leads to code that was found in __btrfs_set_acl and called crap
> or wrong, because perhaps the background and the bigger plan is not
> immediately obvious. I hope the explanation above it puts it to the
> right perspective.

Yes it helps. Thanks for the clarification because this is far from
obvious and changelogs I've checked do not mention this high level plan.
I would have gone with a /* TODO: remove me once transactions use scopes... */
but this is obviously your call.

>
> The other class of scoped NOFS protection is around vmalloc-based
> allocations but that's for a different reason, would be solved by the
> same transaction start/end conversion as well.
>
> I'm working on that from time to time but this usually gets pushed down
> in the todo list. It's changing a lot of code, from what I've researched
> so far cannot be done at once and would probably introduce bugs hard to
> hit because of the external conditions (allocator, system load, ...).
>
> I have a plan to do that incrementally, adding assertions and converting
> functions in small batches to be able to catch bugs early, but I'm not
> exactly thrilled to start such endeavour in addition to normal
> development bug hunting.
>
> To get things moving again, I've refreshed the patch adding stubs and
> will try to find the best timing for merg to avoid patch conflicts, but
> no promises.

Thanks!

--
Michal Hocko
SUSE Labs

\
 
 \ /
  Last update: 2021-10-14 13:47    [W:0.059 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site