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

[Bart Van Assche]

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.

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

[Christoph Hellwig]

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.

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

[Jens Axboe]

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 
>>
>> 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.

 mtip32xx.c |  194 +
 mtip32xx.h |   10 ---
 2 files changed, 29 insertions(+), 175 deletions(-)

diff --git a/drivers/block/mtip32xx/mtip32xx.c 
b/drivers/block/mtip32xx/mtip32xx.c
index 96fe6500e941..60b62a72bc22 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.
- * @tagTag 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(>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,18 @@ 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.
- * @tagTag 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(>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, 

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

[Jens Axboe]

On 05/02/2017 01:28 AM, Christoph Hellwig wrote:
> Looks fine for now:
> 
> Reviewed-by: Christoph Hellwig 
> 
> 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.

-- 
Jens Axboe

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

[Christoph Hellwig]

Looks fine for now:

Reviewed-by: Christoph Hellwig 

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.