Re: [PATCH] bcachefs: Switch to memalloc_flags_do() for vmalloc allocations

[Kent Overstreet <kent.overstreet@linux.dev>]

On Thu, Aug 29, 2024 at 02:34:08PM GMT, Michal Hocko wrote:
> On Thu 29-08-24 07:55:08, Kent Overstreet wrote:
> > On Thu, Aug 29, 2024 at 01:08:53PM GMT, Michal Hocko wrote:
> > > On Wed 28-08-24 18:58:43, Kent Overstreet wrote:
> > > > On Wed, Aug 28, 2024 at 09:26:44PM GMT, Michal Hocko wrote:
> > > > > On Wed 28-08-24 15:11:19, Kent Overstreet wrote:
> > > [...]
> > > > > > It was decided _years_ ago that PF_MEMALLOC flags were how this was
> > > > > > going to be addressed.
> > > > > 
> > > > > Nope! It has been decided that _some_ gfp flags are acceptable to be used
> > > > > by scoped APIs. Most notably NOFS and NOIO are compatible with reclaim
> > > > > modifiers and other flags so these are indeed safe to be used that way.
> > > > 
> > > > Decided by who?
> > > 
> > > Decides semantic of respective GFP flags and their compatibility with
> > > others that could be nested in the scope.
> > 
> > Well, that's a bit of commentary, at least.
> > 
> > The question is which of those could properly apply to a section, not a
> > callsite, and a PF_MEMALLOC_NOWAIT (similar to but not exactly the same
> > as PF_MEMALLOC_NORECLAIM) would be at the top of that list since we
> > already have a clear concept of sections where we're not allowed to
> > sleep.
> 
> Unfortunately a lack of __GFP_DIRECT_RECLAIM means both no reclaim and
> no sleeping allowed for historical reasons. GFP_NOWAIT is both used from
> atomic contexts and as an optimistic allocation attempt with a heavier
> fallback allocation strategy. If you want NORECLAIM semantic then this
> would need to be represented by different means than __GFP_DIRECT_RECLAIM
> alone.

I don't see it as particularly needed - the vmalloc locks you mentioned
previously just mean it's something worth considering. In my usage I
probably wouldn't care about those locks, but for keeping the API simple
we probably want just PF_MEMALLOC_NOWAIT (where those locks become
trylock).

> > And that tells us how to resolve GFP_NOFAIL with other conflicting
> > PF_MEMALLOC flags: GFP_NOFAIL loses.
> > 
> > It is a _bug_ if GFP_NOFAIL is accidentally used in a non sleepable
> > context, and properly labelling those sections to the allocator would
> > allow us to turn undefined behaviour into an error - _that_ would be
> > turning kmalloc() into a safe interface.
> 
> If your definition of safe includes an oops or worse silent failure
> then yes. Not really safe interface in my book though. E.g. (just
> randomly looking at GFP_NOFAIL users) btree_paths_realloc doesn't check
> the return value and if it happened to be called from such a scope it
> would have blown up. That code is safe without the scope though. There
> are many other callsites which do not have failure paths.

Yes, but that's unsafe anyways due to the max allocation size of
GFP_NOFAIL - I'll have to fix that.

Note that even if we got rid of the smaller max allocation size of
GFP_NOFAIL allocations we'd _still_ generally need error paths due to
the hard limit of INT_MAX, and integer overflow checking for array
allocations.