On Tue, May 16, 2017 at 11:02 AM, Ulf Hansson [off-list ref] wrote:

On 10 May 2017 at 10:24, Linus Walleij [off-list ref] wrote:

quoted

@@ -1662,7 +1655,7 @@ static void mmc_blk_rw_try_restart(struct mmc_queue *mq, struct request *req,
        if (mmc_card_removed(mq->card)) {
                req->rq_flags |= RQF_QUIET;
                blk_end_request_all(req, -EIO);
-               mmc_queue_req_free(mq, mqrq);
+               mq->qcnt--; /* FIXME: just set to 0? */

As mentioned below, perhaps this FIXME is fine to add. As I assume you
soon intend to take care of it, right?

Yes that goes away with my MQ patches (not yet rebased)
by stopping to try to look when the queue is empty and just
issue requests asynchronously. I just wanted to point this out,
that counter is kind of fragile and scary to me.

quoted

-       for (i = 0; i < qdepth; i++) {
-               mqrq[i].sg = mmc_alloc_sg(max_segs);
-               if (!mqrq[i].sg)
+       /* FIXME: use req_to_mq_rq() everywhere this is dereferenced */

Why not do that right now, instead of adding a FIXME comment?

This comment is wrong, just a development artifact I will just delete it.

quoted

        mq->card = card;
-       mq->queue = blk_init_queue(mmc_request_fn, lock);
+       mq->queue = blk_alloc_queue_node(GFP_KERNEL, NUMA_NO_NODE);

Seems like we should use blk_alloc_queue() instead, as it calls
blk_alloc_queue_node(gfp_mask, NUMA_NO_NODE) for us.

OK

quoted

+static inline struct mmc_queue_req *req_to_mq_rq(struct request *rq)

To be more consistent with existing function names, perhaps rename this to:
req_to_mmc_queue_req()

quoted

+{
+       return blk_mq_rq_to_pdu(rq);
+}
+

[...]

quoted

 struct mmc_queue {

@@ -45,14 +50,15 @@ struct mmc_queue {
        bool                    asleep;
        struct mmc_blk_data     *blkdata;
        struct request_queue    *queue;
-       struct mmc_queue_req    *mqrq;
-       int                     qdepth;
+       /*
+        * FIXME: this counter is not a very reliable way of keeping
+        * track of how many requests that are ongoing. Switch to just
+        * letting the block core keep track of requests and per-request
+        * associated mmc_queue_req data.
+        */
        int                     qcnt;

I am not very fond of FIXME comments, however perhaps this one really
deserves to be a FIXME because you intend to fix this asap, right?

Same as the first comment. It is fragile and I don't like it,
with asynchronous issueing in MQ this goes away.

Yours,
Linus Walleij