[PATCH] nvme: fix flush dependency in delete controller flow

[Paul E. McKenney]

On Fri, Apr 06, 2018 at 08:19:20AM +0200, Christoph Hellwig wrote:
> On Thu, Apr 05, 2018 at 05:18:11PM -0700, Paul E. McKenney wrote:
> > OK, how does the following (untested) patch look?
> 
> Looks sensible to me.  Nitzan, can you test it with the obvious nvme
> conversion to cleanup_srcu_struct_quiesced?

And it did pass light rcutorture testing overnight, so here is hoping!  ;-)

							Thanx, Paul

> > ------------------------------------------------------------------------
> > 
> > diff --git a/include/linux/srcu.h b/include/linux/srcu.h
> > index 33c1c698df09..91494d7e8e41 100644
> > --- a/include/linux/srcu.h
> > +++ b/include/linux/srcu.h
> > @@ -69,11 +69,45 @@ struct srcu_struct { };
> >  
> >  void call_srcu(struct srcu_struct *sp, struct rcu_head *head,
> >  		void (*func)(struct rcu_head *head));
> > -void cleanup_srcu_struct(struct srcu_struct *sp);
> > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced);
> >  int __srcu_read_lock(struct srcu_struct *sp) __acquires(sp);
> >  void __srcu_read_unlock(struct srcu_struct *sp, int idx) __releases(sp);
> >  void synchronize_srcu(struct srcu_struct *sp);
> >  
> > +/**
> > + * cleanup_srcu_struct - deconstruct a sleep-RCU structure
> > + * @sp: structure to clean up.
> > + *
> > + * Must invoke this after you are finished using a given srcu_struct that
> > + * was initialized via init_srcu_struct(), else you leak memory.
> > + */
> > +static inline void cleanup_srcu_struct(struct srcu_struct *sp)
> > +{
> > +	_cleanup_srcu_struct(sp, false);
> > +}
> > +
> > +/**
> > + * cleanup_srcu_struct_quiesced - deconstruct a quiesced sleep-RCU structure
> > + * @sp: structure to clean up.
> > + *
> > + * Must invoke this after you are finished using a given srcu_struct that
> > + * was initialized via init_srcu_struct(), else you leak memory.  Also,
> > + * all grace-period processing must have completed.
> > + *
> > + * "Completed" means that the last synchronize_srcu() and
> > + * synchronize_srcu_expedited() calls must have returned before the call
> > + * to cleanup_srcu_struct_quiesced().  It also means that the callback
> > + * from the last call_srcu() must have been invoked before the call to
> > + * cleanup_srcu_struct_quiesced(), but you can use srcu_barrier() to help
> > + * with this last.  Violating these rules will get you a WARN_ON() splat
> > + * (with high probability, anyway), and will also cause the srcu_struct
> > + * to be leaked.
> > + */
> > +static inline void cleanup_srcu_struct_quiesced(struct srcu_struct *sp)
> > +{
> > +	_cleanup_srcu_struct(sp, true);
> > +}
> > +
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >  
> >  /**
> > diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> > index 680c96d8c00f..f0e1d44459f8 100644
> > --- a/kernel/rcu/rcutorture.c
> > +++ b/kernel/rcu/rcutorture.c
> > @@ -593,7 +593,12 @@ static void srcu_torture_init(void)
> >  
> >  static void srcu_torture_cleanup(void)
> >  {
> > -	cleanup_srcu_struct(&srcu_ctld);
> > +	static DEFINE_TORTURE_RANDOM(rand);
> > +
> > +	if (torture_random(&rand) & 0x800)
> > +		cleanup_srcu_struct(&srcu_ctld);
> > +	else
> > +		cleanup_srcu_struct_quiesced(&srcu_ctld);
> >  	srcu_ctlp = &srcu_ctl; /* In case of a later rcutorture run. */
> >  }
> >  
> > diff --git a/kernel/rcu/srcutiny.c b/kernel/rcu/srcutiny.c
> > index 76ac5f50b2c7..622792abe41a 100644
> > --- a/kernel/rcu/srcutiny.c
> > +++ b/kernel/rcu/srcutiny.c
> > @@ -86,16 +86,19 @@ EXPORT_SYMBOL_GPL(init_srcu_struct);
> >   * Must invoke this after you are finished using a given srcu_struct that
> >   * was initialized via init_srcu_struct(), else you leak memory.
> >   */
> > -void cleanup_srcu_struct(struct srcu_struct *sp)
> > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced)
> >  {
> >  	WARN_ON(sp->srcu_lock_nesting[0] || sp->srcu_lock_nesting[1]);
> > -	flush_work(&sp->srcu_work);
> > +	if (quiesced)
> > +		WARN_ON(work_pending(&sp->srcu_work));
> > +	else
> > +		flush_work(&sp->srcu_work);
> >  	WARN_ON(sp->srcu_gp_running);
> >  	WARN_ON(sp->srcu_gp_waiting);
> >  	WARN_ON(sp->srcu_cb_head);
> >  	WARN_ON(&sp->srcu_cb_head != sp->srcu_cb_tail);
> >  }
> > -EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> > +EXPORT_SYMBOL_GPL(_cleanup_srcu_struct);
> >  
> >  /*
> >   * Removes the count for the old reader from the appropriate element of
> > diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> > index fb560fca9ef4..b4123d7a2cec 100644
> > --- a/kernel/rcu/srcutree.c
> > +++ b/kernel/rcu/srcutree.c
> > @@ -366,24 +366,28 @@ static unsigned long srcu_get_delay(struct srcu_struct *sp)
> >  	return SRCU_INTERVAL;
> >  }
> >  
> > -/**
> > - * cleanup_srcu_struct - deconstruct a sleep-RCU structure
> > - * @sp: structure to clean up.
> > - *
> > - * Must invoke this after you are finished using a given srcu_struct that
> > - * was initialized via init_srcu_struct(), else you leak memory.
> > - */
> > -void cleanup_srcu_struct(struct srcu_struct *sp)
> > +/* Helper for cleanup_srcu_struct() and cleanup_srcu_struct_quiesced(). */
> > +void _cleanup_srcu_struct(struct srcu_struct *sp, bool quiesced)
> >  {
> >  	int cpu;
> >  
> >  	if (WARN_ON(!srcu_get_delay(sp)))
> > -		return; /* Leakage unless caller handles error. */
> > +		return; /* Just leak it! */
> >  	if (WARN_ON(srcu_readers_active(sp)))
> > -		return; /* Leakage unless caller handles error. */
> > -	flush_delayed_work(&sp->work);
> > +		return; /* Just leak it! */
> > +	if (quiesced) {
> > +		if (WARN_ON(delayed_work_pending(&sp->work)))
> > +			return; /* Just leak it! */
> > +	} else {
> > +		flush_delayed_work(&sp->work);
> > +	}
> >  	for_each_possible_cpu(cpu)
> > -		flush_delayed_work(&per_cpu_ptr(sp->sda, cpu)->work);
> > +		if (quiesced) {
> > +			if (WARN_ON(delayed_work_pending(&per_cpu_ptr(sp->sda, cpu)->work)))
> > +				return; /* Just leak it! */
> > +		} else {
> > +			flush_delayed_work(&per_cpu_ptr(sp->sda, cpu)->work);
> > +		}
> >  	if (WARN_ON(rcu_seq_state(READ_ONCE(sp->srcu_gp_seq)) != SRCU_STATE_IDLE) ||
> >  	    WARN_ON(srcu_readers_active(sp))) {
> >  		pr_info("%s: Active srcu_struct %p state: %d\n", __func__, sp, rcu_seq_state(READ_ONCE(sp->srcu_gp_seq)));
> > @@ -392,7 +396,7 @@ void cleanup_srcu_struct(struct srcu_struct *sp)
> >  	free_percpu(sp->sda);
> >  	sp->sda = NULL;
> >  }
> > -EXPORT_SYMBOL_GPL(cleanup_srcu_struct);
> > +EXPORT_SYMBOL_GPL(_cleanup_srcu_struct);
> >  
> >  /*
> >   * Counts the new reader in the appropriate per-CPU element of the
> ---end quoted text---
>