Outstanding MQ questions from MMC

Outstanding MQ questions from MMC

[Linus Walleij]

Hi folks,

I earlier left some unanswered questions in my MMC to MQ conversion series
but I figured it is better if I collect them and ask the blk-mq
maintainers directly
how to deal with the following situations that occur in the MMC block layer:


1. The current MMC code locks the host when the first request comes in
from blk_fetch_request() and unlocks it when blk_fetch_request() returns
NULL twice in a row. Then the polling thread terminated and is not restarted
until we get called by the mmc_request_fn.

Host locking means that we will not send other commands to the MMC
card from i.e. userspace, which sometimes can send spurious stuff orthogonal
to the block layer. If the block layer has locked the host, userspace
has to wait
and vice versa. It is not a common contention point but it still happens.

In MQ, I have simply locked the host on the first request and then I never
release it. Clearly this does not work. I am uncertain on how to handle this
and whether MQ has a way to tell us that the queue is empty so we may release
the host. I toyed with the idea to just set up a timer, but a "queue
empty" callback
from the block layer is what would be ideal.


2. When MMC cards are ejected a serious error condition occurs. So for this
reason we spool out the queue with

req->rq_flags |= RQF_QUIET;
blk_end_request_all(req, -EIO);

This will shut up a huge amount of console errors for example.
I have no clue on how to manage this with MQ. I am currently using

blk_mq_complete_request(mq_rq->req, -EIO);

and nothing else, and it will hit all requests for the ejected card coming
in from this point. Is this the right solution? Or is there some medium
eject handling I'm not aware of inside the MQ layer? It seems like something
that can happen on pluggable harddrives and CDROMS and what not.


3. Sometimes a read or write gets partially completed. Say we read 12 out
of 15 sectors or somthing like that. I have no idea how often this occurs in
practice. With the old block layer we did this:

blk_end_request(req, 0, bytes_xfered);

It seems MQ cannot handle partially completed transfers. I currently do this:

blk_mq_requeue_request(req, false);

I do not feel that is the right thing to do either, and would like
recommendations
on how to proceed with this.


Yours,
Linus Walleij

Re: Outstanding MQ questions from MMC

[Arnd Bergmann]

On Wed, Mar 29, 2017 at 5:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Hi folks,
>
> I earlier left some unanswered questions in my MMC to MQ conversion series
> but I figured it is better if I collect them and ask the blk-mq
> maintainers directly
> how to deal with the following situations that occur in the MMC block layer:
>
>
> 1. The current MMC code locks the host when the first request comes in
> from blk_fetch_request() and unlocks it when blk_fetch_request() returns
> NULL twice in a row. Then the polling thread terminated and is not restarted
> until we get called by the mmc_request_fn.
>
> Host locking means that we will not send other commands to the MMC
> card from i.e. userspace, which sometimes can send spurious stuff orthogonal
> to the block layer. If the block layer has locked the host, userspace
> has to wait
> and vice versa. It is not a common contention point but it still happens.
>
> In MQ, I have simply locked the host on the first request and then I never
> release it. Clearly this does not work. I am uncertain on how to handle this
> and whether MQ has a way to tell us that the queue is empty so we may release
> the host. I toyed with the idea to just set up a timer, but a "queue
> empty" callback
> from the block layer is what would be ideal.

Would it be possible to change the userspace code to go through
the block layer instead and queue a request there, to avoid having
to lock the card at all?

       Arnd

Re: Outstanding MQ questions from MMC

[Ulf Hansson]

On 30 March 2017 at 14:42, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Mar 29, 2017 at 5:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> Hi folks,
>>
>> I earlier left some unanswered questions in my MMC to MQ conversion series
>> but I figured it is better if I collect them and ask the blk-mq
>> maintainers directly
>> how to deal with the following situations that occur in the MMC block layer:
>>
>>
>> 1. The current MMC code locks the host when the first request comes in
>> from blk_fetch_request() and unlocks it when blk_fetch_request() returns
>> NULL twice in a row. Then the polling thread terminated and is not restarted
>> until we get called by the mmc_request_fn.
>>
>> Host locking means that we will not send other commands to the MMC
>> card from i.e. userspace, which sometimes can send spurious stuff orthogonal
>> to the block layer. If the block layer has locked the host, userspace
>> has to wait
>> and vice versa. It is not a common contention point but it still happens.
>>
>> In MQ, I have simply locked the host on the first request and then I never
>> release it. Clearly this does not work. I am uncertain on how to handle this
>> and whether MQ has a way to tell us that the queue is empty so we may release
>> the host. I toyed with the idea to just set up a timer, but a "queue
>> empty" callback
>> from the block layer is what would be ideal.
>
> Would it be possible to change the userspace code to go through
> the block layer instead and queue a request there, to avoid having
> to lock the card at all?

That would be good from an I/O scheduling point of view, as it would
avoid one side being able to starve the other.

However, we would still need a lock, as we also have card detect work
queue, which also needs to claim the host when it polls for removable
cards.

Kind regards
Uffe

Re: Outstanding MQ questions from MMC

[Arnd Bergmann]

On Thu, Mar 30, 2017 at 6:39 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> On 30 March 2017 at 14:42, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Wed, Mar 29, 2017 at 5:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:

>>> In MQ, I have simply locked the host on the first request and then I never
>>> release it. Clearly this does not work. I am uncertain on how to handle this
>>> and whether MQ has a way to tell us that the queue is empty so we may release
>>> the host. I toyed with the idea to just set up a timer, but a "queue
>>> empty" callback
>>> from the block layer is what would be ideal.
>>
>> Would it be possible to change the userspace code to go through
>> the block layer instead and queue a request there, to avoid having
>> to lock the card at all?
>
> That would be good from an I/O scheduling point of view, as it would
> avoid one side being able to starve the other.
>
> However, we would still need a lock, as we also have card detect work
> queue, which also needs to claim the host when it polls for removable
> cards.

Hmm, In theory the card-detect work queue should not be active
at the same time as any I/O, but I see you point. Could the card-detect
wq perhaps also use the blk queue for sending a special request that
triggers the detection logic?

Alternatively, I had this idea that we could translate blk requests into
mmc commands and then have a (short fixed length) set of outstanding
mmc commands in the device that always get done in order. The card
detect and the user space I/O would then directly put mmc commands
onto the command queue, as would the blk-mq scheduler. You
still need a lock to access that command queue, but the mmc host
would just always pick the next command off the list when one
command completes.
This also lets you integrate packed commands: if the next outstanding
command is the same type as the request coming in from blk-mq,
you can merge it into a single mmc command to be submitted
together, otherwise it gets deferred.

      Arnd

Re: Outstanding MQ questions from MMC

[Linus Walleij]

On Fri, Mar 31, 2017 at 10:43 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Thu, Mar 30, 2017 at 6:39 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> On 30 March 2017 at 14:42, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Wed, Mar 29, 2017 at 5:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>
>>>> In MQ, I have simply locked the host on the first request and then I never
>>>> release it. Clearly this does not work. I am uncertain on how to handle this
>>>> and whether MQ has a way to tell us that the queue is empty so we may release
>>>> the host. I toyed with the idea to just set up a timer, but a "queue
>>>> empty" callback
>>>> from the block layer is what would be ideal.
>>>
>>> Would it be possible to change the userspace code to go through
>>> the block layer instead and queue a request there, to avoid having
>>> to lock the card at all?
>>
>> That would be good from an I/O scheduling point of view, as it would
>> avoid one side being able to starve the other.
>>
>> However, we would still need a lock, as we also have card detect work
>> queue, which also needs to claim the host when it polls for removable
>> cards.
>
> Hmm, In theory the card-detect work queue should not be active
> at the same time as any I/O, but I see you point. Could the card-detect
> wq perhaps also use the blk queue for sending a special request that
> triggers the detection logic?
>
> Alternatively, I had this idea that we could translate blk requests into
> mmc commands and then have a (short fixed length) set of outstanding
> mmc commands in the device that always get done in order. The card
> detect and the user space I/O would then directly put mmc commands
> onto the command queue, as would the blk-mq scheduler. You
> still need a lock to access that command queue, but the mmc host
> would just always pick the next command off the list when one
> command completes.

I looked into this.

The block layer queue can wrap and handle custom device commands
using REQ_OP_DRV_IN/OUT, and that seems to be the best way
to play with the block layer IMO.

The card detect work is a special case because it is also used by
SDIO which does not use the block layer. But that could maybe be
solved by a separate host lock just for the SDIO case, letting
devices accessed as block devices use the method of inserting
custom commands.

I looked at how e.g. IDE and SCSI does this, drivers/ide/ide-ioctls.c
looks like this nowadays:

static int generic_drive_reset(ide_drive_t *drive)
{
        struct request *rq;
        int ret = 0;

        rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
        scsi_req_init(rq);
        ide_req(rq)->type = ATA_PRIV_MISC;
        scsi_req(rq)->cmd_len = 1;
        scsi_req(rq)->cmd[0] = REQ_DRIVE_RESET;
        if (blk_execute_rq(drive->queue, NULL, rq, 1))
                ret = rq->errors;
        blk_put_request(rq);
        return ret;
}

So it creates a custom REQ_OP_DRV_IN request, then scsi_req_init()
sets up the special command, in this case
ATA_PRIV_MISC/REQ_DRIVE_RESET and toss this into the block
queue like everything else.

We could do the same, especially the RPMB operations should
probably have been done like this from the beginning. But due to
historical factors they were not.

It is a bit hairy and the whole thing is in a bit of flux because Christoph
is heavily refactoring this and cleaning up the old block devices as
we speak (I bet) so it is a bit hard to do the right thing.

I easily get confused here ... for example there is custom
per-request data access by this simple:

scsi_req_init(rq)

which does

struct scsi_request *req = scsi_req(rq);

which does

static inline struct scsi_request *scsi_req(struct request *rq)
{
        return blk_mq_rq_to_pdu(rq);
}

Oohps blk_mq_* namespace? You would assume this means that
you have to use blk-mq? Nah, I think not, because all it does is:

static inline void *blk_mq_rq_to_pdu(struct request *rq)
{
        return rq + 1;
}

So while we have to #include <linux/blk-mq.h> this is one of these
mixed semantics that just give you a pointer to something behind
the request, a method that is simple and natural in blk-mq but which
is (I guess) set up by some other mechanism in the !mq case,
albeit access by this inline.

And I have to do this with the old block layer to get to a point
where we can start using blk-mq, sigh.

The border between blk and blk-mq is a bit blurry right now.

With blk-mq I do this:

mq->tag_set.cmd_size = sizeof(foo_cmd);
blk_mq_alloc_tag_set(...)

To do this with the old blk layer I may need some help to figure
out how to set up per-request additional data in a way that works
with the old layer.

scsi_lib.c scsi_alloc_queue() does this:

q = blk_alloc_queue_node(GFP_KERNEL, NUMA_NO_NODE);
if (!q)
      return NULL;
q->cmd_size = sizeof(foo_cmd);

And this means there will be sizeof(foo_cmd) after the request
that can be dereferenced by blk_mq_rq_to_pdu(rq);...

Yeah I'll try it.

Just trying to give a picture of why it's a bit in flux here.
Or documenting it for myself :D

> This also lets you integrate packed commands: if the next outstanding
> command is the same type as the request coming in from blk-mq,
> you can merge it into a single mmc command to be submitted
> together, otherwise it gets deferred.

Essentially the heavy lifting that needs to happen is:

1. Start allocating per-request PDU (extra data) in the MMC case
   this will then be struct mmc_queue_req request items.

2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
   things and funnel them into the block scheduler
   using REQ_OP_DRV_IN/OUT requests.

3. Turn the card detect into an mmc_queue_req as well

4. We can kill the big MMC host lock for block devices and
   split off an SDIO-only host lock.

I'm onto it ... I guess.

Yours,
Linus Walleij

RE: Outstanding MQ questions from MMC

[Avri Altman]

DQoNCj4gDQo+IDIuIFR1cm4gUlBNQiBhbmQgb3RoZXIgaW9jdGwoKSBNTUMgb3BlcmF0aW9ucyBp
bnRvIG1tY19xdWV1ZV9yZXENCj4gICAgdGhpbmdzIGFuZCBmdW5uZWwgdGhlbSBpbnRvIHRoZSBi
bG9jayBzY2hlZHVsZXINCj4gICAgdXNpbmcgUkVRX09QX0RSVl9JTi9PVVQgcmVxdWVzdHMuDQo+
IA0KDQpBY2Nlc3NpbmcgdGhlIFJQTUIgaXMgZG9uZSB2aWEgYSBzdHJhbmdlIHByb3RvY29sLCBp
biB3aGljaCBlYWNoIGFjY2VzcyBpcyBjb21wcmlzZWQgb2Ygc2V2ZXJhbCByZXF1ZXN0cy4NCkZv
ciBleGFtcGxlLCB3cml0aW5nIHRvIHRoZSBSUE1CIHdpbGwgcmVxdWlyZSBzZW5kaW5nIDUgZGlm
ZmVyZW50IHJlcXVlc3RzOiANCjIgcmVxdWVzdHMgdG8gcmVhZCB0aGUgd3JpdGUgY291bnRlciwg
YW5kIHRoZW4gMyBtb3JlIHJlcXVlc3RzIGZvciB0aGUgd3JpdGUgb3BlcmF0aW9uIGl0c2VsZi4N
Cg0KT25jZSB0aGUgc2VxdWVuY2UgaGFzIHN0YXJ0ZWQsIGl0IHNob3VsZCBub3QgZ2V0IGludGVy
ZmVyZWQgYnkgb3RoZXIgcmVxdWVzdHMsIG9yIHRoZSBvcGVyYXRpb24gd2lsbCBmYWlsLg0KDQpT
bywgaWYgeW91IGFyZSBsb29raW5nIHRvIGVsaW1pbmF0ZSB0aGUgaG9zdCBsb2NrLCBhbmQgY291
bnQgbWVyZWx5IG9uIHRoZSBibGsgcXVldWUgdG8gcmVndWxhdGUgIGFjY2VzcyB0byB0aGUgZGV2
aWNlLA0KWW91J2xsIGJlIG5lZWRpbmcgc29tZSBiYXJyaWVyIG1lY2hhbmlzbSB0aGF0IHdpbGwg
YXNzdXJlIHRoYXQgYSBzZXF1ZW5jZSBvZiByZXF1ZXN0cyB3aWxsIHRha2UgcGxhY2UgYXMgYW4g
YXRvbWljIHVuaXQuDQoNCkJ1dCB0aGVuIGFnYWluLCBpc24ndCBpdCBjb250cmFkaWN0cyB0aGUg
dmVyeSBpZGVhIG9mIGEgbXVsdGktcXVldWU/DQoNCkNoZWVycywNCkF2cmkNCg==

RE: Outstanding MQ questions from MMC

[Avri Altman]

> 
> 2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
>    things and funnel them into the block scheduler
>    using REQ_OP_DRV_IN/OUT requests.
> 

Accessing the RPMB is done via a strange protocol, in which each access is comprised of several requests.
For example, writing to the RPMB will require sending 5 different requests: 
2 requests to read the write counter, and then 3 more requests for the write operation itself.

Once the sequence has started, it should not get interfered by other requests, or the operation will fail.

So, if you are looking to eliminate the host lock, and count merely on the blk queue to regulate  access to the device,
You'll be needing some barrier mechanism that will assure that a sequence of requests will take place as an atomic unit.

But then again, isn't it contradicts the very idea of a multi-queue?

Cheers,
Avri

Re: Outstanding MQ questions from MMC

[Linus Walleij]

On Fri, Apr 14, 2017 at 8:41 PM, Avri Altman <Avri.Altman@sandisk.com> wrote:
> [Me]
>> 2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
>>    things and funnel them into the block scheduler
>>    using REQ_OP_DRV_IN/OUT requests.
>>
>
> Accessing the RPMB is done via a strange protocol, in which each access is comprised of several requests.
> For example, writing to the RPMB will require sending 5 different requests:
> 2 requests to read the write counter, and then 3 more requests for the write operation itself.
>
> Once the sequence has started, it should not get interfered by other requests, or the operation will fail.

So I guess currently something takes a host lock and then performs the
5 requests.

Thus we need to send a single custom request containing a list of 5
things to do, and return after that.

Or do you mean that we return to userspace inbetween these different
requests and the sequencing is done in userspace?

I hope not because that sounds fragile, like userspace could crash and
leave the host lock dangling :/

Yours,
Linus Walleij

RE: Outstanding MQ questions from MMC

[Avri Altman]

WW91IGNhbiBzZWUgaG93IGl0J3MgZG9uZSBpbiBtbWNfYmxrX2lvY3RsX3JwbWJfY21kKCkuDQoN
ClRoZSBSUE1CIHByb3RvY29sIGRlZmluZXMgNiB0eXBlcyBvZiBhY2Nlc3NlczoNCkFjY2Vzc2Vz
IHRoYXQgcGVyZm9ybXMgcmVhZCBvcGVyYXRpb24gKHJlYWQgY291bnRlciwgcmVhZCBkYXRhLCBh
bmQgcmVhZCBjb25maWd1cmF0aW9uKSAtIHJlcXVpcmVzIHNlbmRpbmcgMiByZXF1ZXN0cy4gDQpB
Y2Nlc3NlcyB0aGF0IHBlcmZvcm1zIHdyaXRlIG9wZXJhdGlvbiAocHJvZ3JhbSBrZXksIHdyaXRl
IGRhdGEsIHdyaXRlIGNvbmZpZ3VyYXRpb24pIC0gcmVxdWlyZXMgc2VuZGluZyAzIHJlcXVlc3Rz
LA0KQnV0IHlvdSBtdXN0IGRvIHJlYWQgY291bnRlciBiZWZvcmVoYW5kIChhY2NlcHQgZnJvbSBw
cm9ncmFtIGtleSksIGhlbmNlIHRoZSA1IGRpZmZlcmVudCByZXF1ZXN0cy4NCg0KVGhlIHN0YW5k
YXJkIGRvZXMgbm90IGRlZmluZSBhICJzcGVjaWFsIiByZXF1ZXN0IHRoYXQgZG9lcyBpdCBhbGwg
aW4gb25jZSwgDQpidXQgZXhwZWN0cyBhIHByZS1kZWZpbmUgc2VyaWVzIG9mIGNtZDE4ICYgY21k
MjUgZm9yIGVhY2ggYWNjZXNzIHR5cGUsIA0KaW4gd2hpY2ggdGhlIHBheWxvYWQgYXJlIDUxMiBi
eXRlcyBmcmFtZXMgaW4gYSBwcmUtZGVmaW5lZCBzdHJ1Y3R1cmUuDQogIA0KQ2hlZXJzLA0KQXZy
aQ0KDQo+IC0tLS0tT3JpZ2luYWwgTWVzc2FnZS0tLS0tDQo+IEZyb206IExpbnVzIFdhbGxlaWog
W21haWx0bzpsaW51cy53YWxsZWlqQGxpbmFyby5vcmddDQo+IFNlbnQ6IFNhdHVyZGF5LCBBcHJp
bCAxNSwgMjAxNyA5OjM1IFBNDQo+IFRvOiBBdnJpIEFsdG1hbiA8QXZyaS5BbHRtYW5Ac2FuZGlz
ay5jb20+DQo+IENjOiBBcm5kIEJlcmdtYW5uIDxhcm5kQGFybmRiLmRlPjsgVWxmIEhhbnNzb24N
Cj4gPHVsZi5oYW5zc29uQGxpbmFyby5vcmc+OyBsaW51eC1tbWNAdmdlci5rZXJuZWwub3JnOyBs
aW51eC0NCj4gYmxvY2tAdmdlci5rZXJuZWwub3JnOyBKZW5zIEF4Ym9lIDxheGJvZUBrZXJuZWwu
ZGs+OyBDaHJpc3RvcGggSGVsbHdpZw0KPiA8aGNoQGxzdC5kZT47IEFkcmlhbiBIdW50ZXIgPGFk
cmlhbi5odW50ZXJAaW50ZWwuY29tPjsgUGFvbG8gVmFsZW50ZQ0KPiA8cGFvbG8udmFsZW50ZUBs
aW5hcm8ub3JnPg0KPiBTdWJqZWN0OiBSZTogT3V0c3RhbmRpbmcgTVEgcXVlc3Rpb25zIGZyb20g
TU1DDQo+IA0KPiBPbiBGcmksIEFwciAxNCwgMjAxNyBhdCA4OjQxIFBNLCBBdnJpIEFsdG1hbiA8
QXZyaS5BbHRtYW5Ac2FuZGlzay5jb20+DQo+IHdyb3RlOg0KPiA+IFtNZV0NCj4gPj4gMi4gVHVy
biBSUE1CIGFuZCBvdGhlciBpb2N0bCgpIE1NQyBvcGVyYXRpb25zIGludG8gbW1jX3F1ZXVlX3Jl
cQ0KPiA+PiAgICB0aGluZ3MgYW5kIGZ1bm5lbCB0aGVtIGludG8gdGhlIGJsb2NrIHNjaGVkdWxl
cg0KPiA+PiAgICB1c2luZyBSRVFfT1BfRFJWX0lOL09VVCByZXF1ZXN0cy4NCj4gPj4NCj4gPg0K
PiA+IEFjY2Vzc2luZyB0aGUgUlBNQiBpcyBkb25lIHZpYSBhIHN0cmFuZ2UgcHJvdG9jb2wsIGlu
IHdoaWNoIGVhY2ggYWNjZXNzIGlzDQo+IGNvbXByaXNlZCBvZiBzZXZlcmFsIHJlcXVlc3RzLg0K
PiA+IEZvciBleGFtcGxlLCB3cml0aW5nIHRvIHRoZSBSUE1CIHdpbGwgcmVxdWlyZSBzZW5kaW5n
IDUgZGlmZmVyZW50IHJlcXVlc3RzOg0KPiA+IDIgcmVxdWVzdHMgdG8gcmVhZCB0aGUgd3JpdGUg
Y291bnRlciwgYW5kIHRoZW4gMyBtb3JlIHJlcXVlc3RzIGZvciB0aGUNCj4gd3JpdGUgb3BlcmF0
aW9uIGl0c2VsZi4NCj4gPg0KPiA+IE9uY2UgdGhlIHNlcXVlbmNlIGhhcyBzdGFydGVkLCBpdCBz
aG91bGQgbm90IGdldCBpbnRlcmZlcmVkIGJ5IG90aGVyDQo+IHJlcXVlc3RzLCBvciB0aGUgb3Bl
cmF0aW9uIHdpbGwgZmFpbC4NCj4gDQo+IFNvIEkgZ3Vlc3MgY3VycmVudGx5IHNvbWV0aGluZyB0
YWtlcyBhIGhvc3QgbG9jayBhbmQgdGhlbiBwZXJmb3JtcyB0aGUNCj4gNSByZXF1ZXN0cy4NCj4g
DQo+IFRodXMgd2UgbmVlZCB0byBzZW5kIGEgc2luZ2xlIGN1c3RvbSByZXF1ZXN0IGNvbnRhaW5p
bmcgYSBsaXN0IG9mIDUgdGhpbmdzIHRvDQo+IGRvLCBhbmQgcmV0dXJuIGFmdGVyIHRoYXQuDQo+
IA0KPiBPciBkbyB5b3UgbWVhbiB0aGF0IHdlIHJldHVybiB0byB1c2Vyc3BhY2UgaW5iZXR3ZWVu
IHRoZXNlIGRpZmZlcmVudA0KPiByZXF1ZXN0cyBhbmQgdGhlIHNlcXVlbmNpbmcgaXMgZG9uZSBp
biB1c2Vyc3BhY2U/DQo+IA0KPiBJIGhvcGUgbm90IGJlY2F1c2UgdGhhdCBzb3VuZHMgZnJhZ2ls
ZSwgbGlrZSB1c2Vyc3BhY2UgY291bGQgY3Jhc2ggYW5kIGxlYXZlDQo+IHRoZSBob3N0IGxvY2sg
ZGFuZ2xpbmcgOi8NCj4gDQo+IFlvdXJzLA0KPiBMaW51cyBXYWxsZWlqDQo=

RE: Outstanding MQ questions from MMC

[Avri Altman]

You can see how it's done in mmc_blk_ioctl_rpmb_cmd().

The RPMB protocol defines 6 types of accesses:
Accesses that performs read operation (read counter, read data, and read configuration) - requires sending 2 requests. 
Accesses that performs write operation (program key, write data, write configuration) - requires sending 3 requests,
But you must do read counter beforehand (accept from program key), hence the 5 different requests.

The standard does not define a "special" request that does it all in once, 
but expects a pre-define series of cmd18 & cmd25 for each access type, 
in which the payload are 512 bytes frames in a pre-defined structure.
  
Cheers,
Avri

> -----Original Message-----
> From: Linus Walleij [mailto:linus.walleij@linaro.org]
> Sent: Saturday, April 15, 2017 9:35 PM
> To: Avri Altman <Avri.Altman@sandisk.com>
> Cc: Arnd Bergmann <arnd@arndb.de>; Ulf Hansson
> <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; linux-
> block@vger.kernel.org; Jens Axboe <axboe@kernel.dk>; Christoph Hellwig
> <hch@lst.de>; Adrian Hunter <adrian.hunter@intel.com>; Paolo Valente
> <paolo.valente@linaro.org>
> Subject: Re: Outstanding MQ questions from MMC
> 
> On Fri, Apr 14, 2017 at 8:41 PM, Avri Altman <Avri.Altman@sandisk.com>
> wrote:
> > [Me]
> >> 2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
> >>    things and funnel them into the block scheduler
> >>    using REQ_OP_DRV_IN/OUT requests.
> >>
> >
> > Accessing the RPMB is done via a strange protocol, in which each access is
> comprised of several requests.
> > For example, writing to the RPMB will require sending 5 different requests:
> > 2 requests to read the write counter, and then 3 more requests for the
> write operation itself.
> >
> > Once the sequence has started, it should not get interfered by other
> requests, or the operation will fail.
> 
> So I guess currently something takes a host lock and then performs the
> 5 requests.
> 
> Thus we need to send a single custom request containing a list of 5 things to
> do, and return after that.
> 
> Or do you mean that we return to userspace inbetween these different
> requests and the sequencing is done in userspace?
> 
> I hope not because that sounds fragile, like userspace could crash and leave
> the host lock dangling :/
> 
> Yours,
> Linus Walleij

Re: Outstanding MQ questions from MMC

[Alex Lemberg]

VGhlcmUgaXMgYW4gYWRkaXRpb25hbCBmdW5jdGlvbmFsaXR5LCB3aGljaCBpcyByZXF1aXJlIHRo
ZSBob3N0IGxvY2sNCnRvIGJlIGhlbGQgZm9yIHNldmVyYWwgd3JpdGUgY29tbWFuZHMgLSB0aGUg
RkZVLg0KSW4gY2FzZSBvZiBGRlUsIHRoZSBGVyBjYW4gYmUgZG93bmxvYWQvd3JpdGUgaW4gc2V2
ZXJhbCBpdGVyYXRpb25zDQpvZiBXcml0ZSBjb21tYW5kIChDTUQyNSkuIFRoaXMgc2VxdWVuY2Ug
c2hvdWxkIG5vdCBiZSBpbnRlcnJ1cHRlZCBieSByZWd1bGFyDQpXcml0ZSByZXF1ZXN0cy4NCklu
IGN1cnJlbnQgZHJpdmVyLCBib3RoIEZGVSBhbmQgUlBNQiBjYW4gYmUgc2VudCBieSB1c2luZyBt
bWNfYmxrX2lvY3RsX211bHRpX2NtZCgpLg0KDQpUaGFua3MsDQpBbGV4DQoNCg0KPiBPbiBBcHIg
MTUsIDIwMTcsIGF0IDEwOjI0IFBNLCBBdnJpIEFsdG1hbiA8QXZyaS5BbHRtYW5Ac2FuZGlzay5j
b20+IHdyb3RlOg0KPiANCj4gWW91IGNhbiBzZWUgaG93IGl0J3MgZG9uZSBpbiBtbWNfYmxrX2lv
Y3RsX3JwbWJfY21kKCkuDQo+IA0KPiBUaGUgUlBNQiBwcm90b2NvbCBkZWZpbmVzIDYgdHlwZXMg
b2YgYWNjZXNzZXM6DQo+IEFjY2Vzc2VzIHRoYXQgcGVyZm9ybXMgcmVhZCBvcGVyYXRpb24gKHJl
YWQgY291bnRlciwgcmVhZCBkYXRhLCBhbmQgcmVhZCBjb25maWd1cmF0aW9uKSAtIHJlcXVpcmVz
IHNlbmRpbmcgMiByZXF1ZXN0cy4gDQo+IEFjY2Vzc2VzIHRoYXQgcGVyZm9ybXMgd3JpdGUgb3Bl
cmF0aW9uIChwcm9ncmFtIGtleSwgd3JpdGUgZGF0YSwgd3JpdGUgY29uZmlndXJhdGlvbikgLSBy
ZXF1aXJlcyBzZW5kaW5nIDMgcmVxdWVzdHMsDQo+IEJ1dCB5b3UgbXVzdCBkbyByZWFkIGNvdW50
ZXIgYmVmb3JlaGFuZCAoYWNjZXB0IGZyb20gcHJvZ3JhbSBrZXkpLCBoZW5jZSB0aGUgNSBkaWZm
ZXJlbnQgcmVxdWVzdHMuDQo+IA0KPiBUaGUgc3RhbmRhcmQgZG9lcyBub3QgZGVmaW5lIGEgInNw
ZWNpYWwiIHJlcXVlc3QgdGhhdCBkb2VzIGl0IGFsbCBpbiBvbmNlLCANCj4gYnV0IGV4cGVjdHMg
YSBwcmUtZGVmaW5lIHNlcmllcyBvZiBjbWQxOCAmIGNtZDI1IGZvciBlYWNoIGFjY2VzcyB0eXBl
LCANCj4gaW4gd2hpY2ggdGhlIHBheWxvYWQgYXJlIDUxMiBieXRlcyBmcmFtZXMgaW4gYSBwcmUt
ZGVmaW5lZCBzdHJ1Y3R1cmUuDQo+IA0KPiBDaGVlcnMsDQo+IEF2cmkNCj4gDQo+PiAtLS0tLU9y
aWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPj4gRnJvbTogTGludXMgV2FsbGVpaiBbbWFpbHRvOmxpbnVz
LndhbGxlaWpAbGluYXJvLm9yZ10NCj4+IFNlbnQ6IFNhdHVyZGF5LCBBcHJpbCAxNSwgMjAxNyA5
OjM1IFBNDQo+PiBUbzogQXZyaSBBbHRtYW4gPEF2cmkuQWx0bWFuQHNhbmRpc2suY29tPg0KPj4g
Q2M6IEFybmQgQmVyZ21hbm4gPGFybmRAYXJuZGIuZGU+OyBVbGYgSGFuc3Nvbg0KPj4gPHVsZi5o
YW5zc29uQGxpbmFyby5vcmc+OyBsaW51eC1tbWNAdmdlci5rZXJuZWwub3JnOyBsaW51eC0NCj4+
IGJsb2NrQHZnZXIua2VybmVsLm9yZzsgSmVucyBBeGJvZSA8YXhib2VAa2VybmVsLmRrPjsgQ2hy
aXN0b3BoIEhlbGx3aWcNCj4+IDxoY2hAbHN0LmRlPjsgQWRyaWFuIEh1bnRlciA8YWRyaWFuLmh1
bnRlckBpbnRlbC5jb20+OyBQYW9sbyBWYWxlbnRlDQo+PiA8cGFvbG8udmFsZW50ZUBsaW5hcm8u
b3JnPg0KPj4gU3ViamVjdDogUmU6IE91dHN0YW5kaW5nIE1RIHF1ZXN0aW9ucyBmcm9tIE1NQw0K
Pj4gDQo+PiBPbiBGcmksIEFwciAxNCwgMjAxNyBhdCA4OjQxIFBNLCBBdnJpIEFsdG1hbiA8QXZy
aS5BbHRtYW5Ac2FuZGlzay5jb20+DQo+PiB3cm90ZToNCj4+PiBbTWVdDQo+Pj4+IDIuIFR1cm4g
UlBNQiBhbmQgb3RoZXIgaW9jdGwoKSBNTUMgb3BlcmF0aW9ucyBpbnRvIG1tY19xdWV1ZV9yZXEN
Cj4+Pj4gICB0aGluZ3MgYW5kIGZ1bm5lbCB0aGVtIGludG8gdGhlIGJsb2NrIHNjaGVkdWxlcg0K
Pj4+PiAgIHVzaW5nIFJFUV9PUF9EUlZfSU4vT1VUIHJlcXVlc3RzLg0KPj4+PiANCj4+PiANCj4+
PiBBY2Nlc3NpbmcgdGhlIFJQTUIgaXMgZG9uZSB2aWEgYSBzdHJhbmdlIHByb3RvY29sLCBpbiB3
aGljaCBlYWNoIGFjY2VzcyBpcw0KPj4gY29tcHJpc2VkIG9mIHNldmVyYWwgcmVxdWVzdHMuDQo+
Pj4gRm9yIGV4YW1wbGUsIHdyaXRpbmcgdG8gdGhlIFJQTUIgd2lsbCByZXF1aXJlIHNlbmRpbmcg
NSBkaWZmZXJlbnQgcmVxdWVzdHM6DQo+Pj4gMiByZXF1ZXN0cyB0byByZWFkIHRoZSB3cml0ZSBj
b3VudGVyLCBhbmQgdGhlbiAzIG1vcmUgcmVxdWVzdHMgZm9yIHRoZQ0KPj4gd3JpdGUgb3BlcmF0
aW9uIGl0c2VsZi4NCj4+PiANCj4+PiBPbmNlIHRoZSBzZXF1ZW5jZSBoYXMgc3RhcnRlZCwgaXQg
c2hvdWxkIG5vdCBnZXQgaW50ZXJmZXJlZCBieSBvdGhlcg0KPj4gcmVxdWVzdHMsIG9yIHRoZSBv
cGVyYXRpb24gd2lsbCBmYWlsLg0KPj4gDQo+PiBTbyBJIGd1ZXNzIGN1cnJlbnRseSBzb21ldGhp
bmcgdGFrZXMgYSBob3N0IGxvY2sgYW5kIHRoZW4gcGVyZm9ybXMgdGhlDQo+PiA1IHJlcXVlc3Rz
Lg0KPj4gDQo+PiBUaHVzIHdlIG5lZWQgdG8gc2VuZCBhIHNpbmdsZSBjdXN0b20gcmVxdWVzdCBj
b250YWluaW5nIGEgbGlzdCBvZiA1IHRoaW5ncyB0bw0KPj4gZG8sIGFuZCByZXR1cm4gYWZ0ZXIg
dGhhdC4NCj4+IA0KPj4gT3IgZG8geW91IG1lYW4gdGhhdCB3ZSByZXR1cm4gdG8gdXNlcnNwYWNl
IGluYmV0d2VlbiB0aGVzZSBkaWZmZXJlbnQNCj4+IHJlcXVlc3RzIGFuZCB0aGUgc2VxdWVuY2lu
ZyBpcyBkb25lIGluIHVzZXJzcGFjZT8NCj4+IA0KPj4gSSBob3BlIG5vdCBiZWNhdXNlIHRoYXQg
c291bmRzIGZyYWdpbGUsIGxpa2UgdXNlcnNwYWNlIGNvdWxkIGNyYXNoIGFuZCBsZWF2ZQ0KPj4g
dGhlIGhvc3QgbG9jayBkYW5nbGluZyA6Lw0KPj4gDQo+PiBZb3VycywNCj4+IExpbnVzIFdhbGxl
aWoNCj4gE++/ve+/vey5uxzvv70m77+9fu+/vSbvv70Y77+977+9Ky3vv73vv73dthfvv73vv713
77+977+9y5vvv73vv73vv71t77+9Yu+/ve+/vWbvv73Ip++/vRfvv73vv73cqH3vv73vv73vv73G
oHrvv70majordu+/ve+/ve+/vQfvv73vv73vv73vv716Wivvv73vv70rembvv73vv73vv71o77+9
77+977+9fu+/ve+/ve+/ve+/vWnvv73vv73vv71677+9Hu+/vXfvv73vv73vv70/77+977+977+9
77+9Ju+/vSnfohtmDQoNCg==

Re: Outstanding MQ questions from MMC

[Alex Lemberg]

There is an additional functionality, which is require the host lock
to be held for several write commands - the FFU.
In case of FFU, the FW can be download/write in several iterations
of Write command (CMD25). This sequence should not be interrupted by regular
Write requests.
In current driver, both FFU and RPMB can be sent by using mmc_blk_ioctl_multi_cmd().

Thanks,
Alex


> On Apr 15, 2017, at 10:24 PM, Avri Altman <Avri.Altman@sandisk.com> wrote:
> 
> You can see how it's done in mmc_blk_ioctl_rpmb_cmd().
> 
> The RPMB protocol defines 6 types of accesses:
> Accesses that performs read operation (read counter, read data, and read configuration) - requires sending 2 requests. 
> Accesses that performs write operation (program key, write data, write configuration) - requires sending 3 requests,
> But you must do read counter beforehand (accept from program key), hence the 5 different requests.
> 
> The standard does not define a "special" request that does it all in once, 
> but expects a pre-define series of cmd18 & cmd25 for each access type, 
> in which the payload are 512 bytes frames in a pre-defined structure.
> 
> Cheers,
> Avri
> 
>> -----Original Message-----
>> From: Linus Walleij [mailto:linus.walleij@linaro.org]
>> Sent: Saturday, April 15, 2017 9:35 PM
>> To: Avri Altman <Avri.Altman@sandisk.com>
>> Cc: Arnd Bergmann <arnd@arndb.de>; Ulf Hansson
>> <ulf.hansson@linaro.org>; linux-mmc@vger.kernel.org; linux-
>> block@vger.kernel.org; Jens Axboe <axboe@kernel.dk>; Christoph Hellwig
>> <hch@lst.de>; Adrian Hunter <adrian.hunter@intel.com>; Paolo Valente
>> <paolo.valente@linaro.org>
>> Subject: Re: Outstanding MQ questions from MMC
>> 
>> On Fri, Apr 14, 2017 at 8:41 PM, Avri Altman <Avri.Altman@sandisk.com>
>> wrote:
>>> [Me]
>>>> 2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
>>>>   things and funnel them into the block scheduler
>>>>   using REQ_OP_DRV_IN/OUT requests.
>>>> 
>>> 
>>> Accessing the RPMB is done via a strange protocol, in which each access is
>> comprised of several requests.
>>> For example, writing to the RPMB will require sending 5 different requests:
>>> 2 requests to read the write counter, and then 3 more requests for the
>> write operation itself.
>>> 
>>> Once the sequence has started, it should not get interfered by other
>> requests, or the operation will fail.
>> 
>> So I guess currently something takes a host lock and then performs the
>> 5 requests.
>> 
>> Thus we need to send a single custom request containing a list of 5 things to
>> do, and return after that.
>> 
>> Or do you mean that we return to userspace inbetween these different
>> requests and the sequencing is done in userspace?
>> 
>> I hope not because that sounds fragile, like userspace could crash and leave
>> the host lock dangling :/
>> 
>> Yours,
>> Linus Walleij
> \x13��칻\x1c�&�~�&�\x18��+-��ݶ\x17��w��˛���m�b��f�ȧ�\x17��ܨ}���Ơz�&j:+v���\a����zZ+��+zf���h���~����i���z�\x1e�w���?����&�)ߢ^[f

Re: Outstanding MQ questions from MMC

[Linus Walleij]

On Tue, Apr 18, 2017 at 5:31 PM, Alex Lemberg <Alex.Lemberg@sandisk.com> wrote:

> There is an additional functionality, which is require the host lock
> to be held for several write commands - the FFU.
> In case of FFU, the FW can be download/write in several iterations
> of Write command (CMD25). This sequence should not be interrupted by regular
> Write requests.
> In current driver, both FFU and RPMB can be sent by using mmc_blk_ioctl_multi_cmd().

Both single and multi ioctl()s are funneled into the block layer
using the driver-specific request ops in the latest iteration of
my patch set.

It turns out this was a simpler change than I though.

If you check the patch set I realized that I also fixes a userspace
starvation issue when issueing ioctls() such as for RPMB
during heavy block I/O.

This usecase:

> dd if=/dev/mmcblk3 of=/dev/null bs=1M &
> mmc extcs read /dev/mmcblk3

This would previously hang until the dd command was complete before
issuing the ioctl() command, just waiting for the host lock.
I guess RPMB has the same problem...

It is now fixed.

If you can verify the v2 patch set (just posted) and provide Tested-by's
(or bug reports...) it's appreciated.

Yours,
Linus Walleij

Re: Outstanding MQ questions from MMC

[Ulf Hansson]

[...]

>> Alternatively, I had this idea that we could translate blk requests into
>> mmc commands and then have a (short fixed length) set of outstanding
>> mmc commands in the device that always get done in order. The card
>> detect and the user space I/O would then directly put mmc commands
>> onto the command queue, as would the blk-mq scheduler. You
>> still need a lock to access that command queue, but the mmc host
>> would just always pick the next command off the list when one
>> command completes.
>
> I looked into this.
>
> The block layer queue can wrap and handle custom device commands
> using REQ_OP_DRV_IN/OUT, and that seems to be the best way
> to play with the block layer IMO.
>
> The card detect work is a special case because it is also used by
> SDIO which does not use the block layer. But that could maybe be
> solved by a separate host lock just for the SDIO case, letting
> devices accessed as block devices use the method of inserting
> custom commands.

The problem with trying to manage the SDIO case as a specific case, it
that it is the same work (mmc_rescan()) that runs to detect any kind
of removable card.

Moreover, it's not until the card has been fully detected and
initialized, when we can realize what kind of card it is.

Perhaps we can re-factor the hole mmc_rescan() thing so there is one
part that can be run only to detect new cards being inserted in
lockless fashion, while another part could deal with the
polling/removal - which then perhaps could be different depending on
the card type.

Not sure if this helps...

>
> I looked at how e.g. IDE and SCSI does this, drivers/ide/ide-ioctls.c
> looks like this nowadays:
>
> static int generic_drive_reset(ide_drive_t *drive)
> {
>         struct request *rq;
>         int ret = 0;
>
>         rq = blk_get_request(drive->queue, REQ_OP_DRV_IN, __GFP_RECLAIM);
>         scsi_req_init(rq);
>         ide_req(rq)->type = ATA_PRIV_MISC;
>         scsi_req(rq)->cmd_len = 1;
>         scsi_req(rq)->cmd[0] = REQ_DRIVE_RESET;
>         if (blk_execute_rq(drive->queue, NULL, rq, 1))
>                 ret = rq->errors;
>         blk_put_request(rq);
>         return ret;
> }
>
> So it creates a custom REQ_OP_DRV_IN request, then scsi_req_init()
> sets up the special command, in this case
> ATA_PRIV_MISC/REQ_DRIVE_RESET and toss this into the block
> queue like everything else.
>
> We could do the same, especially the RPMB operations should
> probably have been done like this from the beginning. But due to
> historical factors they were not.
>
> It is a bit hairy and the whole thing is in a bit of flux because Christoph
> is heavily refactoring this and cleaning up the old block devices as
> we speak (I bet) so it is a bit hard to do the right thing.
>
> I easily get confused here ... for example there is custom
> per-request data access by this simple:
>
> scsi_req_init(rq)
>
> which does
>
> struct scsi_request *req = scsi_req(rq);
>
> which does
>
> static inline struct scsi_request *scsi_req(struct request *rq)
> {
>         return blk_mq_rq_to_pdu(rq);
> }
>
> Oohps blk_mq_* namespace? You would assume this means that
> you have to use blk-mq? Nah, I think not, because all it does is:
>
> static inline void *blk_mq_rq_to_pdu(struct request *rq)
> {
>         return rq + 1;
> }
>
> So while we have to #include <linux/blk-mq.h> this is one of these
> mixed semantics that just give you a pointer to something behind
> the request, a method that is simple and natural in blk-mq but which
> is (I guess) set up by some other mechanism in the !mq case,
> albeit access by this inline.
>
> And I have to do this with the old block layer to get to a point
> where we can start using blk-mq, sigh.
>
> The border between blk and blk-mq is a bit blurry right now.
>
> With blk-mq I do this:
>
> mq->tag_set.cmd_size = sizeof(foo_cmd);
> blk_mq_alloc_tag_set(...)
>
> To do this with the old blk layer I may need some help to figure
> out how to set up per-request additional data in a way that works
> with the old layer.
>
> scsi_lib.c scsi_alloc_queue() does this:
>
> q = blk_alloc_queue_node(GFP_KERNEL, NUMA_NO_NODE);
> if (!q)
>       return NULL;
> q->cmd_size = sizeof(foo_cmd);
>
> And this means there will be sizeof(foo_cmd) after the request
> that can be dereferenced by blk_mq_rq_to_pdu(rq);...
>
> Yeah I'll try it.
>
> Just trying to give a picture of why it's a bit in flux here.
> Or documenting it for myself :D
>
>> This also lets you integrate packed commands: if the next outstanding
>> command is the same type as the request coming in from blk-mq,
>> you can merge it into a single mmc command to be submitted
>> together, otherwise it gets deferred.
>
> Essentially the heavy lifting that needs to happen is:
>
> 1. Start allocating per-request PDU (extra data) in the MMC case
>    this will then be struct mmc_queue_req request items.
>
> 2. Turn RPMB and other ioctl() MMC operations into mmc_queue_req
>    things and funnel them into the block scheduler
>    using REQ_OP_DRV_IN/OUT requests.
>
> 3. Turn the card detect into an mmc_queue_req as well
>
> 4. We can kill the big MMC host lock for block devices and
>    split off an SDIO-only host lock.
>
> I'm onto it ... I guess.

It looks hairy, but please have a try!

In the meantime, I will wrap my head around and try to see if we can
find a possible easier intermediate step.

Kind regards
Uffe

Re: Outstanding MQ questions from MMC

[Christoph Hellwig]

On Wed, Mar 29, 2017 at 05:09:37AM +0200, Linus Walleij wrote:
> 2. When MMC cards are ejected a serious error condition occurs. So for this
> reason we spool out the queue with
> 
> req->rq_flags |= RQF_QUIET;
> blk_end_request_all(req, -EIO);
> 
> This will shut up a huge amount of console errors for example.
> I have no clue on how to manage this with MQ. I am currently using
> 
> blk_mq_complete_request(mq_rq->req, -EIO);
> 
> and nothing else, and it will hit all requests for the ejected card coming
> in from this point. Is this the right solution? Or is there some medium
> eject handling I'm not aware of inside the MQ layer? It seems like something
> that can happen on pluggable harddrives and CDROMS and what not.

Hot unplug handling currently is a mess in the block layer (not just the
mq case).  Especially if you ever requeue an I/O there is tons of
boilerplate code.  I wish we could move a little more of this into the
core, but right now I don't have a good idea on how to.

> 3. Sometimes a read or write gets partially completed. Say we read 12 out
> of 15 sectors or somthing like that. I have no idea how often this occurs in
> practice. With the old block layer we did this:
> 
> blk_end_request(req, 0, bytes_xfered);

You can still do this with mq, but you have to open code it.  Take a look
at scsi_end_request which has opencoded versions of the mq and non-mq
end_request routines.  Currently it's the only example of blk-mq partial
completions.