On Thu, Aug 29, 2024 at 9:32 PM Kent Overstreet <kent.overstreet@xxxxxxxxx> wrote: > > On Thu, Aug 29, 2024 at 11:12:18PM GMT, Dave Chinner wrote: > > On Thu, Aug 29, 2024 at 06:02:32AM -0400, Kent Overstreet wrote: > > > On Wed, Aug 28, 2024 at 02:09:57PM GMT, Dave Chinner wrote: > > > > On Tue, Aug 27, 2024 at 08:15:43AM +0200, Michal Hocko wrote: > > > > > From: Michal Hocko <mhocko@xxxxxxxx> > > > > > > > > > > bch2_new_inode relies on PF_MEMALLOC_NORECLAIM to try to allocate a new > > > > > inode to achieve GFP_NOWAIT semantic while holding locks. If this > > > > > allocation fails it will drop locks and use GFP_NOFS allocation context. > > > > > > > > > > We would like to drop PF_MEMALLOC_NORECLAIM because it is really > > > > > dangerous to use if the caller doesn't control the full call chain with > > > > > this flag set. E.g. if any of the function down the chain needed > > > > > GFP_NOFAIL request the PF_MEMALLOC_NORECLAIM would override this and > > > > > cause unexpected failure. > > > > > > > > > > While this is not the case in this particular case using the scoped gfp > > > > > semantic is not really needed bacause we can easily pus the allocation > > > > > context down the chain without too much clutter. > > > > > > > > > > Acked-by: Christoph Hellwig <hch@xxxxxx> > > > > > Signed-off-by: Michal Hocko <mhocko@xxxxxxxx> > > > > > > > > Looks good to me. > > > > > > > > Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > > Reposting what I wrote in the other thread: > > > > I've read the thread. I've heard what you have had to say. Like > > several other people, I think your position is just not practical or > > reasonable. > > > > I don't care about the purity or the safety of the API - the > > practical result of PF_MEMALLOC_NORECLAIM is that __GFP_NOFAIL > > allocation can now fail and that will cause unexpected kernel > > crashes. Keeping existing code and API semantics working correctly > > (i.e. regression free) takes precedence over new functionality or > > API features that people want to introduce. > > > > That's all there is to it. This is not a hill you need to die on. > > And more than that, this is coming from you saying "We didn't have to > handle memory allocation failures in IRIX, why can't we be like IRIX? > All those error paths are a pain to test, why can't we get rid of them?" > > Except that's bullshit; at the very least any dynamically sized > allocation _definitely_ has to have an error path that's tested, and if > there's questions about the context a code path might run in, that > that's another reason. > > GFP_NOFAIL is the problem here, and if it's encouraging this brain > damaged "why can't we just get rid of error paths?" thinking, then it > should be removed. > > Error paths have to exist, and they have to be tested. I completely agree. Adding a dead loop in the core of the page allocator just to bypass error handling is a reckless idea. -- Regards Yafang
