[4/6] mtip32xx: convert internal command issue to block IO path

Commit Message

Jens Axboe May 2, 2017, 3:09 p.m. UTC

On 05/02/2017 08:47 AM, Jens Axboe wrote:
> On 05/02/2017 07:53 AM, Jens Axboe wrote:
>> On 05/02/2017 01:28 AM, Christoph Hellwig wrote:
>>> Looks fine for now:
>>>
>>> Reviewed-by: Christoph Hellwig <[e-mail]>
>>>
>>> But rather sooner than later we need to make this path at least go
>>> through the normal end_request processing.  Without that we're just
>>> bound to run into problems like we had with the tag changes again
>>> when the driver is using the block layer APIs incompletely.
>>
>> I completely agree, I'll see if I can find some time to do that
>> soon.
> 
> Seems like a fine thing to do over morning coffee. How about something
> like the below? It's on top of the previous series. Basically this kills
> the private completion handler and data, which were basically redundant
> as they did nothing but spew a warning in case of an error.
> 
> It'd be nice to unify the INTERNAL and regular commands, but we don't
> have to do that to use the block layer APIs. If someone else wants to do
> that, be my guest.
> 
> It compiles and loads fine on my setup, did some basic testing it that
> went fine. Would be nice if someone else could look it over as well, to
> verify that I didn't drop any of the 0-vs-error setting. I think it's
> fine, we just need to ensure that cmd->status is always set to the
> appropriate error, then we propagate that through the mtip32xx softirq
> handler to the block layer.

Looks like I was missing one error conversion, on the internal request
path for taskfile TFE. Changed mtip_complete_command() to take a status
argument, and fixup that case too.

Comments

Christoph Hellwig May 2, 2017, 3:16 p.m. UTC | #1

This looks reasonable to me, although of course I don't have a way
to test it.

Any reason for the move from ->end_io_data to ->special?  I thought
that ->special was something we'd get rid of sooner or later now
that we can have additional per-cmd data even for !mq.

Jens Axboe May 2, 2017, 3:25 p.m. UTC | #2

On 05/02/2017 09:16 AM, Christoph Hellwig wrote:
> This looks reasonable to me, although of course I don't have a way
> to test it.

I've got a few cards at least.

> Any reason for the move from ->end_io_data to ->special?  I thought
> that ->special was something we'd get rid of sooner or later now
> that we can have additional per-cmd data even for !mq.

With the switch to blk_execute_rq(), we can't be using end_io_data
and end_io, as we use that internally for the wakeup. So I have to
stuff it somewhere.

The obvious option would be to move it to mtip_cmd, but we can't
safely access that prior to having a driver tag assigned, which doesn't
happen until we end up in our ->queue_rq(). So we need to stuff it
somewhere.

Bart Van Assche May 2, 2017, 3:42 p.m. UTC | #3

On Tue, 2017-05-02 at 09:25 -0600, Jens Axboe wrote:
> On 05/02/2017 09:16 AM, Christoph Hellwig wrote:
> > Any reason for the move from ->end_io_data to ->special?  I thought
> > that ->special was something we'd get rid of sooner or later now
> > that we can have additional per-cmd data even for !mq.
> 
> With the switch to blk_execute_rq(), we can't be using end_io_data
> and end_io, as we use that internally for the wakeup. So I have to
> stuff it somewhere.
> 
> The obvious option would be to move it to mtip_cmd, but we can't
> safely access that prior to having a driver tag assigned, which doesn't
> happen until we end up in our ->queue_rq(). So we need to stuff it
> somewhere.

Hello Jens,

Do you think it would be a good idea to allow blk_get_request() callers
to specify that a driver tag has to be allocated even if a scheduler has
been configured? That would make it possible to store completion data in
mtip_cmd for the mtip driver.

Thanks,

Bart.

Jens Axboe May 2, 2017, 4 p.m. UTC | #4

On 05/02/2017 09:42 AM, Bart Van Assche wrote:
> On Tue, 2017-05-02 at 09:25 -0600, Jens Axboe wrote:
>> On 05/02/2017 09:16 AM, Christoph Hellwig wrote:
>>> Any reason for the move from ->end_io_data to ->special?  I thought
>>> that ->special was something we'd get rid of sooner or later now
>>> that we can have additional per-cmd data even for !mq.
>>
>> With the switch to blk_execute_rq(), we can't be using end_io_data
>> and end_io, as we use that internally for the wakeup. So I have to
>> stuff it somewhere.
>>
>> The obvious option would be to move it to mtip_cmd, but we can't
>> safely access that prior to having a driver tag assigned, which doesn't
>> happen until we end up in our ->queue_rq(). So we need to stuff it
>> somewhere.
> 
> Hello Jens,
> 
> Do you think it would be a good idea to allow blk_get_request() callers
> to specify that a driver tag has to be allocated even if a scheduler has
> been configured? That would make it possible to store completion data in
> mtip_cmd for the mtip driver.

Honestly, I think it's fine that we can stuff it _somewhere_ in the
request. I'd rather not expose the driver vs internal tag anymore
than we absolutely have, it's just prone to work on one configuration
and then break if someone turns scheduling on or off.

The real fix here, to avoid using ->special, would be to map the
request before insertion. Then we don't have to pass any info at all.
But that will require touching the callers of the internal command
path for mtip32xx. Left as an exercise to the reader...

Patch

diff --git a/drivers/block/mtip32xx/mtip32xx.c b/drivers/block/mtip32xx/mtip32xx.c
index 96fe6500e941..63a64123b3cb 100644
--- a/drivers/block/mtip32xx/mtip32xx.c
+++ b/drivers/block/mtip32xx/mtip32xx.c
@@ -205,66 +205,12 @@  static struct mtip_cmd *mtip_get_int_command(struct driver_data *dd)
 	return blk_mq_rq_to_pdu(rq);
 }
 
-static void mtip_put_int_command(struct driver_data *dd, struct mtip_cmd *cmd)
-{
-	blk_put_request(blk_mq_rq_from_pdu(cmd));
-}
-
-/*
- * Once we add support for one hctx per mtip group, this will change a bit
- */
-static struct request *mtip_rq_from_tag(struct driver_data *dd,
-					unsigned int tag)
-{
-	struct blk_mq_hw_ctx *hctx = dd->queue->queue_hw_ctx[0];
-
-	return blk_mq_tag_to_rq(hctx->tags, tag);
-}
-
 static struct mtip_cmd *mtip_cmd_from_tag(struct driver_data *dd,
 					  unsigned int tag)
 {
-	struct request *rq = mtip_rq_from_tag(dd, tag);
-
-	return blk_mq_rq_to_pdu(rq);
-}
-
-/*
- * IO completion function.
- *
- * This completion function is called by the driver ISR when a
- * command that was issued by the kernel completes. It first calls the
- * asynchronous completion function which normally calls back into the block
- * layer passing the asynchronous callback data, then unmaps the
- * scatter list associated with the completed command, and finally
- * clears the allocated bit associated with the completed command.
- *
- * @port   Pointer to the port data structure.
- * @tag    Tag of the command.
- * @data   Pointer to driver_data.
- * @status Completion status.
- *
- * return value
- *	None
- */
-static void mtip_async_complete(struct mtip_port *port,
-				int tag, struct mtip_cmd *cmd, int status)
-{
-	struct driver_data *dd = port->dd;
-	struct request *rq;
-
-	if (unlikely(!dd) || unlikely(!port))
-		return;
-
-	if (unlikely(status == PORT_IRQ_TF_ERR)) {
-		dev_warn(&port->dd->pdev->dev,
-			"Command tag %d failed due to TFE\n", tag);
-	}
-
-	rq = mtip_rq_from_tag(dd, tag);
+	struct blk_mq_hw_ctx *hctx = dd->queue->queue_hw_ctx[0];
 
-	cmd->status = status;
-	blk_mq_complete_request(rq);
+	return blk_mq_rq_to_pdu(blk_mq_tag_to_rq(hctx->tags, tag));
 }
 
 /*
@@ -581,38 +527,19 @@  static void print_tags(struct driver_data *dd,
 			"%d command(s) %s: tagmap [%s]", cnt, msg, tagmap);
 }
 
-/*
- * Internal command completion callback function.
- *
- * This function is normally called by the driver ISR when an internal
- * command completed. This function signals the command completion by
- * calling complete().
- *
- * @port   Pointer to the port data structure.
- * @tag    Tag of the command that has completed.
- * @data   Pointer to a completion structure.
- * @status Completion status.
- *
- * return value
- *	None
- */
-static void mtip_completion(struct mtip_port *port,
-			    int tag, struct mtip_cmd *command, int status)
-{
-	struct completion *waiting = command->comp_data;
-	if (unlikely(status == PORT_IRQ_TF_ERR))
-		dev_warn(&port->dd->pdev->dev,
-			"Internal command %d completed with TFE\n", tag);
-
-	command->comp_func = NULL;
-	command->comp_data = NULL;
-	complete(waiting);
-}
-
 static int mtip_read_log_page(struct mtip_port *port, u8 page, u16 *buffer,
 				dma_addr_t buffer_dma, unsigned int sectors);
 static int mtip_get_smart_attr(struct mtip_port *port, unsigned int id,
 						struct smart_attr *attrib);
+
+static void mtip_complete_command(struct mtip_cmd *cmd, int status)
+{
+	struct request *req = blk_mq_rq_from_pdu(cmd);
+
+	cmd->status = status;
+	blk_mq_complete_request(req);
+}
+
 /*
  * Handle an error.
  *
@@ -641,11 +568,7 @@  static void mtip_handle_tfe(struct driver_data *dd)
 	if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags)) {
 		cmd = mtip_cmd_from_tag(dd, MTIP_TAG_INTERNAL);
 		dbg_printk(MTIP_DRV_NAME " TFE for the internal command\n");
-
-		if (cmd->comp_data && cmd->comp_func) {
-			cmd->comp_func(port, MTIP_TAG_INTERNAL,
-					cmd, PORT_IRQ_TF_ERR);
-		}
+		mtip_complete_command(cmd, -EIO);
 		return;
 	}
 
@@ -672,19 +595,9 @@  static void mtip_handle_tfe(struct driver_data *dd)
 				continue;
 
 			cmd = mtip_cmd_from_tag(dd, tag);
-			if (likely(cmd->comp_func)) {
-				set_bit(tag, tagaccum);
-				cmd_cnt++;
-				cmd->comp_func(port, tag, cmd, 0);
-			} else {
-				dev_err(&port->dd->pdev->dev,
-					"Missing completion func for tag %d",
-					tag);
-				if (mtip_check_surprise_removal(dd->pdev)) {
-					/* don't proceed further */
-					return;
-				}
-			}
+			mtip_complete_command(cmd, 0);
+			set_bit(tag, tagaccum);
+			cmd_cnt++;
 		}
 	}
 
@@ -754,10 +667,7 @@  static void mtip_handle_tfe(struct driver_data *dd)
 					tag,
 					fail_reason != NULL ?
 						fail_reason : "unknown");
-					if (cmd->comp_func) {
-						cmd->comp_func(port, tag,
-							cmd, -ENODATA);
-					}
+					mtip_complete_command(cmd, -ENODATA);
 					continue;
 				}
 			}
@@ -780,12 +690,7 @@  static void mtip_handle_tfe(struct driver_data *dd)
 			dev_warn(&port->dd->pdev->dev,
 				"retiring tag %d\n", tag);
 
-			if (cmd->comp_func)
-				cmd->comp_func(port, tag, cmd, PORT_IRQ_TF_ERR);
-			else
-				dev_warn(&port->dd->pdev->dev,
-					"Bad completion for tag %d\n",
-					tag);
+			mtip_complete_command(cmd, -EIO);
 		}
 	}
 	print_tags(dd, "reissued (TFE)", tagaccum, cmd_cnt);
@@ -818,18 +723,7 @@  static inline void mtip_workq_sdbfx(struct mtip_port *port, int group,
 				continue;
 
 			command = mtip_cmd_from_tag(dd, tag);
-			if (likely(command->comp_func))
-				command->comp_func(port, tag, command, 0);
-			else {
-				dev_dbg(&dd->pdev->dev,
-					"Null completion for tag %d",
-					tag);
-
-				if (mtip_check_surprise_removal(
-					dd->pdev)) {
-					return;
-				}
-			}
+			mtip_complete_command(command, 0);
 		}
 		completed >>= 1;
 	}
@@ -850,13 +744,8 @@  static inline void mtip_process_legacy(struct driver_data *dd, u32 port_stat)
 	if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags) &&
 	    (cmd != NULL) && !(readl(port->cmd_issue[MTIP_TAG_INTERNAL])
 		& (1 << MTIP_TAG_INTERNAL))) {
-		if (cmd->comp_func) {
-			cmd->comp_func(port, MTIP_TAG_INTERNAL, cmd, 0);
-			return;
-		}
+		mtip_complete_command(cmd, 0);
 	}
-
-	return;
 }
 
 /*
@@ -864,7 +753,6 @@  static inline void mtip_process_legacy(struct driver_data *dd, u32 port_stat)
  */
 static inline void mtip_process_errors(struct driver_data *dd, u32 port_stat)
 {
-
 	if (unlikely(port_stat & PORT_IRQ_CONNECT)) {
 		dev_warn(&dd->pdev->dev,
 			"Clearing PxSERR.DIAG.x\n");
@@ -991,8 +879,7 @@  static irqreturn_t mtip_irq_handler(int irq, void *instance)
 
 static void mtip_issue_non_ncq_command(struct mtip_port *port, int tag)
 {
-	writel(1 << MTIP_TAG_BIT(tag),
-		port->cmd_issue[MTIP_TAG_INDEX(tag)]);
+	writel(1 << MTIP_TAG_BIT(tag), port->cmd_issue[MTIP_TAG_INDEX(tag)]);
 }
 
 static bool mtip_pause_ncq(struct mtip_port *port,
@@ -1121,7 +1008,6 @@  static int mtip_exec_internal_command(struct mtip_port *port,
 					u32 opts,
 					unsigned long timeout)
 {
-	DECLARE_COMPLETION_ONSTACK(wait);
 	struct mtip_cmd *int_cmd;
 	struct driver_data *dd = port->dd;
 	struct request *rq;
@@ -1146,7 +1032,7 @@  static int mtip_exec_internal_command(struct mtip_port *port,
 		return -EFAULT;
 	}
 	rq = blk_mq_rq_from_pdu(int_cmd);
-	rq->end_io_data = &icmd;
+	rq->special = &icmd;
 
 	set_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
 
@@ -1159,17 +1045,13 @@  static int mtip_exec_internal_command(struct mtip_port *port,
 		/* wait for io to complete if non atomic */
 		if (mtip_quiesce_io(port, MTIP_QUIESCE_IO_TIMEOUT_MS) < 0) {
 			dev_warn(&dd->pdev->dev, "Failed to quiesce IO\n");
-			mtip_put_int_command(dd, int_cmd);
+			blk_mq_free_request(rq);
 			clear_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
 			wake_up_interruptible(&port->svc_wait);
 			return -EBUSY;
 		}
 	}
 
-	/* Set the completion function and data for the command. */
-	int_cmd->comp_data = &wait;
-	int_cmd->comp_func = mtip_completion;
-
 	/* Copy the command to the command table */
 	memcpy(int_cmd->command, fis, fis_len*4);
 
@@ -1177,11 +1059,9 @@  static int mtip_exec_internal_command(struct mtip_port *port,
 	rq->timeout = timeout;
 
 	/* insert request and run queue */
-	blk_execute_rq_nowait(rq->q, NULL, rq, true, NULL);
+	blk_execute_rq(rq->q, NULL, rq, true);
 
-	wait_for_completion(&wait);
 	rv = int_cmd->status;
-
 	if (rv < 0) {
 		if (rv == -ERESTARTSYS) { /* interrupted */
 			dev_err(&dd->pdev->dev,
@@ -1223,7 +1103,7 @@  static int mtip_exec_internal_command(struct mtip_port *port,
 	}
 exec_ic_exit:
 	/* Clear the allocated and active bits for the internal command. */
-	mtip_put_int_command(dd, int_cmd);
+	blk_mq_free_request(rq);
 	clear_bit(MTIP_PF_IC_ACTIVE_BIT, &port->flags);
 	if (rv >= 0 && mtip_pause_ncq(port, fis)) {
 		/* NCQ paused */
@@ -2378,12 +2258,6 @@  static void mtip_hw_submit_io(struct driver_data *dd, struct request *rq,
 				(nents << 16) | 5 | AHCI_CMD_PREFETCH);
 	command->command_header->byte_count = 0;
 
-	/*
-	 * Set the completion function and data for the command
-	 * within this layer.
-	 */
-	command->comp_data = dd;
-	command->comp_func = mtip_async_complete;
 	command->direction = dma_dir;
 
 	/*
@@ -3761,15 +3635,13 @@  static int mtip_issue_reserved_cmd(struct blk_mq_hw_ctx *hctx,
 				   struct request *rq)
 {
 	struct driver_data *dd = hctx->queue->queuedata;
-	struct mtip_int_cmd *icmd = rq->end_io_data;
+	struct mtip_int_cmd *icmd = rq->special;
 	struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq);
 	struct mtip_cmd_sg *command_sg;
 
 	if (mtip_commands_active(dd->port))
 		return BLK_MQ_RQ_QUEUE_BUSY;
 
-	rq->end_io_data = NULL;
-
 	/* Populate the SG list */
 	cmd->command_header->opts =
 		 __force_bit2int cpu_to_le32(icmd->opts | icmd->fis_len);
@@ -3857,9 +3729,7 @@  static enum blk_eh_timer_return mtip_cmd_timeout(struct request *req,
 		struct mtip_cmd *cmd = blk_mq_rq_to_pdu(req);
 
 		cmd->status = -ETIME;
-		if (cmd->comp_func)
-			cmd->comp_func(dd->port, MTIP_TAG_INTERNAL, cmd, -ETIME);
-		goto exit_handler;
+		return BLK_EH_HANDLED;
 	}
 
 	if (test_bit(req->tag, dd->port->cmds_to_issue))
@@ -4087,21 +3957,10 @@  static int mtip_block_initialize(struct driver_data *dd)
 
 static void mtip_no_dev_cleanup(struct request *rq, void *data, bool reserv)
 {
-	struct driver_data *dd = (struct driver_data *)data;
-	struct mtip_cmd *cmd;
-
-	if (likely(!reserv)) {
-		cmd = blk_mq_rq_to_pdu(rq);
-		cmd->status = -ENODEV;
-		blk_mq_complete_request(rq);
-	} else if (test_bit(MTIP_PF_IC_ACTIVE_BIT, &dd->port->flags)) {
+	struct mtip_cmd *cmd = blk_mq_rq_to_pdu(rq);
 
-		cmd = mtip_cmd_from_tag(dd, MTIP_TAG_INTERNAL);
-		cmd->status = -ENODEV;
-		if (cmd->comp_func)
-			cmd->comp_func(dd->port, MTIP_TAG_INTERNAL,
-					cmd, -ENODEV);
-	}
+	cmd->status = -ENODEV;
+	blk_mq_complete_request(rq);
 }
 
 /*
diff --git a/drivers/block/mtip32xx/mtip32xx.h b/drivers/block/mtip32xx/mtip32xx.h
index 57b41528a824..37b8e3e0bb78 100644
--- a/drivers/block/mtip32xx/mtip32xx.h
+++ b/drivers/block/mtip32xx/mtip32xx.h
@@ -333,16 +333,6 @@  struct mtip_cmd {
 
 	dma_addr_t command_dma; /* corresponding physical address */
 
-	void *comp_data; /* data passed to completion function comp_func() */
-	/*
-	 * Completion function called by the ISR upon completion of
-	 * a command.
-	 */
-	void (*comp_func)(struct mtip_port *port,
-				int tag,
-				struct mtip_cmd *cmd,
-				int status);
-
 	int scatter_ents; /* Number of scatter list entries used */
 
 	int unaligned; /* command is unaligned on 4k boundary */