On Wed, Aug 31 2016 at 6:18pm -0400,
Bart Van Assche <bart.vanassche@xxxxxxxxxxx> wrote:
> Ensure that all ongoing dm_mq_queue_rq() and dm_mq_requeue_request()
> calls have stopped before setting the "queue stopped" flag. This
> allows to remove the "queue stopped" test from dm_mq_queue_rq() and
> dm_mq_requeue_request(). Use BLK_MQ_S_STOPPED instead of
> QUEUE_FLAG_STOPPED.
>
> Signed-off-by: Bart Van Assche <bart.vanassche@xxxxxxxxxxx>
At first glance, at a minimum this patch needs a better header. It
seems you're doing 2 things:
1) using blk_mq_{freeze,unfreeze}_queue() actually makes dm_stop_queue()
work for blk-mq? Whereby fixing blk-mq race(s)?
2) switching away from QUEUE_FLAG_STOPPED to BLK_MQ_S_STOPPED (via
blk_mq_queue_stopped)
- not clear to me that dm-mq's use of QUEUE_FLAG_STOPPED wasn't fine;
NVMe also uses it for blk-mq
> ---
> drivers/md/dm-rq.c | 29 +++++++----------------------
> 1 file changed, 7 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
> index 8dc8cfb..b5db523 100644
> --- a/drivers/md/dm-rq.c
> +++ b/drivers/md/dm-rq.c
> @@ -78,7 +78,6 @@ void dm_start_queue(struct request_queue *q)
> if (!q->mq_ops)
> dm_old_start_queue(q);
> else {
> - queue_flag_clear_unlocked(QUEUE_FLAG_STOPPED, q);
> blk_mq_start_stopped_hw_queues(q, true);
> blk_mq_kick_requeue_list(q);
> }
> @@ -98,13 +97,13 @@ void dm_stop_queue(struct request_queue *q)
> {
> if (!q->mq_ops)
> dm_old_stop_queue(q);
> - else {
> - spin_lock_irq(q->queue_lock);
> - queue_flag_set(QUEUE_FLAG_STOPPED, q);
> - spin_unlock_irq(q->queue_lock);
> -
> + else if (!blk_mq_queue_stopped(q)) {
> + /* Wait until dm_mq_queue_rq() has finished. */
> + blk_mq_freeze_queue(q);
> + /* Avoid that requeuing could restart the queue. */
> blk_mq_cancel_requeue_work(q);
> blk_mq_stop_hw_queues(q);
> + blk_mq_unfreeze_queue(q);
> }
> }
>
> @@ -318,13 +317,10 @@ static void dm_old_requeue_request(struct request *rq)
> static void dm_mq_requeue_request(struct request *rq)
> {
> struct request_queue *q = rq->q;
> - unsigned long flags;
>
> blk_mq_requeue_request(rq);
> - spin_lock_irqsave(q->queue_lock, flags);
> - if (!blk_queue_stopped(q))
> - blk_mq_kick_requeue_list(q);
> - spin_unlock_irqrestore(q->queue_lock, flags);
> + WARN_ON_ONCE(blk_mq_queue_stopped(q));
> + blk_mq_kick_requeue_list(q);
> }
>
> static void dm_requeue_original_request(struct mapped_device *md,
> @@ -867,17 +863,6 @@ static int dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
> dm_put_live_table(md, srcu_idx);
> }
>
> - /*
> - * On suspend dm_stop_queue() handles stopping the blk-mq
> - * request_queue BUT: even though the hw_queues are marked
> - * BLK_MQ_S_STOPPED at that point there is still a race that
> - * is allowing block/blk-mq.c to call ->queue_rq against a
> - * hctx that it really shouldn't. The following check guards
> - * against this rarity (albeit _not_ race-free).
> - */
> - if (unlikely(test_bit(BLK_MQ_S_STOPPED, &hctx->state)))
> - return BLK_MQ_RQ_QUEUE_BUSY;
> -
> if (ti->type->busy && ti->type->busy(ti))
> return BLK_MQ_RQ_QUEUE_BUSY;
>
> --
> 2.9.3
>
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
