Hi Bart, On Tue, 2020-08-25 at 19:58 -0700, Bart Van Assche wrote: > On 2020-08-25 02:11, Stanley Chu wrote: > >> diff --git a/block/blk-pm.c b/block/blk-pm.c > >> index b85234d758f7..17bd020268d4 100644 > >> --- a/block/blk-pm.c > >> +++ b/block/blk-pm.c > >> @@ -67,6 +67,10 @@ int blk_pre_runtime_suspend(struct request_queue *q) > >> > >> WARN_ON_ONCE(q->rpm_status != RPM_ACTIVE); > >> > >> + spin_lock_irq(&q->queue_lock); > >> + q->rpm_status = RPM_SUSPENDING; > >> + spin_unlock_irq(&q->queue_lock); > >> + > > > > Has below alternative way been considered that RPM_SUSPENDING is set > > after blk_freeze_queue_start()? > > > > blk_freeze_queue_start(q); > > > > + spin_lock_irq(&q->queue_lock); > > + q->rpm_status = RPM_SUSPENDING; > > + spin_unlock_irq(&q->queue_lock); > > > > > > Otherwise requests can enter queue while rpm_status is RPM_SUSPENDING > > during a small window, i.e., before blk_set_pm_only() is invoked. This > > would make the definition of rpm_status ambiguous. > > > > In this way, the racing could be also solved: > > > > - Before blk_freeze_queue_start(), any requests shall be allowed to > > enter queue > > - blk_freeze_queue_start() freezes the queue and blocks all upcoming > > requests (make them wait_event(q->mq_freeze_wq)) > > - rpm_status is set as RPM_SUSPENDING > > - blk_mq_unfreeze_queue() wakes up q->mq_freeze_wq and then > > blk_pm_request_resume() can be executed > > Hi Stanley, > > I prefer the order from the patch. I think it is important to change > q->rpm_status into RPM_SUSPENDING before blk_queue_enter() calls > blk_queue_pm_only(). Otherwise it could happen that blk_queue_enter() > calls blk_pm_request_resume() while q->rpm_status == RPM_ACTIVE, resulting > in blk_queue_enter() not resuming a queue although that queue should be > resumed. I see. Thanks for the explanation. By the order from the patch, it is guaranteed that blk_pm_request_resume() will always trigger runtime resume flow during blk_queue_enter() after blk_set_pm_only() is called by concurrent suspend flow, i.e., blk_pre_runtime_suspend(). However it would have an ambiguous RPM_SUSPENDING rpm_status even though it may not cause issues obviously. Acked-By: Stanley Chu <stanley.chu@xxxxxxxxxxxx>
