Re: [PATCH V14 13/24] mmc: block: Add blk-mq support

[Ulf Hansson]

On 27 November 2017 at 11:20, Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> On 24/11/17 12:12, Ulf Hansson wrote:
>> [...]
>>
>>> +/* Single sector read during recovery */
>>> +static void mmc_blk_ss_read(struct mmc_queue *mq, struct request *req)
>>
>> Nitpick: I think mmc_blk_read_single() would be better as it is a more
>> clear name. Would you mind changing it?
>>
>>> +{
>>> +       struct mmc_queue_req *mqrq = req_to_mmc_queue_req(req);
>>> +       blk_status_t status;
>>> +
>>> +       while (1) {
>>> +               mmc_blk_rw_rq_prep(mqrq, mq->card, 1, mq);
>>> +
>>> +               mmc_wait_for_req(mq->card->host, &mqrq->brq.mrq);
>>> +
>>> +               /*
>>> +                * Not expecting command errors, so just give up in that case.
>>> +                * If there are retries remaining, the request will get
>>> +                * requeued.
>>> +                */
>>> +               if (mqrq->brq.cmd.error)
>>> +                       return;
>>
>> What happens here if the reason to the error is because the card was removed?
>
> Assuming the rescan is waiting for the host claim, the next read / write
> request will end up calling mmc_detect_card_removed() in the recovery.
> After that all following requests will error immediately because
> mmc_mq_queue_rq() calls mmc_card_removed().

Yep, that seems reasonable. I have also tested this, so it seems to
work as expected and similar as before.

>
>>
>> I guess next time __blk_err_check() is called from the
>> mmc_blk_mq_rw_recovery(), this will be detected and managed?
>>
>>> +
>>> +               if (blk_rq_bytes(req) <= 512)
>>
>> Shouldn't you check "if (blk_rq_bytes(req) <  512)"? How would you
>> otherwise read the last 512 bytes block?
>
> At this point we have read the last sector but not updated the request, so
> the number of bytes left should be 512.  The reason we don't update the
> request is so that the logic in mmc_blk_mq_complete_rq() will work.  I will
> add a comment.

Not sure I get that, but I assume the comment will help me understand. :-)

>
>>
>>> +                       break;
>>> +
>>> +               status = mqrq->brq.data.error ? BLK_STS_IOERR : BLK_STS_OK;
>>> +
>>> +               blk_update_request(req, status, 512);
>>
>> Shouldn't we actually bail out, unless the error is a data ECC error?
>> On the other hand, I guess if it a more severe error, cmd.error will
>> anyway be set above!?
>>
>> One more question, if there is a data error, we may want to try to
>> recover by sending a stop command? How do we manage that?
>
> I was thinking a single-block read would not need a stop.  I will think
> some more about error handling here.

Great!

Anyway, you may be right -  and perhaps it may not be worth adding
error handling, especially if it complicates the code a lot.

[...]

>>> +static void mmc_blk_mq_acct_req_done(struct mmc_queue *mq, struct request *req)
>>
>> Nitpick: Can we please try to find a better name for this function. I
>> don't think "acct" is good abbreviation because, to me, it's not
>> self-explaining.
>
> What about mmc_blk_mq_decrement_in_flight() ?

Looks good, or perhaps even: mmc_blk_mq_dec_in_flight().

>
>>
>>> +{
>>> +       struct request_queue *q = req->q;
>>> +       unsigned long flags;
>>> +       bool put_card;
>>> +
>>> +       spin_lock_irqsave(q->queue_lock, flags);
>>> +
>>> +       mq->in_flight[mmc_issue_type(mq, req)] -= 1;
>>> +
>>> +       put_card = (mmc_tot_in_flight(mq) == 0);
>>> +
>>> +       spin_unlock_irqrestore(q->queue_lock, flags);
>>> +
>>> +       if (put_card)
>>> +               mmc_put_card(mq->card, &mq->ctx);
>>
>> I have tried to convince myself that the protection of calling
>> mmc_get|put_card() is safe, but I am not sure.
>>
>> I am wondering whether there could be races for mmc_get|put_card().
>> Please see some more related comments below.
>
> mmc_put_card() is safe and necessary if we have seen mmc_tot_in_flight(mq)
> == 0.  When the next request arrives it will have to do a mmc_get_card()
> because it is changing the number of requests in flight from 0 to 1.  It
> doesn't matter if that mmc_get_card() comes before or after or during this
> mmc_put_card().
>
>>
>> [...]

[...]

>>
>> Anyway, then if using a queue_depth of 64, how will you make sure that
>> you not end up having > 1 requests being prepared at the same time
>> (not counting the one that may be in transfer)?
>
> We are currently single-threaded since every request goes through
> hctx->run_work when BLK_MQ_F_BLOCKING and nr_hw_queues == 1.  It might be
> worth adding a mutex to ensure that never changes.
>
> This point also answers some of the questions below, since there can be no
> parallel dispatches.

Yeah, it clearly does. Thanks!

>>> +
>>> +enum mmc_issued mmc_blk_mq_issue_rq(struct mmc_queue *mq, struct request *req)
>>> +{
>>> +       struct mmc_blk_data *md = mq->blkdata;
>>> +       struct mmc_card *card = md->queue.card;
>>> +       struct mmc_host *host = card->host;
>>> +       int ret;
>>> +
>>> +       ret = mmc_blk_part_switch(card, md->part_type);
>>
>> What if there is an ongoing request, shouldn't you wait for that to
>> complete before switching partition?
>
> Two requests on the same queue cannot be on different partitions because we
> have a different queue (and block device) for each partition.

That's not true for RPMB anymore I am afraid.

RPMB shares the same queue as for the main eMMC partition, which is
because we strive towards fair I/O scheduling across the hole device.

>
>>
>>> +       if (ret)
>>> +               return MMC_REQ_FAILED_TO_START;
>>> +
>>> +       switch (mmc_issue_type(mq, req)) {
>>> +       case MMC_ISSUE_SYNC:
>>> +               ret = mmc_blk_wait_for_idle(mq, host);
>>> +               if (ret)
>>> +                       return MMC_REQ_BUSY;
>>
>> Wouldn't it be possible that yet a new SYNC request becomes queued in
>> parallel with this current one. Then, when reaching this point, how do
>> you make sure that new request waits for the current "SYNC" request?
>
> As mentioned above, there are no parallel dispatches.
>
>>
>> I mean is the above mmc_blk_wait_for_idle(), really sufficient to deal
>> with synchronization?
>
> So long as there are no parallel dispatches.
>
>>
>> I guess we could use mmc_claim_host(no-ctx) in some clever way to deal
>> with this, or perhaps there is a better option?
>
> We are relying on there being no parallel dispatches.  That is the case now,
> but if it weren't we could use a mutex in mmc_mq_queue_rq().
>

Yeah, but then leave that until needed.

>>
>> BTW, I guess the problem is also present if there is SYNC request
>> ongoing and then is a new ASYNC request coming in. Is the ASYNC
>> request really waiting for the SYNC request to finish?
>
> With no parallel dispatches, the SYNC request runs to completion before
> another request can be dispatched.

Yes, I get it now. Thanks for clarifying this!

[...]

>>> +static blk_status_t mmc_mq_queue_rq(struct blk_mq_hw_ctx *hctx,
>>> +                                   const struct blk_mq_queue_data *bd)
>>> +{
>>> +       struct request *req = bd->rq;
>>> +       struct request_queue *q = req->q;
>>> +       struct mmc_queue *mq = q->queuedata;
>>> +       struct mmc_card *card = mq->card;
>>> +       enum mmc_issue_type issue_type;
>>> +       enum mmc_issued issued;
>>> +       bool get_card;
>>> +       int ret;
>>> +
>>> +       if (mmc_card_removed(mq->card)) {
>>> +               req->rq_flags |= RQF_QUIET;
>>> +               return BLK_STS_IOERR;
>>> +       }
>>> +
>>> +       issue_type = mmc_issue_type(mq, req);
>>> +
>>> +       spin_lock_irq(q->queue_lock);
>>> +
>>> +       switch (issue_type) {
>>> +       case MMC_ISSUE_ASYNC:
>>> +               break;
>>> +       default:
>>> +               /*
>>> +                * Timeouts are handled by mmc core, and we don't have a host
>>> +                * API to abort requests, so we can't handle the timeout anyway.
>>> +                * However, when the timeout happens, blk_mq_complete_request()
>>> +                * no longer works (to stop the request disappearing under us).
>>> +                * To avoid racing with that, set a large timeout.
>>> +                */
>>> +               req->timeout = 600 * HZ;
>>> +               break;
>>> +       }
>>> +
>>> +       mq->in_flight[issue_type] += 1;
>>> +       get_card = (mmc_tot_in_flight(mq) == 1);
>>> +
>>> +       spin_unlock_irq(q->queue_lock);
>>> +
>>> +       if (!(req->rq_flags & RQF_DONTPREP)) {
>>> +               req_to_mmc_queue_req(req)->retries = 0;
>>> +               req->rq_flags |= RQF_DONTPREP;
>>> +       }
>>> +
>>> +       if (get_card)
>>
>> Coming back to the get_card() thingy, which I wonder if it's fragile.
>>
>> A request that finds get_card == true here, doesn't necessarily have
>> to reach to this point first (the task may be preempted), in case
>> there is another request being queued in parallel (or that can't
>> happen?).
>>
>> That could then lead to that the following steps become executed for
>> the other requests, prior anybody calling mmc_get_card().
>
> You are right, this logic does not support parallel dispatches.
>

This do raises a question, don't you think it would be beneficial,
especially for CQE to allow parallel dispatches?

I am not saying we should change this at this point, just that we may
consider changing this for future improvements.

[...]

Kind regards
Uffe