Re: [PATCH v3 3/3] iova: defer maple tree erase on GFP_ATOMIC failure

[Liam R. Howlett]

On 26/06/19 09:08AM, Jason Gunthorpe wrote:
> On Thu, Jun 18, 2026 at 01:27:02PM -0400, Liam R. Howlett wrote:
> > On 26/06/18 12:24PM, Jason Gunthorpe wrote:
> > > On Thu, Jun 18, 2026 at 10:50:56AM -0400, Liam R. Howlett wrote:
> > > 
> > > > > If that's the case it should be documented like this too :)
> > > > 
> > > > Yeah, very much should add some notes here... but also maybe stop them
> > > > from doing it by adding this to mas_erase():
> > > > 
> > > > if (mt_external_lock(mas->tree))
> > > >         might_alloc(GFP_KERNEL);
> > > 
> > > Yeah, and then I'd add a might_alloc() to mtree_erase() as well, it
> > > can always sleep..
> > 
> > mtree_erase() calls mas_erase(), so I think the one should be enough.
> 
> I was thinking an unconditional one before locking:
> 
> @@ -5919,6 +5919,8 @@ void *mtree_erase(struct maple_tree *mt, unsigned long index)
>         MA_STATE(mas, mt, index, index);
>         trace_ma_op(TP_FCT, &mas);
>  
> +       might_alloc(GFP_KERNEL);
> +

Right, yes.  I'll do that.

>         mtree_lock(mt);
> 
> Since with internal locking this can still sleep, and it has to be
> called in a sleepable context.
> 
> The one in mas_erase() is conditional on external locking and is
> basically asserting the external lock is non-atomic.
> 
> > > Okay, so to summarize:
> > > 
> > > - mas_erase, mtree_erase cannot fail with ENOMEM. The check for ENOMEM
> > >   inside should be changed to a WARN_ON to document this.
> > >   Rational: in a GFP_KERNEL context it will sleep forever until it
> > >   gets memory "too small to fail"
> > 
> > The ENOMEM check inside mas_nomem() is still possible because we try
> > NOWAIT first, so it is possible that we hit a retry.
> > 
> > The return from mas_nomem() without allocations should be impossible.
> > So if (!mas->sheaf && !mas->alloc) should be a WARN_ON_ONCE().
> > 
> > I believe this is what you meant anyways?
> 
> Yeah, probably, or maybe this:
> 
> 	if (mas_is_err(mas))
> 		goto out;
> 
> Can it ever be anything but -ENOMEM within mas_erase()?
>  
> > So cleaning them up should be handled sooner rather than later.  I'm
> > trying to figure out the best place to do this as I do not like the
> > retry with the timer idea.
> 
> I would clean them up when gap searching to allocate a new thing. That
> is a safe place to fail and ensures the gaps are correct before
> checking them.
> 
> Jason
> 
> -- 
> maple-tree mailing list
> maple-tree@xxxxxxxxxxxxxxxxxxx
> https://lists.infradead.org/mailman/listinfo/maple-tree