Re: [PATCHv6 07/16] pps: move idr stuff to pps.c

[Andrew Morton <akpm@linux-foundation.org>]

On Sat, 18 Dec 2010 04:07:38 +0300
Alexander Gordeev <lasaine@lvk.cs.msu.su> wrote:

> > > +	if (err < 0)
> > > +		return err;
> > 
> > The IDR interface really sucks :(
> > 
> > What this code should be doing is
> > 
> > retry:
> > 	if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0)
> > 		return -ENOMEM;
> > 	spin_lock_irq(&pps_idr_lock);
> > 	err = idr_get_new(&pps_idr, pps, &pps->id);
> > 	spin_unlock_irq(&pps_idr_lock);
> > 	if (err < 0) {
> > 		if (err == -EAGAIN)
> > 			goto retry;
> > 		return err;
> > 	}
> > 
> > this way it correctly handles the case where the idr_pre_get()
> > succeeded in precharging the pool, but some other task cam in and stole
> > your reservation.
> 
> Yeah, I see. Maybe switching from spin lock to mutex and protecting the
> whole thing with it can do? Like this:
> 
> ...
> mutex_lock(&pps_idr_lock);
> if (idr_pre_get(&pps_idr, GFP_KERNEL) == 0) {
> 	mutex_unlock(&pps_idr_lock);
> 	return -ENOMEM;
> }
> err = idr_get_new(&pps_idr, pps, &pps->id);
> mutex_unlock(&pps_idr_lock);
> 
> if (err < 0)
> 	return err;
> ...

That works so, as long as no code path will take pps_idr_lock in the
page allocator direct-reclaim path.

That's unlikely to be happening in the PPS driver of course.  It's
conceivable that some filesystems might want to read the time when
playing with file timestamps in the direct-reclaim path, but I assume
pps_idr_lock wouldn't be taken on any read-system-time paths.