[PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Ming Lei]

From: David Jeffery <djeffery@redhat.com>

David Jeffery found one double ->queue_rq() issue, so far it can
be triggered in VM use case because of long vmexit latency or preempt
latency of vCPU pthread or long page fault in vCPU pthread, then block
IO req could be timed out before queuing the request to hardware but after
calling blk_mq_start_request() during ->queue_rq(), then timeout handler
may handle it by requeue, then double ->queue_rq() is caused, and kernel
panic.

So far, it is driver's responsibility to cover the race between timeout
and completion, so it seems supposed to be solved in driver in theory,
given driver has enough knowledge.

But it is really one common problem, lots of driver could have similar
issue, and could be hard to fix all affected drivers, even it isn't easy
for driver to handle the race. So David suggests this patch by draining
in-progress ->queue_rq() for solving this issue.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: David Jeffery <djeffery@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 33292c01875d..e0aafbc7390f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1523,7 +1523,12 @@ static void blk_mq_rq_timed_out(struct request *req)
 	blk_add_timer(req);
 }
 
-static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
+struct blk_expired_data {
+	unsigned long next;
+	unsigned long now;
+};
+
+static bool blk_mq_req_expired(struct request *rq, struct blk_expired_data *expired)
 {
 	unsigned long deadline;
 
@@ -1533,13 +1538,13 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
 		return false;
 
 	deadline = READ_ONCE(rq->deadline);
-	if (time_after_eq(jiffies, deadline))
+	if (time_after_eq(expired->now, deadline))
 		return true;
 
-	if (*next == 0)
-		*next = deadline;
-	else if (time_after(*next, deadline))
-		*next = deadline;
+	if (expired->next == 0)
+		expired->next = deadline;
+	else if (time_after(expired->next, deadline))
+		expired->next = deadline;
 	return false;
 }
 
@@ -1555,7 +1560,7 @@ void blk_mq_put_rq_ref(struct request *rq)
 
 static bool blk_mq_check_expired(struct request *rq, void *priv)
 {
-	unsigned long *next = priv;
+	struct blk_expired_data *expired = priv;
 
 	/*
 	 * blk_mq_queue_tag_busy_iter() has locked the request, so it cannot
@@ -1564,7 +1569,7 @@ static bool blk_mq_check_expired(struct request *rq, void *priv)
 	 * it was completed and reallocated as a new request after returning
 	 * from blk_mq_check_expired().
 	 */
-	if (blk_mq_req_expired(rq, next))
+	if (blk_mq_req_expired(rq, expired))
 		blk_mq_rq_timed_out(rq);
 	return true;
 }
@@ -1573,7 +1578,7 @@ static void blk_mq_timeout_work(struct work_struct *work)
 {
 	struct request_queue *q =
 		container_of(work, struct request_queue, timeout_work);
-	unsigned long next = 0;
+	struct blk_expired_data expired = {.next = 0, .now = jiffies};
 	struct blk_mq_hw_ctx *hctx;
 	unsigned long i;
 
@@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
 	if (!percpu_ref_tryget(&q->q_usage_counter))
 		return;
 
-	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
+	/*
+	 * Before walking tags, we must ensure any submit started before
+	 * the current time has finished. Since the submit uses srcu or rcu,
+	 * wait for a synchronization point to ensure all running submits
+	 * have finished
+	 */
+	blk_mq_wait_quiesce_done(q);
+
+	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &expired);
 
-	if (next != 0) {
-		mod_timer(&q->timeout, next);
+	if (expired.next != 0) {
+		mod_timer(&q->timeout, expired.next);
 	} else {
 		/*
 		 * Request timeouts are handled as a forward rolling timer. If
-- 
2.31.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

[PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Ming Lei]

From: David Jeffery <djeffery@redhat.com>

David Jeffery found one double ->queue_rq() issue, so far it can
be triggered in VM use case because of long vmexit latency or preempt
latency of vCPU pthread or long page fault in vCPU pthread, then block
IO req could be timed out before queuing the request to hardware but after
calling blk_mq_start_request() during ->queue_rq(), then timeout handler
may handle it by requeue, then double ->queue_rq() is caused, and kernel
panic.

So far, it is driver's responsibility to cover the race between timeout
and completion, so it seems supposed to be solved in driver in theory,
given driver has enough knowledge.

But it is really one common problem, lots of driver could have similar
issue, and could be hard to fix all affected drivers, even it isn't easy
for driver to handle the race. So David suggests this patch by draining
in-progress ->queue_rq() for solving this issue.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: David Jeffery <djeffery@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Keith Busch <kbusch@kernel.org>
Cc: virtualization@lists.linux-foundation.org
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-mq.c | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 33292c01875d..e0aafbc7390f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1523,7 +1523,12 @@ static void blk_mq_rq_timed_out(struct request *req)
 	blk_add_timer(req);
 }
 
-static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
+struct blk_expired_data {
+	unsigned long next;
+	unsigned long now;
+};
+
+static bool blk_mq_req_expired(struct request *rq, struct blk_expired_data *expired)
 {
 	unsigned long deadline;
 
@@ -1533,13 +1538,13 @@ static bool blk_mq_req_expired(struct request *rq, unsigned long *next)
 		return false;
 
 	deadline = READ_ONCE(rq->deadline);
-	if (time_after_eq(jiffies, deadline))
+	if (time_after_eq(expired->now, deadline))
 		return true;
 
-	if (*next == 0)
-		*next = deadline;
-	else if (time_after(*next, deadline))
-		*next = deadline;
+	if (expired->next == 0)
+		expired->next = deadline;
+	else if (time_after(expired->next, deadline))
+		expired->next = deadline;
 	return false;
 }
 
@@ -1555,7 +1560,7 @@ void blk_mq_put_rq_ref(struct request *rq)
 
 static bool blk_mq_check_expired(struct request *rq, void *priv)
 {
-	unsigned long *next = priv;
+	struct blk_expired_data *expired = priv;
 
 	/*
 	 * blk_mq_queue_tag_busy_iter() has locked the request, so it cannot
@@ -1564,7 +1569,7 @@ static bool blk_mq_check_expired(struct request *rq, void *priv)
 	 * it was completed and reallocated as a new request after returning
 	 * from blk_mq_check_expired().
 	 */
-	if (blk_mq_req_expired(rq, next))
+	if (blk_mq_req_expired(rq, expired))
 		blk_mq_rq_timed_out(rq);
 	return true;
 }
@@ -1573,7 +1578,7 @@ static void blk_mq_timeout_work(struct work_struct *work)
 {
 	struct request_queue *q =
 		container_of(work, struct request_queue, timeout_work);
-	unsigned long next = 0;
+	struct blk_expired_data expired = {.next = 0, .now = jiffies};
 	struct blk_mq_hw_ctx *hctx;
 	unsigned long i;
 
@@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
 	if (!percpu_ref_tryget(&q->q_usage_counter))
 		return;
 
-	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
+	/*
+	 * Before walking tags, we must ensure any submit started before
+	 * the current time has finished. Since the submit uses srcu or rcu,
+	 * wait for a synchronization point to ensure all running submits
+	 * have finished
+	 */
+	blk_mq_wait_quiesce_done(q);
+
+	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &expired);
 
-	if (next != 0) {
-		mod_timer(&q->timeout, next);
+	if (expired.next != 0) {
+		mod_timer(&q->timeout, expired.next);
 	} else {
 		/*
 		 * Request timeouts are handled as a forward rolling timer. If
-- 
2.31.1

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[David Jeffery]

On Mon, Oct 24, 2022 at 8:55 PM Ming Lei <ming.lei@redhat.com> wrote:
>
> From: David Jeffery <djeffery@redhat.com>
>
> David Jeffery found one double ->queue_rq() issue, so far it can
> be triggered in VM use case because of long vmexit latency or preempt
> latency of vCPU pthread or long page fault in vCPU pthread, then block
> IO req could be timed out before queuing the request to hardware but after
> calling blk_mq_start_request() during ->queue_rq(), then timeout handler
> may handle it by requeue, then double ->queue_rq() is caused, and kernel
> panic.
>
> So far, it is driver's responsibility to cover the race between timeout
> and completion, so it seems supposed to be solved in driver in theory,
> given driver has enough knowledge.
>
> But it is really one common problem, lots of driver could have similar
> issue, and could be hard to fix all affected drivers, even it isn't easy
> for driver to handle the race. So David suggests this patch by draining
> in-progress ->queue_rq() for solving this issue.
>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Cc: David Jeffery <djeffery@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@redhat.com>
> Cc: Keith Busch <kbusch@kernel.org>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> ---

Thank you for your help and patience, Ming.

Signed-off-by: David Jeffery <djeffery@redhat.com>

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Bart Van Assche]

On 10/24/22 17:55, Ming Lei wrote:
> +struct blk_expired_data {
> +	unsigned long next;
> +	unsigned long now;
> +};

How about renaming 'now' into 'before_quiesce'? Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Bart Van Assche]

On 10/24/22 17:55, Ming Lei wrote:
> +struct blk_expired_data {
> +	unsigned long next;
> +	unsigned long now;
> +};

How about renaming 'now' into 'before_quiesce'? Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Jens Axboe]

On 10/24/22 6:55 PM, Ming Lei wrote:
> @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
>  	if (!percpu_ref_tryget(&q->q_usage_counter))
>  		return;
>  
> -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
> +	/*
> +	 * Before walking tags, we must ensure any submit started before
> +	 * the current time has finished. Since the submit uses srcu or rcu,
> +	 * wait for a synchronization point to ensure all running submits
> +	 * have finished
> +	 */
> +	blk_mq_wait_quiesce_done(q);

I'm a little worried about this bit - so we'll basically do a sync RCU
every time the timeout timer runs... Depending on machine load, that
can take a long time.

-- 
Jens Axboe

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Jens Axboe]

On 10/24/22 6:55 PM, Ming Lei wrote:
> @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
>  	if (!percpu_ref_tryget(&q->q_usage_counter))
>  		return;
>  
> -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
> +	/*
> +	 * Before walking tags, we must ensure any submit started before
> +	 * the current time has finished. Since the submit uses srcu or rcu,
> +	 * wait for a synchronization point to ensure all running submits
> +	 * have finished
> +	 */
> +	blk_mq_wait_quiesce_done(q);

I'm a little worried about this bit - so we'll basically do a sync RCU
every time the timeout timer runs... Depending on machine load, that
can take a long time.

-- 
Jens Axboe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Ming Lei]

On Tue, Oct 25, 2022 at 12:11:39PM -0600, Jens Axboe wrote:
> On 10/24/22 6:55 PM, Ming Lei wrote:
> > @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
> >  	if (!percpu_ref_tryget(&q->q_usage_counter))
> >  		return;
> >  
> > -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
> > +	/*
> > +	 * Before walking tags, we must ensure any submit started before
> > +	 * the current time has finished. Since the submit uses srcu or rcu,
> > +	 * wait for a synchronization point to ensure all running submits
> > +	 * have finished
> > +	 */
> > +	blk_mq_wait_quiesce_done(q);
> 
> I'm a little worried about this bit - so we'll basically do a sync RCU
> every time the timeout timer runs... Depending on machine load, that
> can take a long time.

Yeah, the per-queue timeout timer is never canceled after request is
completed, so most of times the timeout work does nothing.

Can we run the sync RCU only if there is timed out request found? Then
the wait is only needed in case that timeout handling is required. Also
sync rcu is already done in some driver's ->timeout().


Thanks,
Ming

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Ming Lei]

On Tue, Oct 25, 2022 at 12:11:39PM -0600, Jens Axboe wrote:
> On 10/24/22 6:55 PM, Ming Lei wrote:
> > @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
> >  	if (!percpu_ref_tryget(&q->q_usage_counter))
> >  		return;
> >  
> > -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
> > +	/*
> > +	 * Before walking tags, we must ensure any submit started before
> > +	 * the current time has finished. Since the submit uses srcu or rcu,
> > +	 * wait for a synchronization point to ensure all running submits
> > +	 * have finished
> > +	 */
> > +	blk_mq_wait_quiesce_done(q);
> 
> I'm a little worried about this bit - so we'll basically do a sync RCU
> every time the timeout timer runs... Depending on machine load, that
> can take a long time.

Yeah, the per-queue timeout timer is never canceled after request is
completed, so most of times the timeout work does nothing.

Can we run the sync RCU only if there is timed out request found? Then
the wait is only needed in case that timeout handling is required. Also
sync rcu is already done in some driver's ->timeout().


Thanks,
Ming

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Jens Axboe]

On 10/25/22 6:21 PM, Ming Lei wrote:
> On Tue, Oct 25, 2022 at 12:11:39PM -0600, Jens Axboe wrote:
>> On 10/24/22 6:55 PM, Ming Lei wrote:
>>> @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
>>>  	if (!percpu_ref_tryget(&q->q_usage_counter))
>>>  		return;
>>>  
>>> -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
>>> +	/*
>>> +	 * Before walking tags, we must ensure any submit started before
>>> +	 * the current time has finished. Since the submit uses srcu or rcu,
>>> +	 * wait for a synchronization point to ensure all running submits
>>> +	 * have finished
>>> +	 */
>>> +	blk_mq_wait_quiesce_done(q);
>>
>> I'm a little worried about this bit - so we'll basically do a sync RCU
>> every time the timeout timer runs... Depending on machine load, that
>> can take a long time.
> 
> Yeah, the per-queue timeout timer is never canceled after request is
> completed, so most of times the timeout work does nothing.

Yep, it just keeps going, that's the point of the rolling timeout timer.

> Can we run the sync RCU only if there is timed out request found? Then
> the wait is only needed in case that timeout handling is required. Also
> sync rcu is already done in some driver's ->timeout().

That was going to be my suggestion, if it can get done only for when
there's actually a potential timeout candidate, then that would be
orders of magnitude better.

-- 
Jens Axboe

Re: [PATCH] blk-mq: avoid double ->queue_rq() because of early timeout

[Jens Axboe]

On 10/25/22 6:21 PM, Ming Lei wrote:
> On Tue, Oct 25, 2022 at 12:11:39PM -0600, Jens Axboe wrote:
>> On 10/24/22 6:55 PM, Ming Lei wrote:
>>> @@ -1593,10 +1598,18 @@ static void blk_mq_timeout_work(struct work_struct *work)
>>>  	if (!percpu_ref_tryget(&q->q_usage_counter))
>>>  		return;
>>>  
>>> -	blk_mq_queue_tag_busy_iter(q, blk_mq_check_expired, &next);
>>> +	/*
>>> +	 * Before walking tags, we must ensure any submit started before
>>> +	 * the current time has finished. Since the submit uses srcu or rcu,
>>> +	 * wait for a synchronization point to ensure all running submits
>>> +	 * have finished
>>> +	 */
>>> +	blk_mq_wait_quiesce_done(q);
>>
>> I'm a little worried about this bit - so we'll basically do a sync RCU
>> every time the timeout timer runs... Depending on machine load, that
>> can take a long time.
> 
> Yeah, the per-queue timeout timer is never canceled after request is
> completed, so most of times the timeout work does nothing.

Yep, it just keeps going, that's the point of the rolling timeout timer.

> Can we run the sync RCU only if there is timed out request found? Then
> the wait is only needed in case that timeout handling is required. Also
> sync rcu is already done in some driver's ->timeout().

That was going to be my suggestion, if it can get done only for when
there's actually a potential timeout candidate, then that would be
orders of magnitude better.

-- 
Jens Axboe
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization