Hi.
> >
> > Why is it ok to remove the "wait for congestion and retry" logic here?
> > Does GFP_NOFAIL do all that for us?
>
> Yes - kmalloc will never return failure if GFP_NOFAIL is set, and
> it has internal congestion_wait() backoffs. All we lose here is
> the XFS specific error message - we'll get the generic warnings
> now...
>
> > Same question for the other two patches that remove similar loops from
> > other functions.
> >
> > > - } while (1);
> > > -}
> > > -
> > > -
> > > -/*
> > > - * __vmalloc() will allocate data pages and auxiliary structures (e.g.
> > > - * pagetables) with GFP_KERNEL, yet we may be under GFP_NOFS context here. Hence
> > > - * we need to tell memory reclaim that we are in such a context via
> > > - * PF_MEMALLOC_NOFS to prevent memory reclaim re-entering the filesystem here
> > > - * and potentially deadlocking.
> > > - */
> > > -static void *
> > > -__kmem_vmalloc(size_t size, xfs_km_flags_t flags)
> > > -{
> > > - unsigned nofs_flag = 0;
> > > - void *ptr;
> > > - gfp_t lflags = kmem_flags_convert(flags);
> > > -
> > > - if (flags & KM_NOFS)
> > > - nofs_flag = memalloc_nofs_save();
> > > -
> > > - ptr = __vmalloc(size, lflags, PAGE_KERNEL);
> > > -
> > > - if (flags & KM_NOFS)
> > > - memalloc_nofs_restore(nofs_flag);
> > > -
> > > - return ptr;
> > > -}
> >
> > I think this deletes too much -- as the comment says, a caller can pass
> > in GFP_NOFS and we have to push that context all the way through to
> > /any/ allocation that happens under __vmalloc.
> >
> > However, it's possible that the mm developers have fixed __vmalloc to
> > pass GFP_NOFS through to every allocation that can be made. Is that the
> > case?
>
> Nope, vmalloc will still do GFP_KERNEL allocations internally. We
> need to keep the kmem_vmalloc wrappers as they stand, just converted
> to use GFP flags.
>
Ok, I'll leave it as-is (with converted flags).
> > TBH this series would be better sequenced as (1) get rid of pointless
> > wrappers, (2) convert kmem* callers to use GFP flags, and (3) save
> > whatever logic changes for the end. If nothing else I could pull the
> > first two parts and leave the third for later.
>
I need to fix the line >80 characters and some other nit picks on the patches,
so I can split this into this 3-series guideline and submit everything again, no
worries.
I do believe though the kmem_zone_alloc() removal falls into the 1st series here
right? Giving the fact it removes the loop but doesn't really alter the logic at
all, since __GFP_NOFAIL now implies the 'infinite loop' and the
congestion_wait()?
>
> > > static inline void *
> > > -kmem_zalloc_large(size_t size, xfs_km_flags_t flags)
> > > +kmem_zalloc_large(size_t size, gfp_t flags)
> > > {
> > > - return kmem_alloc_large(size, flags | KM_ZERO);
> > > + return kmem_alloc_large(size, flags | __GFP_ZERO);
> > > }
>
> I'd also kill these zalloc wrappers and just pass __GFP_ZERO
> directly.
Thanks for the review guys.
Cheers
--
Carlos
