Re: [PATCH] null_blk: execute complete callback for fake timeout request

[Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>]

On 3/12/23 21:35, Akinobu Mita wrote:
> When injecting a fake timeout into the null_blk driver by fail_io_timeout,
> the request timeout handler doen't execute blk_mq_complete_request(), so
> the complete callback will never be executed for that timed out request.
> 
> The null_blk driver also has a driver-specific fake timeout mechanism and
> does not have the problem that occur when using the generic one.
> Fix the problem by doing similar to what the driver-specific one does.
> 
> Fixes: de3510e52b0a ("null_blk: fix command timeout completion handling")
> Cc: Damien Le Moal <damien.lemoal@xxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> Signed-off-by: Akinobu Mita <akinobu.mita@xxxxxxxxx>
> ---
>  drivers/block/null_blk/main.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
> index 4c601ca9552a..69250b3cfecd 100644
> --- a/drivers/block/null_blk/main.c
> +++ b/drivers/block/null_blk/main.c
> @@ -1413,7 +1413,9 @@ static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
>  	case NULL_IRQ_SOFTIRQ:
>  		switch (cmd->nq->dev->queue_mode) {
>  		case NULL_Q_MQ:
> -			if (likely(!blk_should_fake_timeout(cmd->rq->q)))
> +			if (unlikely(blk_should_fake_timeout(cmd->rq->q)))
> +				cmd->fake_timeout = true;
> +			else
>  				blk_mq_complete_request(cmd->rq);
>  			break;
>  		case NULL_Q_BIO:

I have not checked, but does this work ?

diff --git a/drivers/block/null_blk/main.c b/drivers/block/null_blk/main.c
index 4c601ca9552a..52d689aa3171 100644
--- a/drivers/block/null_blk/main.c
+++ b/drivers/block/null_blk/main.c
@@ -1413,7 +1413,7 @@ static inline void nullb_complete_cmd(struct nullb_cmd *cmd)
        case NULL_IRQ_SOFTIRQ:
                switch (cmd->nq->dev->queue_mode) {
                case NULL_Q_MQ:
-                       if (likely(!blk_should_fake_timeout(cmd->rq->q)))
+                       if (!cmd->fake_timeout)
                                blk_mq_complete_request(cmd->rq);
                        break;
                case NULL_Q_BIO:
@@ -1675,7 +1675,8 @@ static blk_status_t null_queue_rq(struct blk_mq_hw_ctx *hctx,
        cmd->rq = bd->rq;
        cmd->error = BLK_STS_OK;
        cmd->nq = nq;
-       cmd->fake_timeout = should_timeout_request(bd->rq);
+       cmd->fake_timeout = should_timeout_request(bd->rq) ||
+               blk_should_fake_timeout(bd->rq->q);

        blk_mq_start_request(bd->rq);


It is I think cleaner as it unifies the internal fake timeout and
blk_should_fake_timeout().

-- 
Damien Le Moal
Western Digital Research