On 2020-05-02 05:11, Hannes Reinecke wrote:
> On 5/1/20 6:59 AM, Bart Van Assche wrote:
>> On 2020-04-30 06:18, Hannes Reinecke wrote:
>>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>>> index 44482aaed11e..402cf104d183 100644
>>> --- a/block/blk-mq.c
>>> +++ b/block/blk-mq.c
>>> @@ -402,9 +402,14 @@ struct request *blk_mq_alloc_request(struct
>>> request_queue *q, unsigned int op,
>>> {
>>> struct blk_mq_alloc_data alloc_data = { .flags = flags,
>>> .cmd_flags = op };
>>> struct request *rq;
>>> - int ret;
>>> + int ret = 0;
>>> - ret = blk_queue_enter(q, flags);
>>> + if (flags & BLK_MQ_REQ_PERSISTENT) {
>>> + if (blk_queue_dying(q))
>>> + ret = -ENODEV;
>>> + alloc_data.cmd_flags |= REQ_PERSISTENT;
>>> + } else
>>> + ret = blk_queue_enter(q, flags);
>>> if (ret)
>>> return ERR_PTR(ret);
>>>
>>
>> I think that not calling blk_queue_enter() for persistent commands means
>> opening a giant can of worms. There is quite some code in the block
>> layer that assumes that neither .queue_rq() nor the request completion
>> code will be called if q_usage_counter == 0. Skipping the
>> blk_queue_enter() call for persistent commands breaks that assumption. I
>> think we need a better solution.
>>
> Well, yeah, maybe.
> My aim here is that _all_ I/O requiring a tag from the hardware will be
> tracked by the blocklayer tagset. Only that will give the block-layer
> accurate information about outstanding commands, such that the ongoing
> CPU hotplug discussion can make the correct decisions and implement
> functions really covering all outstanding I/O.
> It also allows us to use the scsi_host_busy_iter() functions within the
> driver, and will get rid of the hand-crafted iterations the driver has
> to do now.
>
> It worked reasonably well, until I encountered the infamous AEN
> commands, which actually require the opposite: _not_ to be tracked by
> the block layer at all, as the commands themselves are just placeholders
> to be returned by the firmware once an event occurs.
> (And yes, I _do_ think this is a quite dangerous operation, because I
> can't quite see how one could reliably return this command in case of a
> firmware crash ...)
> (But anyhow, that's how the firmware is written and we have to live with
> it.)
>
> So I implemented this approach, to have tags which are ignored by the
> block layer. But I have to admit that this approach relies on quite some
> assumptions (like these tags are never actually submitted to the
> blocklayer itself, are never started etc), none of which are spelled out
> clearly (yet).
> An alternative approach would be to arbitrary decrease the tagset size
> by one (eg by shifting the tags by one), and use the free tag for AENs).
> That would have to be coded within the driver, though.
>
> If that's a solution which you like better I could give it a go.
How about dropping support for AEN commands entirely? As far as I know
such a command has never been standardized. Additionally, all SCSI core
code I'm familiar with supports unit attentions and does not rely on
asynchronous events to be reported immediately.
If dropping support for AEN commands is not an option, how about
aborting these commands before freezing a request queue?
Thanks,
Bart.
