* [PATCH] null_blk: execute complete callback for fake timeout request
@ 2023-03-12 12:35 Akinobu Mita
2023-03-13 1:00 ` Damien Le Moal
0 siblings, 1 reply; 5+ messages in thread
From: Akinobu Mita @ 2023-03-12 12:35 UTC (permalink / raw)
To: linux-block; +Cc: Akinobu Mita, Damien Le Moal, Jens Axboe
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@wdc.com>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
---
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:
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] null_blk: execute complete callback for fake timeout request
2023-03-12 12:35 [PATCH] null_blk: execute complete callback for fake timeout request Akinobu Mita
@ 2023-03-13 1:00 ` Damien Le Moal
2023-03-13 16:39 ` Akinobu Mita
2023-03-13 16:50 ` Keith Busch
0 siblings, 2 replies; 5+ messages in thread
From: Damien Le Moal @ 2023-03-13 1:00 UTC (permalink / raw)
To: Akinobu Mita, linux-block; +Cc: Jens Axboe
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@wdc.com>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> ---
> 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
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] null_blk: execute complete callback for fake timeout request
2023-03-13 1:00 ` Damien Le Moal
@ 2023-03-13 16:39 ` Akinobu Mita
2023-03-13 16:50 ` Keith Busch
1 sibling, 0 replies; 5+ messages in thread
From: Akinobu Mita @ 2023-03-13 16:39 UTC (permalink / raw)
To: Damien Le Moal; +Cc: linux-block, Jens Axboe
2023年3月13日(月) 10:00 Damien Le Moal <damien.lemoal@opensource.wdc.com>:
>
> 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@wdc.com>
> > Cc: Jens Axboe <axboe@kernel.dk>
> > Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > ---
> > 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 ?
Yes it worked.
Tested-by: Akinobu Mita <akinobu.mita@gmail.com>
> 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().
I also prefer this one.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] null_blk: execute complete callback for fake timeout request
2023-03-13 1:00 ` Damien Le Moal
2023-03-13 16:39 ` Akinobu Mita
@ 2023-03-13 16:50 ` Keith Busch
2023-03-14 3:56 ` Damien Le Moal
1 sibling, 1 reply; 5+ messages in thread
From: Keith Busch @ 2023-03-13 16:50 UTC (permalink / raw)
To: Damien Le Moal; +Cc: Akinobu Mita, linux-block, Jens Axboe
On Mon, Mar 13, 2023 at 10:00:30AM +0900, Damien Le Moal wrote:
> On 3/12/23 21:35, Akinobu Mita wrote:
>
> 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);
I think you can remove the fake_timeout check from here now since this function
is never called when it's true.
> 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);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] null_blk: execute complete callback for fake timeout request
2023-03-13 16:50 ` Keith Busch
@ 2023-03-14 3:56 ` Damien Le Moal
0 siblings, 0 replies; 5+ messages in thread
From: Damien Le Moal @ 2023-03-14 3:56 UTC (permalink / raw)
To: Keith Busch; +Cc: Akinobu Mita, linux-block, Jens Axboe
On 3/14/23 01:50, Keith Busch wrote:
> On Mon, Mar 13, 2023 at 10:00:30AM +0900, Damien Le Moal wrote:
>> On 3/12/23 21:35, Akinobu Mita wrote:
>>
>> 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);
>
> I think you can remove the fake_timeout check from here now since this function
> is never called when it's true.
Right. Sending a proper patch with that.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-14 3:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-12 12:35 [PATCH] null_blk: execute complete callback for fake timeout request Akinobu Mita
2023-03-13 1:00 ` Damien Le Moal
2023-03-13 16:39 ` Akinobu Mita
2023-03-13 16:50 ` Keith Busch
2023-03-14 3:56 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.