Re: [PATCH RFC v3 22/41] block: implement persistent commands

[Bart Van Assche <bvanassche@acm.org>]

On 2020-04-30 06:18, Hannes Reinecke wrote:
> Some LLDDs implement event handling by sending a command to the
> firmware, which then will be completed once the firmware wants
> to register an event.
     ^^^^^^^^
     report?

> So worst case a command is being sent to the firmware then the
                                                        ^^^^
                                                        when?
> driver initializes, and will be returned once the driver unloads.
> To avoid these commands to block the queues during freezing or
> quiescing this patch implements support for 'persistent' commands,
> which will be excluded from blk_queue_enter() and blk_queue_exit()
> calls.

How is it prevented that the SCSI timeout handler is activated for
persistent commands?

> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  block/blk-mq.c            | 12 +++++++++---
>  include/linux/blk-mq.h    |  2 ++
>  include/linux/blk_types.h |  4 ++++
>  3 files changed, 15 insertions(+), 3 deletions(-)
> 
> 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.

Thanks,

Bart.