Re: [PATCH 2/2] mm: introduce PF_MEMALLOC_NOWARN

[Michal Hocko <mhocko@suse.com>]

On Sun 28-01-24 14:43:16, Kent Overstreet wrote:
> On Sun, Jan 28, 2024 at 04:45:32PM +0100, Michal Hocko wrote:
> > On Fri 26-01-24 17:07:56, Kent Overstreet wrote:
> > > If we're using PF_MEMALLOC, we might have a fallback and might not to
> > > warn about a failing allocation - thus we need a PF_* equivalent of
> > > __GFP_NOWARN.
> > 
> > Could you be more specific about the user? Is this an allocation from
> > the reclaim path or an explicit PF_MEMALLOC one? It would be also really
> > helpful to explain why GFP_NOWARN cannot be used directly.
> 
> Explicit PF_MEMALLOC.
> 
> It's for a call to alloc_inode(), which doesn't take gfp flags, and
> plumbing it would require modifying a s_ops callback and would touch
> every filesystem -

OK, I see. This should be part of the changelog.

> but we want to get away from gfp flags anyways :)
> 
> More specifically, the code where I'm using it is doing a "try
> GFP_NOWAIT first; if that fails drop locks and do GFP_KERNEL" dance;
> it's part of a cleanup for some weird lifetime stuff related to
> fs/inode.c.
> 
> #define memalloc_flags_do(_flags, _do)						\
> ({										\
> 	unsigned _saved_flags = memalloc_flags_save(_flags);			\
> 	typeof(_do) _ret = _do;							\
> 	memalloc_noreclaim_restore(_saved_flags);				\
> 	_ret;									\
> })
> 
> /*
>  * Allocate a new inode, dropping/retaking btree locks if necessary:
>  */
> static struct bch_inode_info *bch2_new_inode(struct btree_trans *trans)
> {
> 	struct bch_fs *c = trans->c;
> 
> 	struct bch_inode_info *inode =
> 		memalloc_flags_do(PF_MEMALLOC|PF_MEMALLOC_NOWARN, to_bch_ei(new_inode(c->vfs_sb)));

Is this what you meant by GFP_NOWAIT allocation? You would be right
about this allocation not entering the direct reclaim but do you realize
this is not an ordinary NOWAIT request beause PF_MEMALLOC will allow to
dip into memory reserves without any limits? (Unless the specific
allocation down the road is explicitly GFP_NOMEMALLOC)
A failure here means that the system is really in a desparate state with all
the memory reserves gone which migt break reclaimers who rely on those
reserves.

Are you sure it is a good idea? Unless I am missing something you are
just giving an ordinary user access to those reserves by creating inodes
without any bounds.

-- 
Michal Hocko
SUSE Labs