[PATCH 0/3] mtip32xx: make it working with mq scheduler

[PATCH 0/3] mtip32xx: make it working with mq scheduler

[Ming Lei]

Hi,

This patchset fixes two issues on mtip32xx when mq schedluer is
used, and the issue is introduced in v4.11-rc:

1) the 1st two patches fixes one kernel oops trigered in blk_mq_tag_to_rq()

2) the 3rd patch fixes hardware failure and system panic which is caused
by misusing request index as request tag in .init_request()


Please consider this patchset for v4.11.

Sorry for being a bit late because my test system is just recovered.

Thanks,
Ming

Ming Lei (3):
  blk-mq: export blk_mq_get_driver_tag
  mtip32xx: assign driver tag
  mtip32xx: use runtime tag to initialize command header

 block/blk-mq.c                    |  1 +
 drivers/block/mtip32xx/mtip32xx.c | 38 ++++++++++++++++++++++++++------------
 include/linux/blk-mq.h            |  3 +++
 3 files changed, 30 insertions(+), 12 deletions(-)

-- 
2.9.3

[PATCH 1/3] blk-mq: export blk_mq_get_driver_tag

[Ming Lei]

Before blk-mq iosched is introduced, drivers may suppose
the the allocated request includes one valid tag number,
but it becomes not true any more after we bring mq iosched in.

So introduces this helper to make driver get req's hw tag.

Mtip32xx needs this helper for sending internal command via
one request.

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

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 572966f49596..4614ab8b941e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -865,6 +865,7 @@ bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
 		*hctx = data.hctx;
 	return rq->tag != -1;
 }
+EXPORT_SYMBOL(blk_mq_get_driver_tag);
 
 static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx,
 				    struct request *rq)
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 9382c5da7a2e..ecb7b681aa7c 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -255,6 +255,9 @@ int blk_mq_reinit_tagset(struct blk_mq_tag_set *set);
 int blk_mq_map_queues(struct blk_mq_tag_set *set);
 void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues);
 
+bool blk_mq_get_driver_tag(struct request *rq, struct blk_mq_hw_ctx **hctx,
+			   bool wait);
+
 /*
  * Driver command data is immediately after the request. So subtract request
  * size to get back to the original request, add request size to get the PDU.
-- 
2.9.3

[PATCH 2/3] mtip32xx: assign driver tag

[Ming Lei]

Assign driver tag for internal command, so that we
can avoid kernel oops[1] trigered in blk_mq_tag_to_rq().

[1] https://bugzilla.kernel.org/show_bug.cgi?id=195429

Reported-by: Jozef Mikovic <jmikovic@redhat.com>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index f96ab717534c..8cfe5a6edb05 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -180,6 +180,8 @@ static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd)
 	if (IS_ERR(rq))
 		return NULL;
 
+	WARN_ON(!blk_mq_get_driver_tag(rq, NULL, true));
+
 	return blk_mq_rq_to_pdu(rq);
 }
 
-- 
2.9.3

[PATCH 3/3] mtip32xx: use runtime tag to initialize command header

[Ming Lei]

mtip32xx supposes that 'request_idx' passed to .init_request()
is tag of the request, and use that as request's tag to initialize
command header.

After MQ IO scheduler is in, request tag assigned isn't same with
the request index anymore, so cause strange hardware failure on
mtip32xx, even whole system panic is triggered.

This patch fixes the issue by initializing command header via
request's real tag.

Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 drivers/block/mtip32xx/mtip32xx.c | 36 ++++++++++++++++++++++++------------
 1 file changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 8cfe5a6edb05..b122dc0368f4 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -169,6 +169,25 @@ static bool mtip_check_surprise_removal(struct pci_dev *pdev)
 	return false; /* device present */
 }
 
+/* we have to use runtime tag to setup command header */
+static void mtip_init_cmd_header(struct request *rq)
+{
+	struct driver_data *dd = rq->q->queuedata;
+	struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq);
+	u32 host_cap_64 = readl(dd->mmio + HOST_CAP) & HOST_CAP_64;
+
+	/* Point the command headers at the command tables. */
+	cmd->command_header = dd->port->command_list +
+				(sizeof(struct mtip_cmd_hdr) * rq->tag);
+	cmd->command_header_dma = dd->port->command_list_dma +
+				(sizeof(struct mtip_cmd_hdr) * rq->tag);
+
+	if (host_cap_64)
+		cmd->command_header->ctbau = __force_bit2int cpu_to_le32((cmd->command_dma >> 16) >> 16);
+
+	cmd->command_header->ctba = __force_bit2int cpu_to_le32(cmd->command_dma & 0xFFFFFFFF);
+}
+
 static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd)
 {
 	struct request *rq;
@@ -182,6 +201,9 @@ static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd)
 
 	WARN_ON(!blk_mq_get_driver_tag(rq, NULL, true));
 
+	/* Internal cmd isn't submitted via .queue_rq */
+	mtip_init_cmd_header(rq);
+
 	return blk_mq_rq_to_pdu(rq);
 }
 
@@ -3809,6 +3831,8 @@ static int mtip_queue_rq(struct blk_mq_hw_ctx *hctx,
 	struct request *rq = bd->rq;
 	int ret;
 
+	mtip_init_cmd_header(rq);
+
 	if (unlikely(mtip_check_unal_depth(hctx, rq)))
 		return BLK_MQ_RQ_QUEUE_BUSY;
 
@@ -3840,7 +3864,6 @@ static int mtip_init_cmd(void *data, struct request *rq, unsigned int hctx_idx,
 {
 	struct driver_data *dd = data;
 	struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq);
-	u32 host_cap_64 = readl(dd->mmio + HOST_CAP) & HOST_CAP_64;
 
 	/*
 	 * For flush requests, request_idx starts at the end of the
@@ -3857,17 +3880,6 @@ static int mtip_init_cmd(void *data, struct request *rq, unsigned int hctx_idx,
 
 	memset(cmd->command, 0, CMD_DMA_ALLOC_SZ);
 
-	/* Point the command headers at the command tables. */
-	cmd->command_header = dd->port->command_list +
-				(sizeof(struct mtip_cmd_hdr) * request_idx);
-	cmd->command_header_dma = dd->port->command_list_dma +
-				(sizeof(struct mtip_cmd_hdr) * request_idx);
-
-	if (host_cap_64)
-		cmd->command_header->ctbau = __force_bit2int cpu_to_le32((cmd->command_dma >> 16) >> 16);
-
-	cmd->command_header->ctba = __force_bit2int cpu_to_le32(cmd->command_dma & 0xFFFFFFFF);
-
 	sg_init_table(cmd->sg, MTIP_MAX_SG);
 	return 0;
 }
-- 
2.9.3

Re: [PATCH 3/3] mtip32xx: use runtime tag to initialize command header

[Christoph Hellwig]

On Wed, Apr 19, 2017 at 08:31:42AM +0800, Ming Lei wrote:
> mtip32xx supposes that 'request_idx' passed to .init_request()
> is tag of the request, and use that as request's tag to initialize
> command header.
> 
> After MQ IO scheduler is in, request tag assigned isn't same with
> the request index anymore, so cause strange hardware failure on
> mtip32xx, even whole system panic is triggered.
> 
> This patch fixes the issue by initializing command header via
> request's real tag.

This patch looks fine:

Reviewed-by: Christoph Hellwig <hch@lst.de>

But we should also remove the request_idx parameter from the
init_request and exit_request methods given that they can't be
used once a scheduler is in use.

Re: [PATCH 0/3] mtip32xx: make it working with mq scheduler

[Christoph Hellwig]

Hi Ming,

I don't think your patch goes int the right direction.  The mtip32xx
driver never submits the requests it allocates using blk_mq_alloc_request.

So to fix this it should simply kmalloc the request, set a tag aside
and not use a block layer request for it at all, similar to what nvme
does for the AER command.

Re: [PATCH 0/3] mtip32xx: make it working with mq scheduler

[Ming Lei]

On Tue, Apr 18, 2017 at 11:27:13PM -0700, Christoph Hellwig wrote:
> Hi Ming,
> 
> I don't think your patch goes int the right direction.  The mtip32xx
> driver never submits the requests it allocates using blk_mq_alloc_request.
> So to fix this it should simply kmalloc the request, set a tag aside
> and not use a block layer request for it at all, similar to what nvme
> does for the AER command.

That is only about the internal command part(patch 1 and patch 2),
and the patch 3 is required regardless.

I choose to use blk_mq_get_driver_tag() for the 1st fix becasue it is the
simpleset way, also given blk_mq_alloc_request() has been working on that
for long time.

If We do that via kmalloc, the command has to be initialzed as the way in
.init_cmd(), the reserved tag space has to be accessed exclusively like what
block layer provides, and mtip_cmd_from_tag() need to be fixed too.
We still have to pass 'reserved_tags = 1' to blk-mq anyway.

That is why I suggest to use blk_mq_get_driver_tag() to fix the issue.

Thanks,
Ming