Re: [PATCH v7 5/6] block: Make blk_get_request() block for non-PM requests while suspended

["jianchao.wang" <jianchao.w.wang@xxxxxxxxxx>]

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