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
