Hi Bart
On 09/19/2018 01:44 AM, Bart Van Assche wrote:
>>> + * ago. Since blk_queue_enter() is called by the request allocation
>>> + * code before pm_request_resume(), if no requests have a tag assigned
>>> + * it is safe to suspend the device.
>>> + */
>>> + ret = -EBUSY;
>>> + if (blk_requests_in_flight(q) == 0) {
>>> + /*
>>> + * Call synchronize_rcu() such that later blk_queue_enter()
>>> + * calls see the pm-only state. See also
>>> + *
>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lwn.net_Articles_573497_&d=DwIDAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=o3yai95U5Ge5APoSiMJX64Z8wlI7gf3x0mFnfHpuA4E&s=VPnOWAeo4J4VB944La0uBcynCgVHE-qp52b_9pV8NH4&e=.
>>> + */
>>> + synchronize_rcu();
>>> + if (blk_requests_in_flight(q) == 0)
>>
>> Seems not safe here.
>>
>> For blk-mq path:
>> Someone may have escaped from the preempt checking, missed the
>> blk_pm_request_resume there,
>> entered into generic_make_request, but have not allocated request and
>> occupied any tag.
>>
>> There could be a similar scenario for blk-legacy path, the q->nr_pending is
>> increased when
>> request is queued.
>>
>> So I guess the q_usage_counter checking is still needed here.
>
> There is only one blk_pm_request_resume() call and that call is inside
> blk_queue_enter() after the pm_only counter has been checked.
>
> For the legacy block layer, nr_pending is increased after the
> blk_queue_enter() call from inside blk_old_get_request() succeeded.
>
> So I don't see how blk_pm_request_resume() or q->nr_pending++ could escape
> from the preempt checking?
For example:
blk_pre_runtime_suspend generic_make_request
blk_queue_enter // success here,
no blk_pm_request_resume here
blk_mq_make_requset
blk_set_pm_only
if (blk_requests_in_flight(q) == 0) {
synchronize_rcu();
if (blk_requests_in_flight(q) == 0)
ret = 0;
}
blk_mq_get_request
Thanks
Jianchao
