On 07/27/2018 11:29 PM, Bart Van Assche wrote:

On Fri, 2018-07-27 at 09:57 +0800, jianchao.wang wrote:

quoted

Something like:

	percpu_ref_switch_to_atomic_sync(&q->q_usage_counter);
	if (!percpu_ref_is_zero(&q->q_usage_counter)) {
		ret = -EBUSY;
		pm_runtime_mark_last_busy(q->dev);
     	} else {
		blk_set_preempt_only(q);
		if (!percpu_ref_is_zero(&q->q_usage_counter) {
			ret = -EBUSY;
			pm_runtime_mark_last_busy(q->dev);
			blk_clear_preempt_only(q);
		} else {
			q->rpm_status = RPM_SUSPENDIN;    
		}
        }

I think this code is racy. Because there is no memory barrier in
blk_queue_enter() between the percpu_ref_tryget_live() and the
blk_queue_preempt_only() calls, the context that sets the PREEMPT_ONLY flag
has to use synchronize_rcu() or call_rcu() to ensure that blk_queue_enter()
sees the PREEMPT_ONLY flag after it has called percpu_ref_tryget_live().
See also https://urldefense.proofpoint.com/v2/url?u=http-3A__lwn.net_Articles_573497_&d=DwIFgQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=mkAOXQtxuVTAej2tBjOvEed2h4TRvX3mE-EPeNEUD5E&s=xTgTB2x1JwvRUQVyOL8m3rhbbk8xhOkZMC_Io9bmpFc&e=.

Yes, a synchrorize_rcu is indeed needed here to ensure a q_usage_counter is
got successfully with increasing one,  or unsuccessfully without increasing one.

Thanks
Jianchao