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
