Re: [PATCH v5 2/2] block: cancel all throttled bios in del_gendisk()

[Paul E. McKenney]

On Fri, Jan 07, 2022 at 11:18:47AM -1000, Tejun Heo wrote:
> Hello,
> 
> On Fri, Jan 07, 2022 at 04:05:19PM +0100, Michal Koutný wrote:
> > On Fri, Dec 10, 2021 at 04:31:43PM +0800, Yu Kuai <yukuai3@xxxxxxxxxx> wrote:
> > > +	 * queue_lock is held, rcu lock is not needed here.
> > > +	 */
> > > +	blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg)
> > > +		tg_drain_bios(&blkg_to_tg(blkg)->service_queue);
> > 
> > FTR, I acknowledge this can work due to RCU peculiarities, however, I
> > don't understand why is it preferred against more robust explict
> > rcu_read_lock().
> > 
> > (All in all, this isn't a deal breaker and I'm not confident evaluating
> > the rest of the patch.)
> 
> Cc'ing Paul for RCU. Paul, this nit is around whether or not to use
> rcu_read_lock() in an irq disabled section. I can see both sides of the
> arguments - it's weird to put in an extra rcu_read_lock() when technically
> unnecessary but it's also nice to have something explicit and structured to
> mark parts which require RCU protection. Putting in a comment is okay but
> consistency is difficult to achieve that way.
> 
> Maybe all these are unnecessary as lockdep would be able to catch them
> anyway, or maybe we'd want something to explicitly mark RCU protected
> sections. I don't know but whichever way, I think we need to establish a
> convention.

The easiest thing to do is to use rcu_dereference_sched() instead of
rcu_dereference().  This will cause lockdep to insist on preemption
(for example, interrupts) being disabled.

Or is this a case where a function containing rcu_dereference() is invoked
with interrupts disabled from some call sites and under rcu_read_lock()
protection from other call sites?  In this case, it is usually best to
include that redundant rcu_read_lock() [1].

							Thanx, Paul

[1]	If you are a glutton for punishment, or if you would otherwise
	have to add a cubic goatskin of rcu_read_lock() calls, you
	could instead write this priceless gem in place of the calls to
	rcu_dereference() in that common function:

	p = rcu_dereference_check(ptr, rcu_read_lock_sched_held());

	This would cause lockdep to be happy with either rcu_read_lock()
	or preemption being disabled.

	This is more precise, and would be preferable in some cases,
	for example, if there were lots of hotpath callsites with
	interrupts disabled.  "Do we add 25 pairs of rcu_read_lock()
	and rcu_read_unlock()?	Or do we add just the one ugly
	rcu_dereference_check()?"