[PATCH 0/6] blk-mq: cleanup start/stop queue

[PATCH 0/6] blk-mq: cleanup start/stop queue

[Ming Lei]

Hi,

We have replaced most of start/stop queue via quiesce/unquiesce.
And only the usage in case of handling BLK_STS_RESOURCE is kept.
This patch moves this handling into blk-mq for xen-blkfront and
virtio-blk, then we can avoid to let drivers touch the 'stopped'
state, because allowing driver to do that has caused lots of trouble
for us.

Thanks,
Ming

Ming Lei (6):
  xen-blkfront: quiesce/unquiesce queue instead of start/stop queues
  SCSI: use blk_mq_run_hw_queues() in scsi_kick_queue()
  block: don't call blk_mq_delay_run_hw_queue() in case of
    BLK_STS_RESOURCE
  blk-mq: introduce auto restart
  block: use BLK_MQ_F_AUTO_RESTART on virtio-blk and xen-blkfront
  blk-mq: unexport APIs for start/stop queues

 block/blk-mq.c               | 124 +++++++++++++++----------------------------
 block/blk-mq.h               |   1 +
 drivers/block/virtio_blk.c   |   8 +--
 drivers/block/xen-blkfront.c |  31 +++--------
 drivers/md/dm-rq.c           |   1 -
 drivers/nvme/host/fc.c       |   3 --
 drivers/scsi/scsi_lib.c      |   6 +--
 include/linux/blk-mq.h       |   9 +---
 8 files changed, 54 insertions(+), 129 deletions(-)

-- 
2.9.4

[PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

stopping queue may cause race and may not stop the queue really
after the API returns, and we have improved quiescing
interface and it really can block dispatching once it returns.

So switch to quiesce/unquiece like what we did on other drivers
(NVMe, NBD, mtip32xx, ...)

The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
used in blkif_queue_rq() and blkif_interrupt() are for congestion
control, we leave it as it is since it is safe for this usage.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/xen-blkfront.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c852ed3c01d5..1578befda635 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1187,7 +1187,7 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 		return;
 
 	/* No more blkif_request(). */
-	blk_mq_stop_hw_queues(info->rq);
+	blk_mq_quiesce_queue(info->rq);
 
 	for (i = 0; i < info->nr_rings; i++) {
 		struct blkfront_ring_info *rinfo = &info->rinfo[i];
@@ -1216,8 +1216,10 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
 {
-	if (!RING_FULL(&rinfo->ring))
+	if (!RING_FULL(&rinfo->ring)) {
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
+		blk_mq_kick_requeue_list(rinfo->dev_info->rq);
+	}
 }
 
 static void kick_pending_request_queues(struct blkfront_ring_info *rinfo)
@@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo)
 	unsigned long flags;
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
-	kick_pending_request_queues_locked(rinfo);
+	if (!RING_FULL(&rinfo->ring))
+		blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
 	spin_unlock_irqrestore(&rinfo->ring_lock, flags);
 }
 
@@ -1346,7 +1349,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;
 	/* No more blkif_request(). */
 	if (info->rq)
-		blk_mq_stop_hw_queues(info->rq);
+		blk_mq_quiesce_queue(info->rq);
 
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
@@ -2018,22 +2021,13 @@ static int blkif_recover(struct blkfront_info *info)
 	/* Now safe for us to use the shared ring */
 	info->connected = BLKIF_STATE_CONNECTED;
 
-	for (r_index = 0; r_index < info->nr_rings; r_index++) {
-		struct blkfront_ring_info *rinfo;
-
-		rinfo = &info->rinfo[r_index];
-		/* Kick any other new requests queued since we resumed */
-		kick_pending_request_queues(rinfo);
-	}
-
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);
 		BUG_ON(req->nr_phys_segments > segs);
 		blk_mq_requeue_request(req, false);
 	}
-	blk_mq_start_stopped_hw_queues(info->rq, true);
-	blk_mq_kick_requeue_list(info->rq);
+	blk_mq_unquiesce_queue(info->rq);
 
 	while ((bio = bio_list_pop(&info->bio_list)) != NULL) {
 		/* Traverse the list of pending bios and re-queue them */
-- 
2.9.4

[PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

stopping queue may cause race and may not stop the queue really
after the API returns, and we have improved quiescing
interface and it really can block dispatching once it returns.

So switch to quiesce/unquiece like what we did on other drivers
(NVMe, NBD, mtip32xx, ...)

The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
used in blkif_queue_rq() and blkif_interrupt() are for congestion
control, we leave it as it is since it is safe for this usage.

Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: "Roger Pau Monné" <roger.pau@citrix.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: xen-devel@lists.xenproject.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/xen-blkfront.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index c852ed3c01d5..1578befda635 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -1187,7 +1187,7 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 		return;
 
 	/* No more blkif_request(). */
-	blk_mq_stop_hw_queues(info->rq);
+	blk_mq_quiesce_queue(info->rq);
 
 	for (i = 0; i < info->nr_rings; i++) {
 		struct blkfront_ring_info *rinfo = &info->rinfo[i];
@@ -1216,8 +1216,10 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 /* Already hold rinfo->ring_lock. */
 static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
 {
-	if (!RING_FULL(&rinfo->ring))
+	if (!RING_FULL(&rinfo->ring)) {
 		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
+		blk_mq_kick_requeue_list(rinfo->dev_info->rq);
+	}
 }
 
 static void kick_pending_request_queues(struct blkfront_ring_info *rinfo)
@@ -1225,7 +1227,8 @@ static void kick_pending_request_queues(struct blkfront_ring_info *rinfo)
 	unsigned long flags;
 
 	spin_lock_irqsave(&rinfo->ring_lock, flags);
-	kick_pending_request_queues_locked(rinfo);
+	if (!RING_FULL(&rinfo->ring))
+		blk_mq_run_hw_queues(rinfo->dev_info->rq, true);
 	spin_unlock_irqrestore(&rinfo->ring_lock, flags);
 }
 
@@ -1346,7 +1349,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
 		BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;
 	/* No more blkif_request(). */
 	if (info->rq)
-		blk_mq_stop_hw_queues(info->rq);
+		blk_mq_quiesce_queue(info->rq);
 
 	for (i = 0; i < info->nr_rings; i++)
 		blkif_free_ring(&info->rinfo[i]);
@@ -2018,22 +2021,13 @@ static int blkif_recover(struct blkfront_info *info)
 	/* Now safe for us to use the shared ring */
 	info->connected = BLKIF_STATE_CONNECTED;
 
-	for (r_index = 0; r_index < info->nr_rings; r_index++) {
-		struct blkfront_ring_info *rinfo;
-
-		rinfo = &info->rinfo[r_index];
-		/* Kick any other new requests queued since we resumed */
-		kick_pending_request_queues(rinfo);
-	}
-
 	list_for_each_entry_safe(req, n, &info->requests, queuelist) {
 		/* Requeue pending requests (flush or discard) */
 		list_del_init(&req->queuelist);
 		BUG_ON(req->nr_phys_segments > segs);
 		blk_mq_requeue_request(req, false);
 	}
-	blk_mq_start_stopped_hw_queues(info->rq, true);
-	blk_mq_kick_requeue_list(info->rq);
+	blk_mq_unquiesce_queue(info->rq);
 
 	while ((bio = bio_list_pop(&info->bio_list)) != NULL) {
 		/* Traverse the list of pending bios and re-queue them */
-- 
2.9.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/6] SCSI: use blk_mq_run_hw_queues() in scsi_kick_queue()

[Ming Lei]

Now SCSI won't stop queue at all, and not necessary to use
blk_mq_start_hw_queues() to clear the state of 'stopped',
so switch to blk_mq_run_hw_queues() instead.

Cc: "James E.J. Bottomley" <jejb@linux.vnet.ibm.com>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/scsi/scsi_lib.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f6097b89d5d3..91d890356b78 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -333,7 +333,7 @@ void scsi_device_unbusy(struct scsi_device *sdev)
 static void scsi_kick_queue(struct request_queue *q)
 {
 	if (q->mq_ops)
-		blk_mq_start_hw_queues(q);
+		blk_mq_run_hw_queues(q, false);
 	else
 		blk_run_queue(q);
 }
-- 
2.9.4

[PATCH 3/6] block: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE

[Ming Lei]

If .queue_rq() returns BLK_STS_RESOURCE, blk-mq will rerun
the queue in the three situations:

1) if BLK_MQ_S_SCHED_RESTART is set
- queue is rerun after one rq is completed, see blk_mq_sched_restart()
which is run from blk_mq_free_request()

2) BLK_MQ_S_TAG_WAITING is set
- queue is rerun after one tag is freed

3) otherwise
- queue is run immediately in blk_mq_dispatch_rq_list()

So calling blk_mq_delay_run_hw_queue() inside .queue_rq() doesn't make
sense because no matter it is called or not, the queue still will be
rerun soon in above three situations, and the busy req can be dispatched
again.

Also delay a while radomly is also like a workaround.

Cc: Alasdair Kergon <agk@redhat.com>
Cc: Mike Snitzer <snitzer@redhat.com>
Cc: dm-devel@redhat.com
Cc: Sagi Grimberg <sagi@grimberg.me>
Cc: linux-nvme@lists.infradead.org
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi@vger.kernel.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/md/dm-rq.c      | 1 -
 drivers/nvme/host/fc.c  | 3 ---
 drivers/scsi/scsi_lib.c | 4 ----
 3 files changed, 8 deletions(-)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index c6ebc5b1e00e..71422cea1c4a 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -761,7 +761,6 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 		/* Undo dm_start_request() before requeuing */
 		rq_end_stats(md, rq);
 		rq_completed(md, rq_data_dir(rq), false);
-		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
 		return BLK_STS_RESOURCE;
 	}
 
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d666ada39a9b..3155bbcd26a2 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1967,9 +1967,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 		if (ret != -EBUSY)
 			return BLK_STS_IOERR;
 
-		if (op->rq)
-			blk_mq_delay_run_hw_queue(queue->hctx, NVMEFC_QUEUE_DELAY);
-
 		return BLK_STS_RESOURCE;
 	}
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 91d890356b78..03a7d7953df2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1974,11 +1974,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 out:
 	switch (ret) {
 	case BLK_STS_OK:
-		break;
 	case BLK_STS_RESOURCE:
-		if (atomic_read(&sdev->device_busy) == 0 &&
-		    !scsi_device_blocked(sdev))
-			blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
 		break;
 	default:
 		/*
-- 
2.9.4

[PATCH 3/6] block: don't call blk_mq_delay_run_hw_queue() in case of BLK_STS_RESOURCE

[Ming Lei]

If .queue_rq() returns BLK_STS_RESOURCE, blk-mq will rerun
the queue in the three situations:

1) if BLK_MQ_S_SCHED_RESTART is set
- queue is rerun after one rq is completed, see blk_mq_sched_restart()
which is run from blk_mq_free_request()

2) BLK_MQ_S_TAG_WAITING is set
- queue is rerun after one tag is freed

3) otherwise
- queue is run immediately in blk_mq_dispatch_rq_list()

So calling blk_mq_delay_run_hw_queue() inside .queue_rq() doesn't make
sense because no matter it is called or not, the queue still will be
rerun soon in above three situations, and the busy req can be dispatched
again.

Also delay a while radomly is also like a workaround.

Cc: Alasdair Kergon <agk at redhat.com>
Cc: Mike Snitzer <snitzer at redhat.com>
Cc: dm-devel at redhat.com
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: linux-nvme at lists.infradead.org
Cc: "James E.J. Bottomley"
Cc: "Martin K. Petersen"
Cc: linux-scsi at vger.kernel.org
Signed-off-by: Ming Lei <ming.lei at redhat.com>
---
 drivers/md/dm-rq.c      | 1 -
 drivers/nvme/host/fc.c  | 3 ---
 drivers/scsi/scsi_lib.c | 4 ----
 3 files changed, 8 deletions(-)

diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
index c6ebc5b1e00e..71422cea1c4a 100644
--- a/drivers/md/dm-rq.c
+++ b/drivers/md/dm-rq.c
@@ -761,7 +761,6 @@ static blk_status_t dm_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
 		/* Undo dm_start_request() before requeuing */
 		rq_end_stats(md, rq);
 		rq_completed(md, rq_data_dir(rq), false);
-		blk_mq_delay_run_hw_queue(hctx, 100/*ms*/);
 		return BLK_STS_RESOURCE;
 	}
 
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d666ada39a9b..3155bbcd26a2 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1967,9 +1967,6 @@ nvme_fc_start_fcp_op(struct nvme_fc_ctrl *ctrl, struct nvme_fc_queue *queue,
 		if (ret != -EBUSY)
 			return BLK_STS_IOERR;
 
-		if (op->rq)
-			blk_mq_delay_run_hw_queue(queue->hctx, NVMEFC_QUEUE_DELAY);
-
 		return BLK_STS_RESOURCE;
 	}
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 91d890356b78..03a7d7953df2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1974,11 +1974,7 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 out:
 	switch (ret) {
 	case BLK_STS_OK:
-		break;
 	case BLK_STS_RESOURCE:
-		if (atomic_read(&sdev->device_busy) == 0 &&
-		    !scsi_device_blocked(sdev))
-			blk_mq_delay_run_hw_queue(hctx, SCSI_QUEUE_DELAY);
 		break;
 	default:
 		/*
-- 
2.9.4

[PATCH 4/6] blk-mq: introduce auto restart

[Ming Lei]

Both xen-blkfront and virito-blk stops queue when
the queue becomes busy, and restarts the queue after
one request is completed.

This patch implements this function in blk-mq core
by introducing BLK_MQ_F_AUTO_RESTART, then we can
remove stop/start queue related APIs and make 'stopped'
state invisible to drivers. It has caused lots of trouble
to expose this state to driver and let driver control
the state.

Given we can't get exact queue depth for one request queue
in case of TAG_SHARED, we only support this function on
non-TAG_SHARED devices/drivers.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/blk-mq.h |  1 +
 2 files changed, 42 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 041f7b7fa0d6..f3b582eb492f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -416,6 +416,39 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q,
 }
 EXPORT_SYMBOL_GPL(blk_mq_alloc_request_hctx);
 
+static void blk_mq_handle_auto_stop(struct blk_mq_hw_ctx *hctx)
+{
+	struct sbitmap_queue *sbq;
+
+	if (!(hctx->flags & BLK_MQ_F_AUTO_RESTART))
+		return;
+
+	set_bit(BLK_MQ_S_STOPPED, &hctx->state);
+	sbq = &hctx->tags->bitmap_tags;
+
+	/* order setting 'stopped' and reading queue depth */
+	smp_mb();
+
+	/*
+	 * all requests may be completed just before setting
+	 * 'stopped', so we have to check queue depth to see
+	 * if there is pending requests
+	 */
+	if (unlikely(!sbitmap_weight(&sbq->sb)))
+		clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
+}
+
+static void blk_mq_handle_auto_restart(struct blk_mq_hw_ctx *hctx)
+{
+	if (!(hctx->flags & BLK_MQ_F_AUTO_RESTART))
+		return;
+	/*
+	 * blk_mq_put_tag() implies one barrier, which is the pair of
+	 * smp_mb() in blk_mq_handle_auto_stop()
+	 */
+	clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
+}
+
 void blk_mq_free_request(struct request *rq)
 {
 	struct request_queue *q = rq->q;
@@ -446,6 +479,7 @@ void blk_mq_free_request(struct request *rq)
 	if (sched_tag != -1)
 		blk_mq_put_tag(hctx, hctx->sched_tags, ctx, sched_tag);
 	blk_mq_sched_restart(hctx);
+	blk_mq_handle_auto_restart(hctx);
 	blk_queue_exit(q);
 }
 EXPORT_SYMBOL_GPL(blk_mq_free_request);
@@ -1066,6 +1100,8 @@ bool blk_mq_dispatch_rq_list(struct request_queue *q, struct list_head *list)
 		list_splice_init(list, &hctx->dispatch);
 		spin_unlock(&hctx->lock);
 
+		blk_mq_handle_auto_stop(hctx);
+
 		/*
 		 * If SCHED_RESTART was set by the caller of this function and
 		 * it is no longer set that means that it was cleared by another
@@ -1502,6 +1538,7 @@ static void __blk_mq_try_issue_directly(struct blk_mq_hw_ctx *hctx,
 		return;
 	case BLK_STS_RESOURCE:
 		__blk_mq_requeue_request(rq);
+		blk_mq_handle_auto_stop(hctx);
 		goto insert;
 	default:
 		*cookie = BLK_QC_T_NONE;
@@ -2112,16 +2149,20 @@ static void queue_set_hctx_shared(struct request_queue *q, bool shared)
 {
 	struct blk_mq_hw_ctx *hctx;
 	int i;
+	struct blk_mq_tag_set *set = q->tag_set;
 
 	queue_for_each_hw_ctx(q, hctx, i) {
 		if (shared) {
 			if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
 				atomic_inc(&q->shared_hctx_restart);
 			hctx->flags |= BLK_MQ_F_TAG_SHARED;
+			hctx->flags &= ~BLK_MQ_F_AUTO_RESTART;
 		} else {
 			if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
 				atomic_dec(&q->shared_hctx_restart);
 			hctx->flags &= ~BLK_MQ_F_TAG_SHARED;
+			if (set->flags & BLK_MQ_F_AUTO_RESTART)
+				hctx->flags |= BLK_MQ_F_AUTO_RESTART;
 		}
 	}
 }
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 14542308d25b..251af99e9ba8 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -162,6 +162,7 @@ enum {
 	BLK_MQ_F_SHOULD_MERGE	= 1 << 0,
 	BLK_MQ_F_TAG_SHARED	= 1 << 1,
 	BLK_MQ_F_SG_MERGE	= 1 << 2,
+	BLK_MQ_F_AUTO_RESTART	= 1 << 3,
 	BLK_MQ_F_BLOCKING	= 1 << 5,
 	BLK_MQ_F_NO_SCHED	= 1 << 6,
 	BLK_MQ_F_ALLOC_POLICY_START_BIT = 8,
-- 
2.9.4

[PATCH 5/6] block: use BLK_MQ_F_AUTO_RESTART on virtio-blk and xen-blkfront

[Ming Lei]

Now blk-mq implements such function via BLK_MQ_F_AUTO_RESTART,
so just use that and remove related code in virtio-blk and xen-blkfront.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/virtio_blk.c   |  8 +-------
 drivers/block/xen-blkfront.c | 15 ++-------------
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 4e02aa5fdac0..3ab630e91306 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -189,7 +189,6 @@ static inline void virtblk_request_done(struct request *req)
 static void virtblk_done(struct virtqueue *vq)
 {
 	struct virtio_blk *vblk = vq->vdev->priv;
-	bool req_done = false;
 	int qid = vq->index;
 	struct virtblk_req *vbr;
 	unsigned long flags;
@@ -202,15 +201,11 @@ static void virtblk_done(struct virtqueue *vq)
 			struct request *req = blk_mq_rq_from_pdu(vbr);
 
 			blk_mq_complete_request(req);
-			req_done = true;
 		}
 		if (unlikely(virtqueue_is_broken(vq)))
 			break;
 	} while (!virtqueue_enable_cb(vq));
 
-	/* In case queue is stopped waiting for more buffers. */
-	if (req_done)
-		blk_mq_start_stopped_hw_queues(vblk->disk->queue, true);
 	spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
 }
 
@@ -271,7 +266,6 @@ static blk_status_t virtio_queue_rq(struct blk_mq_hw_ctx *hctx,
 		err = virtblk_add_req(vblk->vqs[qid].vq, vbr, vbr->sg, num);
 	if (err) {
 		virtqueue_kick(vblk->vqs[qid].vq);
-		blk_mq_stop_hw_queue(hctx);
 		spin_unlock_irqrestore(&vblk->vqs[qid].lock, flags);
 		/* Out of mem doesn't actually happen, since we fall back
 		 * to direct descriptors */
@@ -670,7 +664,7 @@ static int virtblk_probe(struct virtio_device *vdev)
 	vblk->tag_set.ops = &virtio_mq_ops;
 	vblk->tag_set.queue_depth = virtblk_queue_depth;
 	vblk->tag_set.numa_node = NUMA_NO_NODE;
-	vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
+	vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_AUTO_RESTART;
 	vblk->tag_set.cmd_size =
 		sizeof(struct virtblk_req) +
 		sizeof(struct scatterlist) * sg_elems;
diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
index 1578befda635..d80f867ffc3f 100644
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -907,7 +907,6 @@ static blk_status_t blkif_queue_rq(struct blk_mq_hw_ctx *hctx,
 
 out_busy:
 	spin_unlock_irqrestore(&rinfo->ring_lock, flags);
-	blk_mq_stop_hw_queue(hctx);
 	return BLK_STS_RESOURCE;
 }
 
@@ -975,7 +974,8 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
 	} else
 		info->tag_set.queue_depth = BLK_RING_SIZE(info);
 	info->tag_set.numa_node = NUMA_NO_NODE;
-	info->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE;
+	info->tag_set.flags = BLK_MQ_F_SHOULD_MERGE | BLK_MQ_F_SG_MERGE |
+		BLK_MQ_F_AUTO_RESTART;
 	info->tag_set.cmd_size = sizeof(struct blkif_req);
 	info->tag_set.driver_data = info;
 
@@ -1213,15 +1213,6 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
 	info->gd = NULL;
 }
 
-/* Already hold rinfo->ring_lock. */
-static inline void kick_pending_request_queues_locked(struct blkfront_ring_info *rinfo)
-{
-	if (!RING_FULL(&rinfo->ring)) {
-		blk_mq_start_stopped_hw_queues(rinfo->dev_info->rq, true);
-		blk_mq_kick_requeue_list(rinfo->dev_info->rq);
-	}
-}
-
 static void kick_pending_request_queues(struct blkfront_ring_info *rinfo)
 {
 	unsigned long flags;
@@ -1659,8 +1650,6 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
 	} else
 		rinfo->ring.sring->rsp_event = i + 1;
 
-	kick_pending_request_queues_locked(rinfo);
-
 	spin_unlock_irqrestore(&rinfo->ring_lock, flags);
 
 	return IRQ_HANDLED;
-- 
2.9.4

[PATCH 6/6] blk-mq: unexport APIs for start/stop queues

[Ming Lei]

Now no one uses these APIs anymore, so unexport them.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c         | 83 ++------------------------------------------------
 block/blk-mq.h         |  1 +
 include/linux/blk-mq.h |  8 -----
 3 files changed, 3 insertions(+), 89 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f3b582eb492f..70ce222da405 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1200,12 +1200,6 @@ static void __blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async,
 					 msecs_to_jiffies(msecs));
 }
 
-void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs)
-{
-	__blk_mq_delay_run_hw_queue(hctx, true, msecs);
-}
-EXPORT_SYMBOL(blk_mq_delay_run_hw_queue);
-
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
 {
 	__blk_mq_delay_run_hw_queue(hctx, async, 0);
@@ -1247,61 +1241,8 @@ bool blk_mq_queue_stopped(struct request_queue *q)
 }
 EXPORT_SYMBOL(blk_mq_queue_stopped);
 
-/*
- * This function is often used for pausing .queue_rq() by driver when
- * there isn't enough resource or some conditions aren't satisfied, and
- * BLK_MQ_RQ_QUEUE_BUSY is usually returned.
- *
- * We do not guarantee that dispatch can be drained or blocked
- * after blk_mq_stop_hw_queue() returns. Please use
- * blk_mq_quiesce_queue() for that requirement.
- */
-void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx)
-{
-	cancel_delayed_work(&hctx->run_work);
-
-	set_bit(BLK_MQ_S_STOPPED, &hctx->state);
-}
-EXPORT_SYMBOL(blk_mq_stop_hw_queue);
-
-/*
- * This function is often used for pausing .queue_rq() by driver when
- * there isn't enough resource or some conditions aren't satisfied, and
- * BLK_MQ_RQ_QUEUE_BUSY is usually returned.
- *
- * We do not guarantee that dispatch can be drained or blocked
- * after blk_mq_stop_hw_queues() returns. Please use
- * blk_mq_quiesce_queue() for that requirement.
- */
-void blk_mq_stop_hw_queues(struct request_queue *q)
-{
-	struct blk_mq_hw_ctx *hctx;
-	int i;
-
-	queue_for_each_hw_ctx(q, hctx, i)
-		blk_mq_stop_hw_queue(hctx);
-}
-EXPORT_SYMBOL(blk_mq_stop_hw_queues);
-
-void blk_mq_start_hw_queue(struct blk_mq_hw_ctx *hctx)
-{
-	clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
-
-	blk_mq_run_hw_queue(hctx, false);
-}
-EXPORT_SYMBOL(blk_mq_start_hw_queue);
-
-void blk_mq_start_hw_queues(struct request_queue *q)
-{
-	struct blk_mq_hw_ctx *hctx;
-	int i;
-
-	queue_for_each_hw_ctx(q, hctx, i)
-		blk_mq_start_hw_queue(hctx);
-}
-EXPORT_SYMBOL(blk_mq_start_hw_queues);
-
-void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
+static void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx,
+					  bool async)
 {
 	if (!blk_mq_hctx_stopped(hctx))
 		return;
@@ -1309,7 +1250,6 @@ void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx, bool async)
 	clear_bit(BLK_MQ_S_STOPPED, &hctx->state);
 	blk_mq_run_hw_queue(hctx, async);
 }
-EXPORT_SYMBOL_GPL(blk_mq_start_stopped_hw_queue);
 
 void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async)
 {
@@ -1319,7 +1259,6 @@ void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async)
 	queue_for_each_hw_ctx(q, hctx, i)
 		blk_mq_start_stopped_hw_queue(hctx, async);
 }
-EXPORT_SYMBOL(blk_mq_start_stopped_hw_queues);
 
 static void blk_mq_run_work_fn(struct work_struct *work)
 {
@@ -1344,24 +1283,6 @@ static void blk_mq_run_work_fn(struct work_struct *work)
 }
 
 
-void blk_mq_delay_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs)
-{
-	if (WARN_ON_ONCE(!blk_mq_hw_queue_mapped(hctx)))
-		return;
-
-	/*
-	 * Stop the hw queue, then modify currently delayed work.
-	 * This should prevent us from running the queue prematurely.
-	 * Mark the queue as auto-clearing STOPPED when it runs.
-	 */
-	blk_mq_stop_hw_queue(hctx);
-	set_bit(BLK_MQ_S_START_ON_RUN, &hctx->state);
-	kblockd_mod_delayed_work_on(blk_mq_hctx_next_cpu(hctx),
-					&hctx->run_work,
-					msecs_to_jiffies(msecs));
-}
-EXPORT_SYMBOL(blk_mq_delay_queue);
-
 static inline void __blk_mq_insert_req_list(struct blk_mq_hw_ctx *hctx,
 					    struct request *rq,
 					    bool at_head)
diff --git a/block/blk-mq.h b/block/blk-mq.h
index 60b01c0309bc..2a26c74eaca1 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -25,6 +25,7 @@ struct blk_mq_ctx {
 	struct kobject		kobj;
 } ____cacheline_aligned_in_smp;
 
+void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async);
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_freeze_queue(struct request_queue *q);
 void blk_mq_free_queue(struct request_queue *q);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 251af99e9ba8..203bc77fdea9 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -242,18 +242,10 @@ void blk_mq_delay_kick_requeue_list(struct request_queue *q, unsigned long msecs
 void blk_mq_complete_request(struct request *rq);
 
 bool blk_mq_queue_stopped(struct request_queue *q);
-void blk_mq_stop_hw_queue(struct blk_mq_hw_ctx *hctx);
-void blk_mq_start_hw_queue(struct blk_mq_hw_ctx *hctx);
-void blk_mq_stop_hw_queues(struct request_queue *q);
-void blk_mq_start_hw_queues(struct request_queue *q);
-void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
-void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async);
 void blk_mq_quiesce_queue(struct request_queue *q);
 void blk_mq_unquiesce_queue(struct request_queue *q);
-void blk_mq_delay_run_hw_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
 void blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_run_hw_queues(struct request_queue *q, bool async);
-void blk_mq_delay_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
 void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
 		busy_tag_iter_fn *fn, void *priv);
 void blk_mq_freeze_queue(struct request_queue *q);
-- 
2.9.4

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> stopping queue may cause race and may not stop the queue really
> after the API returns, and we have improved quiescing
> interface and it really can block dispatching once it returns.
> 
> So switch to quiesce/unquiece like what we did on other drivers
> (NVMe, NBD, mtip32xx, ...)
> 
> The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> used in blkif_queue_rq() and blkif_interrupt() are for congestion
> control, we leave it as it is since it is safe for this usage.

Again I yet don't understand the difference between those two, neither
why start/stop is not fixed instead of introducing quiesce/unquiece.
Not to mention that start/stop is not documented, which makes all this
even more fun.

Anyway I would like to ask, is the way to re-start a stopped queue the
same way to unquiece?

If not I would rather prefer that start/stop or quiece/unquiece is
used exclusively, in order to not make the code even more complex. It
seems fairly easy to mess up and call "start" on a "quiesced" queue
(or the other way around).

Roger.

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> stopping queue may cause race and may not stop the queue really
> after the API returns, and we have improved quiescing
> interface and it really can block dispatching once it returns.
> 
> So switch to quiesce/unquiece like what we did on other drivers
> (NVMe, NBD, mtip32xx, ...)
> 
> The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> used in blkif_queue_rq() and blkif_interrupt() are for congestion
> control, we leave it as it is since it is safe for this usage.

Again I yet don't understand the difference between those two, neither
why start/stop is not fixed instead of introducing quiesce/unquiece.
Not to mention that start/stop is not documented, which makes all this
even more fun.

Anyway I would like to ask, is the way to re-start a stopped queue the
same way to unquiece?

If not I would rather prefer that start/stop or quiece/unquiece is
used exclusively, in order to not make the code even more complex. It
seems fairly easy to mess up and call "start" on a "quiesced" queue
(or the other way around).

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monn� wrote:
> On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > stopping queue may cause race and may not stop the queue really
> > after the API returns, and we have improved quiescing
> > interface and it really can block dispatching once it returns.
> > 
> > So switch to quiesce/unquiece like what we did on other drivers
> > (NVMe, NBD, mtip32xx, ...)
> > 
> > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > control, we leave it as it is since it is safe for this usage.
> 
> Again I yet don't understand the difference between those two, neither
> why start/stop is not fixed instead of introducing quiesce/unquiece.

There are two usages covered by start/stop now:

- congestion control for handling queue busy(BLK_STS_RESOURCE), now
only xen-blkfront and virtio-blk use that

- other usages, such as in xlvbd_release_gendisk(), for blocking
IO to driver/device

For the 1st case, it is usually fine to use stop/start

For the 2nd case, stop queue isn't enough, and we can't guarantee 
no IO is dispatched to device/driver after returning from stop queue,
for details. Most of this usage have been fixed by  Sagi Grimberg:

        http://marc.info/?t=149927415900006&r=1&w=2

start/stop is a bad name for 2nd usage too, what we really want
is to block IO to driver/devices, so we should use quiesce/unquiesce.

xen-blkfront is missed in Sagi's patchset which has been merged to
linus tree already, so this patch just fixes xen-blkfront simply like
other patches.

We can't use quiesce/unquiesce for replacing stop/start in the
case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
block IO for this usage actually.

> Not to mention that start/stop is not documented, which makes all this
> even more fun.

Did you read comment of blk_mq_stop_hw_queue() and
blk_mq_stop_hw_queues() in linus tree?

> 
> Anyway I would like to ask, is the way to re-start a stopped queue the
> same way to unquiece?

I don't know what your exact question, but it is definitely that
unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
depend on 'stopped' state any more if you take a look at the code.

> 
> If not I would rather prefer that start/stop or quiece/unquiece is
> used exclusively, in order to not make the code even more complex. It

I do not think the code becomes more complex, please see the line change
of this patch:

 1 file changed, 8 insertions(+), 14 deletions(-)

then see the change of the whole patchset:

 8 files changed, 54 insertions(+), 129 deletions(-)

It is really a cleanup and simplifying.

> seems fairly easy to mess up and call "start" on a "quiesced" queue
> (or the other way around).

Definitely it shouldn't be worried because start/stop is removed
in this patchset.

-- 
Ming

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote:
> On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > stopping queue may cause race and may not stop the queue really
> > after the API returns, and we have improved quiescing
> > interface and it really can block dispatching once it returns.
> > 
> > So switch to quiesce/unquiece like what we did on other drivers
> > (NVMe, NBD, mtip32xx, ...)
> > 
> > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > control, we leave it as it is since it is safe for this usage.
> 
> Again I yet don't understand the difference between those two, neither
> why start/stop is not fixed instead of introducing quiesce/unquiece.

There are two usages covered by start/stop now:

- congestion control for handling queue busy(BLK_STS_RESOURCE), now
only xen-blkfront and virtio-blk use that

- other usages, such as in xlvbd_release_gendisk(), for blocking
IO to driver/device

For the 1st case, it is usually fine to use stop/start

For the 2nd case, stop queue isn't enough, and we can't guarantee 
no IO is dispatched to device/driver after returning from stop queue,
for details. Most of this usage have been fixed by  Sagi Grimberg:

        http://marc.info/?t=149927415900006&r=1&w=2

start/stop is a bad name for 2nd usage too, what we really want
is to block IO to driver/devices, so we should use quiesce/unquiesce.

xen-blkfront is missed in Sagi's patchset which has been merged to
linus tree already, so this patch just fixes xen-blkfront simply like
other patches.

We can't use quiesce/unquiesce for replacing stop/start in the
case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
block IO for this usage actually.

> Not to mention that start/stop is not documented, which makes all this
> even more fun.

Did you read comment of blk_mq_stop_hw_queue() and
blk_mq_stop_hw_queues() in linus tree?

> 
> Anyway I would like to ask, is the way to re-start a stopped queue the
> same way to unquiece?

I don't know what your exact question, but it is definitely that
unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
depend on 'stopped' state any more if you take a look at the code.

> 
> If not I would rather prefer that start/stop or quiece/unquiece is
> used exclusively, in order to not make the code even more complex. It

I do not think the code becomes more complex, please see the line change
of this patch:

 1 file changed, 8 insertions(+), 14 deletions(-)

then see the change of the whole patchset:

 8 files changed, 54 insertions(+), 129 deletions(-)

It is really a cleanup and simplifying.

> seems fairly easy to mess up and call "start" on a "quiesced" queue
> (or the other way around).

Definitely it shouldn't be worried because start/stop is removed
in this patchset.

-- 
Ming

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monn� wrote:
> > On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > > stopping queue may cause race and may not stop the queue really
> > > after the API returns, and we have improved quiescing
> > > interface and it really can block dispatching once it returns.
> > > 
> > > So switch to quiesce/unquiece like what we did on other drivers
> > > (NVMe, NBD, mtip32xx, ...)
> > > 
> > > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > > control, we leave it as it is since it is safe for this usage.
> > 
> > Again I yet don't understand the difference between those two, neither
> > why start/stop is not fixed instead of introducing quiesce/unquiece.
> 
> There are two usages covered by start/stop now:
> 
> - congestion control for handling queue busy(BLK_STS_RESOURCE), now
> only xen-blkfront and virtio-blk use that
> 
> - other usages, such as in xlvbd_release_gendisk(), for blocking
> IO to driver/device
> 
> For the 1st case, it is usually fine to use stop/start
> 
> For the 2nd case, stop queue isn't enough, and we can't guarantee 
> no IO is dispatched to device/driver after returning from stop queue,
> for details. Most of this usage have been fixed by  Sagi Grimberg:

OK, so basically after calling stop the queue might still be running.

> We can't use quiesce/unquiesce for replacing stop/start in the
> case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
> block IO for this usage actually.

Do you mean that quiesce/unquiesce cannot be used while holding a
spinlock?

> 
> > Not to mention that start/stop is not documented, which makes all this
> > even more fun.
> 
> Did you read comment of blk_mq_stop_hw_queue() and
> blk_mq_stop_hw_queues() in linus tree?

OK, this has been added very recently.

> > 
> > Anyway I would like to ask, is the way to re-start a stopped queue the
> > same way to unquiece?
> 
> I don't know what your exact question, but it is definitely that
> unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
> depend on 'stopped' state any more if you take a look at the code.
> 
> > 
> > If not I would rather prefer that start/stop or quiece/unquiece is
> > used exclusively, in order to not make the code even more complex. It
> 
> I do not think the code becomes more complex, please see the line change
> of this patch:

Before this patch blkfront used:
blk_mq_stop_hw_queues
blk_mq_start_stopped_hw_queues
blk_mq_kick_requeue_list

After the patch it uses:
blk_mq_quiesce_queue
blk_mq_unquiesce_queue
blk_mq_stop_hw_queues
blk_mq_start_stopped_hw_queues
blk_mq_kick_requeue_list
blk_mq_run_hw_queues

It's not about line changes, but the amount of interfaces blkfront has
to use. Apart from introducing the quiesce/unquiesce, you also
introduce a call to blk_mq_run_hw_queues, which is not documented in
the commit message.

> > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > (or the other way around).
> 
> Definitely it shouldn't be worried because start/stop is removed
> in this patchset.

Hm, how is that? I haven't seen any patch to blkfront to remove the
usage of start/stop, am I missing something?

Roger.

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote:
> > On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > > stopping queue may cause race and may not stop the queue really
> > > after the API returns, and we have improved quiescing
> > > interface and it really can block dispatching once it returns.
> > > 
> > > So switch to quiesce/unquiece like what we did on other drivers
> > > (NVMe, NBD, mtip32xx, ...)
> > > 
> > > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > > control, we leave it as it is since it is safe for this usage.
> > 
> > Again I yet don't understand the difference between those two, neither
> > why start/stop is not fixed instead of introducing quiesce/unquiece.
> 
> There are two usages covered by start/stop now:
> 
> - congestion control for handling queue busy(BLK_STS_RESOURCE), now
> only xen-blkfront and virtio-blk use that
> 
> - other usages, such as in xlvbd_release_gendisk(), for blocking
> IO to driver/device
> 
> For the 1st case, it is usually fine to use stop/start
> 
> For the 2nd case, stop queue isn't enough, and we can't guarantee 
> no IO is dispatched to device/driver after returning from stop queue,
> for details. Most of this usage have been fixed by  Sagi Grimberg:

OK, so basically after calling stop the queue might still be running.

> We can't use quiesce/unquiesce for replacing stop/start in the
> case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
> block IO for this usage actually.

Do you mean that quiesce/unquiesce cannot be used while holding a
spinlock?

> 
> > Not to mention that start/stop is not documented, which makes all this
> > even more fun.
> 
> Did you read comment of blk_mq_stop_hw_queue() and
> blk_mq_stop_hw_queues() in linus tree?

OK, this has been added very recently.

> > 
> > Anyway I would like to ask, is the way to re-start a stopped queue the
> > same way to unquiece?
> 
> I don't know what your exact question, but it is definitely that
> unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
> depend on 'stopped' state any more if you take a look at the code.
> 
> > 
> > If not I would rather prefer that start/stop or quiece/unquiece is
> > used exclusively, in order to not make the code even more complex. It
> 
> I do not think the code becomes more complex, please see the line change
> of this patch:

Before this patch blkfront used:
blk_mq_stop_hw_queues
blk_mq_start_stopped_hw_queues
blk_mq_kick_requeue_list

After the patch it uses:
blk_mq_quiesce_queue
blk_mq_unquiesce_queue
blk_mq_stop_hw_queues
blk_mq_start_stopped_hw_queues
blk_mq_kick_requeue_list
blk_mq_run_hw_queues

It's not about line changes, but the amount of interfaces blkfront has
to use. Apart from introducing the quiesce/unquiesce, you also
introduce a call to blk_mq_run_hw_queues, which is not documented in
the commit message.

> > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > (or the other way around).
> 
> Definitely it shouldn't be worried because start/stop is removed
> in this patchset.

Hm, how is that? I haven't seen any patch to blkfront to remove the
usage of start/stop, am I missing something?

Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monn� wrote:
> On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monn� wrote:
> > > On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > > > stopping queue may cause race and may not stop the queue really
> > > > after the API returns, and we have improved quiescing
> > > > interface and it really can block dispatching once it returns.
> > > > 
> > > > So switch to quiesce/unquiece like what we did on other drivers
> > > > (NVMe, NBD, mtip32xx, ...)
> > > > 
> > > > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > > > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > > > control, we leave it as it is since it is safe for this usage.
> > > 
> > > Again I yet don't understand the difference between those two, neither
> > > why start/stop is not fixed instead of introducing quiesce/unquiece.
> > 
> > There are two usages covered by start/stop now:
> > 
> > - congestion control for handling queue busy(BLK_STS_RESOURCE), now
> > only xen-blkfront and virtio-blk use that
> > 
> > - other usages, such as in xlvbd_release_gendisk(), for blocking
> > IO to driver/device
> > 
> > For the 1st case, it is usually fine to use stop/start
> > 
> > For the 2nd case, stop queue isn't enough, and we can't guarantee 
> > no IO is dispatched to device/driver after returning from stop queue,
> > for details. Most of this usage have been fixed by  Sagi Grimberg:
> 
> OK, so basically after calling stop the queue might still be running.
> 
> > We can't use quiesce/unquiesce for replacing stop/start in the
> > case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
> > block IO for this usage actually.
> 
> Do you mean that quiesce/unquiesce cannot be used while holding a
> spinlock?

Yes.

> 
> > 
> > > Not to mention that start/stop is not documented, which makes all this
> > > even more fun.
> > 
> > Did you read comment of blk_mq_stop_hw_queue() and
> > blk_mq_stop_hw_queues() in linus tree?
> 
> OK, this has been added very recently.
> 
> > > 
> > > Anyway I would like to ask, is the way to re-start a stopped queue the
> > > same way to unquiece?
> > 
> > I don't know what your exact question, but it is definitely that
> > unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
> > depend on 'stopped' state any more if you take a look at the code.
> > 
> > > 
> > > If not I would rather prefer that start/stop or quiece/unquiece is
> > > used exclusively, in order to not make the code even more complex. It
> > 
> > I do not think the code becomes more complex, please see the line change
> > of this patch:
> 
> Before this patch blkfront used:
> blk_mq_stop_hw_queues
> blk_mq_start_stopped_hw_queues
> blk_mq_kick_requeue_list
> 
> After the patch it uses:
> blk_mq_quiesce_queue
> blk_mq_unquiesce_queue
> blk_mq_stop_hw_queues
> blk_mq_start_stopped_hw_queues
> blk_mq_kick_requeue_list
> blk_mq_run_hw_queues
> 
> It's not about line changes, but the amount of interfaces blkfront has
> to use. Apart from introducing the quiesce/unquiesce, you also
> introduce a call to blk_mq_run_hw_queues, which is not documented in
> the commit message.
> 
> > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > (or the other way around).
> > 
> > Definitely it shouldn't be worried because start/stop is removed
> > in this patchset.
> 
> Hm, how is that? I haven't seen any patch to blkfront to remove the
> usage of start/stop, am I missing something?

http://marc.info/?l=linux-block&m=150007418321196&w=2

-- 
Ming

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monné wrote:
> On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote:
> > > On Sat, Jul 15, 2017 at 07:15:56AM +0800, Ming Lei wrote:
> > > > stopping queue may cause race and may not stop the queue really
> > > > after the API returns, and we have improved quiescing
> > > > interface and it really can block dispatching once it returns.
> > > > 
> > > > So switch to quiesce/unquiece like what we did on other drivers
> > > > (NVMe, NBD, mtip32xx, ...)
> > > > 
> > > > The blk_mq_stop_hw_queues() and blk_mq_start_stopped_hw_queues()
> > > > used in blkif_queue_rq() and blkif_interrupt() are for congestion
> > > > control, we leave it as it is since it is safe for this usage.
> > > 
> > > Again I yet don't understand the difference between those two, neither
> > > why start/stop is not fixed instead of introducing quiesce/unquiece.
> > 
> > There are two usages covered by start/stop now:
> > 
> > - congestion control for handling queue busy(BLK_STS_RESOURCE), now
> > only xen-blkfront and virtio-blk use that
> > 
> > - other usages, such as in xlvbd_release_gendisk(), for blocking
> > IO to driver/device
> > 
> > For the 1st case, it is usually fine to use stop/start
> > 
> > For the 2nd case, stop queue isn't enough, and we can't guarantee 
> > no IO is dispatched to device/driver after returning from stop queue,
> > for details. Most of this usage have been fixed by  Sagi Grimberg:
> 
> OK, so basically after calling stop the queue might still be running.
> 
> > We can't use quiesce/unquiesce for replacing stop/start in the
> > case of BLK_STS_RESOURCE, because quiesce may sleep, and we needn't
> > block IO for this usage actually.
> 
> Do you mean that quiesce/unquiesce cannot be used while holding a
> spinlock?

Yes.

> 
> > 
> > > Not to mention that start/stop is not documented, which makes all this
> > > even more fun.
> > 
> > Did you read comment of blk_mq_stop_hw_queue() and
> > blk_mq_stop_hw_queues() in linus tree?
> 
> OK, this has been added very recently.
> 
> > > 
> > > Anyway I would like to ask, is the way to re-start a stopped queue the
> > > same way to unquiece?
> > 
> > I don't know what your exact question, but it is definitely that
> > unquiesce is counter part of quiesce, and quiesce/unquiesce doesn't
> > depend on 'stopped' state any more if you take a look at the code.
> > 
> > > 
> > > If not I would rather prefer that start/stop or quiece/unquiece is
> > > used exclusively, in order to not make the code even more complex. It
> > 
> > I do not think the code becomes more complex, please see the line change
> > of this patch:
> 
> Before this patch blkfront used:
> blk_mq_stop_hw_queues
> blk_mq_start_stopped_hw_queues
> blk_mq_kick_requeue_list
> 
> After the patch it uses:
> blk_mq_quiesce_queue
> blk_mq_unquiesce_queue
> blk_mq_stop_hw_queues
> blk_mq_start_stopped_hw_queues
> blk_mq_kick_requeue_list
> blk_mq_run_hw_queues
> 
> It's not about line changes, but the amount of interfaces blkfront has
> to use. Apart from introducing the quiesce/unquiesce, you also
> introduce a call to blk_mq_run_hw_queues, which is not documented in
> the commit message.
> 
> > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > (or the other way around).
> > 
> > Definitely it shouldn't be worried because start/stop is removed
> > in this patchset.
> 
> Hm, how is that? I haven't seen any patch to blkfront to remove the
> usage of start/stop, am I missing something?

http://marc.info/?l=linux-block&m=150007418321196&w=2

-- 
Ming

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Tue, Jul 18, 2017 at 08:53:28AM +0800, Ming Lei wrote:
> On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monn� wrote:
> > On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monn� wrote:
> > > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > > (or the other way around).
> > > 
> > > Definitely it shouldn't be worried because start/stop is removed
> > > in this patchset.
> > 
> > Hm, how is that? I haven't seen any patch to blkfront to remove the
> > usage of start/stop, am I missing something?
> 
> http://secure-web.cisco.com/19iIG2bc3Ce2_t8mSF4YQ2LopepIvjAOqnPAXQ_QahSNOEvLBmechzNZ0pQOPfSRhy3hyC4t6L-JJzu81FvWRyLoBmhtq7F9uEk7XZsTt-XNxLN1KdEJvEeUAWelYSd9NthbvpGad6DmpJ0QCHSOgq4mRcDaqlz5mRmNkTvxls-ri1qCqy6am0jTDdraGINb_BUyV0894BtaFOMGJGEKLjcrBfFfT6C5XHSEdsiuB12ZLIUyaRVyU0TCUz6Sm4uhD/http%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-block%26m%3D150007418321196%26w%3D2

Sadly I have not been CCed or otherwise I've lost patch #5. Related to
that patch AFAICT kick_pending_request_queues can be removed, since
it's only used by one caller (or it can even be removed in this patch,
#1).

Thanks, Roger.

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Roger Pau Monné]

On Tue, Jul 18, 2017 at 08:53:28AM +0800, Ming Lei wrote:
> On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monné wrote:
> > On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote:
> > > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > > (or the other way around).
> > > 
> > > Definitely it shouldn't be worried because start/stop is removed
> > > in this patchset.
> > 
> > Hm, how is that? I haven't seen any patch to blkfront to remove the
> > usage of start/stop, am I missing something?
> 
> http://secure-web.cisco.com/19iIG2bc3Ce2_t8mSF4YQ2LopepIvjAOqnPAXQ_QahSNOEvLBmechzNZ0pQOPfSRhy3hyC4t6L-JJzu81FvWRyLoBmhtq7F9uEk7XZsTt-XNxLN1KdEJvEeUAWelYSd9NthbvpGad6DmpJ0QCHSOgq4mRcDaqlz5mRmNkTvxls-ri1qCqy6am0jTDdraGINb_BUyV0894BtaFOMGJGEKLjcrBfFfT6C5XHSEdsiuB12ZLIUyaRVyU0TCUz6Sm4uhD/http%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-block%26m%3D150007418321196%26w%3D2

Sadly I have not been CCed or otherwise I've lost patch #5. Related to
that patch AFAICT kick_pending_request_queues can be removed, since
it's only used by one caller (or it can even be removed in this patch,
#1).

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Tue, Jul 18, 2017 at 08:40:18AM +0100, Roger Pau Monn� wrote:
> On Tue, Jul 18, 2017 at 08:53:28AM +0800, Ming Lei wrote:
> > On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monn� wrote:
> > > On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > > > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monn� wrote:
> > > > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > > > (or the other way around).
> > > > 
> > > > Definitely it shouldn't be worried because start/stop is removed
> > > > in this patchset.
> > > 
> > > Hm, how is that? I haven't seen any patch to blkfront to remove the
> > > usage of start/stop, am I missing something?
> > 
> > http://secure-web.cisco.com/19iIG2bc3Ce2_t8mSF4YQ2LopepIvjAOqnPAXQ_QahSNOEvLBmechzNZ0pQOPfSRhy3hyC4t6L-JJzu81FvWRyLoBmhtq7F9uEk7XZsTt-XNxLN1KdEJvEeUAWelYSd9NthbvpGad6DmpJ0QCHSOgq4mRcDaqlz5mRmNkTvxls-ri1qCqy6am0jTDdraGINb_BUyV0894BtaFOMGJGEKLjcrBfFfT6C5XHSEdsiuB12ZLIUyaRVyU0TCUz6Sm4uhD/http%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-block%26m%3D150007418321196%26w%3D2
> 
> Sadly I have not been CCed or otherwise I've lost patch #5. Related to
> that patch AFAICT kick_pending_request_queues can be removed, since
> it's only used by one caller (or it can even be removed in this patch,
> #1).

There are two callers: blkfront_connect() and blkif_restart_queue().


-- 
Ming

Re: [PATCH 1/6] xen-blkfront: quiesce/unquiesce queue instead of start/stop queues

[Ming Lei]

On Tue, Jul 18, 2017 at 08:40:18AM +0100, Roger Pau Monné wrote:
> On Tue, Jul 18, 2017 at 08:53:28AM +0800, Ming Lei wrote:
> > On Mon, Jul 17, 2017 at 05:02:27PM +0100, Roger Pau Monné wrote:
> > > On Mon, Jul 17, 2017 at 11:06:28PM +0800, Ming Lei wrote:
> > > > On Mon, Jul 17, 2017 at 12:20:56PM +0100, Roger Pau Monné wrote:
> > > > > seems fairly easy to mess up and call "start" on a "quiesced" queue
> > > > > (or the other way around).
> > > > 
> > > > Definitely it shouldn't be worried because start/stop is removed
> > > > in this patchset.
> > > 
> > > Hm, how is that? I haven't seen any patch to blkfront to remove the
> > > usage of start/stop, am I missing something?
> > 
> > http://secure-web.cisco.com/19iIG2bc3Ce2_t8mSF4YQ2LopepIvjAOqnPAXQ_QahSNOEvLBmechzNZ0pQOPfSRhy3hyC4t6L-JJzu81FvWRyLoBmhtq7F9uEk7XZsTt-XNxLN1KdEJvEeUAWelYSd9NthbvpGad6DmpJ0QCHSOgq4mRcDaqlz5mRmNkTvxls-ri1qCqy6am0jTDdraGINb_BUyV0894BtaFOMGJGEKLjcrBfFfT6C5XHSEdsiuB12ZLIUyaRVyU0TCUz6Sm4uhD/http%3A%2F%2Fmarc.info%2F%3Fl%3Dlinux-block%26m%3D150007418321196%26w%3D2
> 
> Sadly I have not been CCed or otherwise I've lost patch #5. Related to
> that patch AFAICT kick_pending_request_queues can be removed, since
> it's only used by one caller (or it can even be removed in this patch,
> #1).

There are two callers: blkfront_connect() and blkif_restart_queue().


-- 
Ming

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel