Re: [RFC PATCH 3/3] blk-mq: Remove generation seqeunce

Hannes Reinecke
2018-05-23T05:48:47Z

On 05/22/2018 06:17 PM, Christoph Hellwig wrote:
> Hi Keith,
>
> I like this series a lot. One comment that is probably close
> to the big discussion in the thread:
>
>> switch (ret) {
>> case BLK_EH_HANDLED:
>> /*
>> + * If the request is still in flight, the driver is requesting
>> + * blk-mq complete it.
>> */
>> + if (blk_mq_rq_state(req) == MQ_RQ_IN_FLIGHT)
>> + __blk_mq_complete_request(req);
>> + break;
>
> The state check here really irked me, and from the thread it seems like
> I'm not the only one. At least for the NVMe case I think it is perfectly
> safe, although I agree I'd rather audit what other drivers do carefully.
>
> That being said I think BLK_EH_HANDLED seems like a fundamentally broken
> idea, and I'd actually prefer to get rid of it over adding things like
> the MQ_RQ_IN_FLIGHT check above.
>
I can't agree more here.

BLK_EH_HANDLED is breaking all sorts of assumptions, and I'd be glad to
see it go.

> E.g. if we look at the cases where nvme-pci returns it:
>
> - if we did call nvme_dev_disable, we already canceled all requests,
> so we might as well just return BLK_EH_NOT_HANDLED
> - the poll for completion case already completed the command,
> so we should return BLK_EH_NOT_HANDLED
>
> So I think we need to fix up nvme and if needed any other driver
> to return the right value and then assert that the request is
> still in in-flight status for the BLK_EH_HANDLED case.
>
... and then kill BLK_EH_HANDLED :-)

Cheers,

Hannes