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

[Christoph Hellwig]

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?

> 
> 							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---