On 07/27/2018 11:29 PM, Bart Van Assche wrote: > On Fri, 2018-07-27 at 09:57 +0800, jianchao.wang wrote: >> 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