Linux-Block Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] blk-mq: put driver tag when this request is completed
@ 2020-06-29  9:47 Ming Lei
  2020-06-29 15:04 ` Christoph Hellwig
                   ` (2 more replies)
  0 siblings, 3 replies; 15+ messages in thread
From: Ming Lei @ 2020-06-29  9:47 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig

It is natural to release driver tag when this request is completed by
LLD or device since its purpose is for LLD use.

One big benefit is that the released tag can be re-used quicker since
bio_endio() may take too long.

Meantime we don't need to release driver tag for flush request.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-flush.c | 6 ------
 block/blk-mq.c    | 2 ++
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 15ae0155ec07..21108a550fbf 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -240,7 +240,6 @@ static void flush_end_io(struct request *flush_rq, blk_status_t error)
 		blk_mq_tag_set_rq(hctx, flush_rq->tag, fq->orig_rq);
 		flush_rq->tag = -1;
 	} else {
-		blk_mq_put_driver_tag(flush_rq);
 		flush_rq->internal_tag = -1;
 	}
 
@@ -341,11 +340,6 @@ static void mq_flush_data_end_io(struct request *rq, blk_status_t error)
 	unsigned long flags;
 	struct blk_flush_queue *fq = blk_get_flush_queue(q, ctx);
 
-	if (q->elevator) {
-		WARN_ON(rq->tag < 0);
-		blk_mq_put_driver_tag(rq);
-	}
-
 	/*
 	 * After populating an empty queue, kick it to avoid stall.  Read
 	 * the comment in flush_end_io().
diff --git a/block/blk-mq.c b/block/blk-mq.c
index b8738b3c6d06..d07e55455726 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -674,6 +674,8 @@ bool blk_mq_complete_request_remote(struct request *rq)
 {
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
 
+	blk_mq_put_driver_tag(rq);
+
 	/*
 	 * For a polled request, always complete locallly, it's pointless
 	 * to redirect the completion.
-- 
2.25.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-06-29  9:47 [PATCH] blk-mq: put driver tag when this request is completed Ming Lei
@ 2020-06-29 15:04 ` Christoph Hellwig
  2020-06-29 15:56 ` Jens Axboe
       [not found] ` <CGME20200701130104eucas1p1f8dcce58bf704b726aee1e89980fe19e@eucas1p1.samsung.com>
  2 siblings, 0 replies; 15+ messages in thread
From: Christoph Hellwig @ 2020-06-29 15:04 UTC (permalink / raw)
  To: Ming Lei; +Cc: Jens Axboe, linux-block, Christoph Hellwig

On Mon, Jun 29, 2020 at 05:47:59PM +0800, Ming Lei wrote:
> It is natural to release driver tag when this request is completed by
> LLD or device since its purpose is for LLD use.
> 
> One big benefit is that the released tag can be re-used quicker since
> bio_endio() may take too long.
> 
> Meantime we don't need to release driver tag for flush request.


Reviewed-by: Christoph Hellwig <hch@lst.de>

A bunch of comments on the surrounding code from reviewing it, though:

 - blk_mq_put_driver_tag and __blk_mq_put_driver_tag can and should be
   moved to blk-mq.c and marked static in a follow on patch (and possibly
   merged as well)
 - The rq->internal_tag == BLK_MQ_NO_TAG check in shold probably be
   replaced with a !data->q->elevator check to make it more obvious
   that we are only putting the driver tag for the iosched case

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-06-29  9:47 [PATCH] blk-mq: put driver tag when this request is completed Ming Lei
  2020-06-29 15:04 ` Christoph Hellwig
@ 2020-06-29 15:56 ` Jens Axboe
       [not found] ` <CGME20200701130104eucas1p1f8dcce58bf704b726aee1e89980fe19e@eucas1p1.samsung.com>
  2 siblings, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2020-06-29 15:56 UTC (permalink / raw)
  To: Ming Lei; +Cc: linux-block, Christoph Hellwig

On 6/29/20 3:47 AM, Ming Lei wrote:
> It is natural to release driver tag when this request is completed by
> LLD or device since its purpose is for LLD use.
> 
> One big benefit is that the released tag can be re-used quicker since
> bio_endio() may take too long.
> 
> Meantime we don't need to release driver tag for flush request.

Applied, thanks.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
       [not found] ` <CGME20200701130104eucas1p1f8dcce58bf704b726aee1e89980fe19e@eucas1p1.samsung.com>
@ 2020-07-01 13:01   ` Marek Szyprowski
  2020-07-01 13:45     ` Ming Lei
  0 siblings, 1 reply; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-01 13:01 UTC (permalink / raw)
  To: Ming Lei, Jens Axboe
  Cc: linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi

On 29.06.2020 11:47, Ming Lei wrote:
> It is natural to release driver tag when this request is completed by
> LLD or device since its purpose is for LLD use.
>
> One big benefit is that the released tag can be re-used quicker since
> bio_endio() may take too long.
>
> Meantime we don't need to release driver tag for flush request.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Signed-off-by: Ming Lei <ming.lei@redhat.com>

This patch landed recently in linux-next as commit 36a3df5a4574. Sadly 
it causes a regression on one of my test systems (ARM 32bit, Samsung 
Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine 
and then after a few seconds every executed command hangs. No 
panic/ops/any other message. I will try to provide more information asap 
I find something to share. Simple reverting it in linux-next is not 
possible due to dependencies.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-01 13:01   ` Marek Szyprowski
@ 2020-07-01 13:45     ` Ming Lei
  2020-07-01 14:16       ` Marek Szyprowski
  0 siblings, 1 reply; 15+ messages in thread
From: Ming Lei @ 2020-07-01 13:45 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi Marek,

On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
> Hi
> 
> On 29.06.2020 11:47, Ming Lei wrote:
> > It is natural to release driver tag when this request is completed by
> > LLD or device since its purpose is for LLD use.
> >
> > One big benefit is that the released tag can be re-used quicker since
> > bio_endio() may take too long.
> >
> > Meantime we don't need to release driver tag for flush request.
> >
> > Cc: Christoph Hellwig <hch@lst.de>
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> 
> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly 
> it causes a regression on one of my test systems (ARM 32bit, Samsung 
> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine 
> and then after a few seconds every executed command hangs. No 
> panic/ops/any other message. I will try to provide more information asap 
> I find something to share. Simple reverting it in linux-next is not 
> possible due to dependencies.

What is the exact eMMC's driver code(include the host driver)?

The usual way for handling completion is that host driver notifies block
layer via blk_mq_complete_request() after the LLD specific handling for
this request is done.

However, there might be driver which may use rq->tag in its rq completion
handler. I will see if the special case can be dealt with once you share
the driver info.


Thanks,
Ming


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-01 13:45     ` Ming Lei
@ 2020-07-01 14:16       ` Marek Szyprowski
  2020-07-01 14:58         ` Marek Szyprowski
  2020-07-02  1:22         ` Ming Lei
  0 siblings, 2 replies; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-01 14:16 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi Ming,

On 01.07.2020 15:45, Ming Lei wrote:
> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>> On 29.06.2020 11:47, Ming Lei wrote:
>>> It is natural to release driver tag when this request is completed by
>>> LLD or device since its purpose is for LLD use.
>>>
>>> One big benefit is that the released tag can be re-used quicker since
>>> bio_endio() may take too long.
>>>
>>> Meantime we don't need to release driver tag for flush request.
>>>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>> and then after a few seconds every executed command hangs. No
>> panic/ops/any other message. I will try to provide more information asap
>> I find something to share. Simple reverting it in linux-next is not
>> possible due to dependencies.
> What is the exact eMMC's driver code(include the host driver)?

dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)

> The usual way for handling completion is that host driver notifies block
> layer via blk_mq_complete_request() after the LLD specific handling for
> this request is done.
>
> However, there might be driver which may use rq->tag in its rq completion
> handler. I will see if the special case can be dealt with once you share
> the driver info.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-01 14:16       ` Marek Szyprowski
@ 2020-07-01 14:58         ` Marek Szyprowski
  2020-07-02  1:22         ` Ming Lei
  1 sibling, 0 replies; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-01 14:58 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi,

On 01.07.2020 16:16, Marek Szyprowski wrote:
> On 01.07.2020 15:45, Ming Lei wrote:
>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>>> On 29.06.2020 11:47, Ming Lei wrote:
>>>> It is natural to release driver tag when this request is completed by
>>>> LLD or device since its purpose is for LLD use.
>>>>
>>>> One big benefit is that the released tag can be re-used quicker since
>>>> bio_endio() may take too long.
>>>>
>>>> Meantime we don't need to release driver tag for flush request.
>>>>
>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>>> and then after a few seconds every executed command hangs. No
>>> panic/ops/any other message. I will try to provide more information 
>>> asap
>>> I find something to share. Simple reverting it in linux-next is not
>>> possible due to dependencies.
>> What is the exact eMMC's driver code(include the host driver)?
>
> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
>
>> The usual way for handling completion is that host driver notifies block
>> layer via blk_mq_complete_request() after the LLD specific handling for
>> this request is done.
>>
>> However, there might be driver which may use rq->tag in its rq 
>> completion
>> handler. I will see if the special case can be dealt with once you share
>> the driver info.

One more information: revering 37f4a24c2469a10a4c16c641671bd766e276cf9f, 
723bf178f158abd1ce6069cb049581b3cb003aab and 
36a3df5a4574d5ddf59804fcd0c4e9654c514d9a on top of today's linux-next 
fixes the issues on the tested system, what shows that the bisect is 
indeed correct and there is no other hidden issue.

The most surprising thing to me is that other Exynos5422-based boards I 
have, which also use same eMMC controller, work fine...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-01 14:16       ` Marek Szyprowski
  2020-07-01 14:58         ` Marek Szyprowski
@ 2020-07-02  1:22         ` Ming Lei
  2020-07-02  5:03           ` Jens Axboe
  2020-07-02  8:04           ` Marek Szyprowski
  1 sibling, 2 replies; 15+ messages in thread
From: Ming Lei @ 2020-07-02  1:22 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
> Hi Ming,
> 
> On 01.07.2020 15:45, Ming Lei wrote:
> > On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
> >> On 29.06.2020 11:47, Ming Lei wrote:
> >>> It is natural to release driver tag when this request is completed by
> >>> LLD or device since its purpose is for LLD use.
> >>>
> >>> One big benefit is that the released tag can be re-used quicker since
> >>> bio_endio() may take too long.
> >>>
> >>> Meantime we don't need to release driver tag for flush request.
> >>>
> >>> Cc: Christoph Hellwig <hch@lst.de>
> >>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
> >> it causes a regression on one of my test systems (ARM 32bit, Samsung
> >> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
> >> and then after a few seconds every executed command hangs. No
> >> panic/ops/any other message. I will try to provide more information asap
> >> I find something to share. Simple reverting it in linux-next is not
> >> possible due to dependencies.
> > What is the exact eMMC's driver code(include the host driver)?
> 
> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)

Hi,

Just take a quick look at mmc code, there are only two req->tag
consumers:

1) cqhci_tag
cqhci_tag
	cqhci_request
		host->cqe_ops->cqe_request
			mmc_cqe_start_req
	cqhci_timeout

2) mmc_hsq_request
mmc_hsq_request
	host->cqe_ops->cqe_request
		mmc_cqe_start_req

mmc_cqe_start_req() is called before issuing this request to hardware,
so completion won't happen when the tag is used in mmc_cqe_start_req().

cqhci_timeout() may race with normal completion, however looks the
following code can handle the race correctly:

        spin_lock_irqsave(&cq_host->lock, flags);
        timed_out = slot->mrq == mrq;

So still no idea why the commit causes the trouble for mmc.

Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
And can you apply the following patch and see if warning can be
triggered?

diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
index 75934f3c117e..2cb49ecfbf34 100644
--- a/drivers/mmc/host/cqhci.c
+++ b/drivers/mmc/host/cqhci.c
@@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		goto out_unlock;
 	}
 
+	WARN_ON_ONCE(cq_host->slot[tag].mrq);
 	cq_host->slot[tag].mrq = mrq;
 	cq_host->slot[tag].flags = 0;
 
diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
index a5e05ed0fda3..11a4c1f3a970 100644
--- a/drivers/mmc/host/mmc_hsq.c
+++ b/drivers/mmc/host/mmc_hsq.c
@@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
 		return -EBUSY;
 	}
 
+	WARN_ON_ONCE(hsq->slot[tag].mrq);
 	hsq->slot[tag].mrq = mrq;
 
 	/*

Thanks, 
Ming


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02  1:22         ` Ming Lei
@ 2020-07-02  5:03           ` Jens Axboe
  2020-07-02  8:04           ` Marek Szyprowski
  1 sibling, 0 replies; 15+ messages in thread
From: Jens Axboe @ 2020-07-02  5:03 UTC (permalink / raw)
  To: Ming Lei, Marek Szyprowski
  Cc: linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

On 7/1/20 7:22 PM, Ming Lei wrote:
> On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
>> Hi Ming,
>>
>> On 01.07.2020 15:45, Ming Lei wrote:
>>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>>>> On 29.06.2020 11:47, Ming Lei wrote:
>>>>> It is natural to release driver tag when this request is completed by
>>>>> LLD or device since its purpose is for LLD use.
>>>>>
>>>>> One big benefit is that the released tag can be re-used quicker since
>>>>> bio_endio() may take too long.
>>>>>
>>>>> Meantime we don't need to release driver tag for flush request.
>>>>>
>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>>>> and then after a few seconds every executed command hangs. No
>>>> panic/ops/any other message. I will try to provide more information asap
>>>> I find something to share. Simple reverting it in linux-next is not
>>>> possible due to dependencies.
>>> What is the exact eMMC's driver code(include the host driver)?
>>
>> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
> 
> Hi,
> 
> Just take a quick look at mmc code, there are only two req->tag
> consumers:

Just a heads up that I've queued up a revert of the offending commits.
It's not just this issue, there's also another one reported with
swapping.

-- 
Jens Axboe


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02  1:22         ` Ming Lei
  2020-07-02  5:03           ` Jens Axboe
@ 2020-07-02  8:04           ` Marek Szyprowski
  2020-07-02  9:23             ` Ming Lei
  1 sibling, 1 reply; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-02  8:04 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi Ming,

On 02.07.2020 03:22, Ming Lei wrote:
> On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
>> On 01.07.2020 15:45, Ming Lei wrote:
>>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>>>> On 29.06.2020 11:47, Ming Lei wrote:
>>>>> It is natural to release driver tag when this request is completed by
>>>>> LLD or device since its purpose is for LLD use.
>>>>>
>>>>> One big benefit is that the released tag can be re-used quicker since
>>>>> bio_endio() may take too long.
>>>>>
>>>>> Meantime we don't need to release driver tag for flush request.
>>>>>
>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>>>> and then after a few seconds every executed command hangs. No
>>>> panic/ops/any other message. I will try to provide more information asap
>>>> I find something to share. Simple reverting it in linux-next is not
>>>> possible due to dependencies.
>>> What is the exact eMMC's driver code(include the host driver)?
>> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
> Hi,
>
> Just take a quick look at mmc code, there are only two req->tag
> consumers:
>
> 1) cqhci_tag
> cqhci_tag
> 	cqhci_request
> 		host->cqe_ops->cqe_request
> 			mmc_cqe_start_req
> 	cqhci_timeout
>
> 2) mmc_hsq_request
> mmc_hsq_request
> 	host->cqe_ops->cqe_request
> 		mmc_cqe_start_req
>
> mmc_cqe_start_req() is called before issuing this request to hardware,
> so completion won't happen when the tag is used in mmc_cqe_start_req().
>
> cqhci_timeout() may race with normal completion, however looks the
> following code can handle the race correctly:
>
>          spin_lock_irqsave(&cq_host->lock, flags);
>          timed_out = slot->mrq == mrq;
>
> So still no idea why the commit causes the trouble for mmc.
>
> Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
> And can you apply the following patch and see if warning can be
> triggered?
>
> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> index 75934f3c117e..2cb49ecfbf34 100644
> --- a/drivers/mmc/host/cqhci.c
> +++ b/drivers/mmc/host/cqhci.c
> @@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>   		goto out_unlock;
>   	}
>   
> +	WARN_ON_ONCE(cq_host->slot[tag].mrq);
>   	cq_host->slot[tag].mrq = mrq;
>   	cq_host->slot[tag].flags = 0;
>   
> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> index a5e05ed0fda3..11a4c1f3a970 100644
> --- a/drivers/mmc/host/mmc_hsq.c
> +++ b/drivers/mmc/host/mmc_hsq.c
> @@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
>   		return -EBUSY;
>   	}
>   
> +	WARN_ON_ONCE(hsq->slot[tag].mrq);
>   	hsq->slot[tag].mrq = mrq;
>   
>   	/*

None of the above is even compiled for my system (I'm using 
arm/exynos_defconfig), so this must be something else.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02  8:04           ` Marek Szyprowski
@ 2020-07-02  9:23             ` Ming Lei
  2020-07-02 10:19               ` Marek Szyprowski
  0 siblings, 1 reply; 15+ messages in thread
From: Ming Lei @ 2020-07-02  9:23 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

On Thu, Jul 02, 2020 at 10:04:38AM +0200, Marek Szyprowski wrote:
> Hi Ming,
> 
> On 02.07.2020 03:22, Ming Lei wrote:
> > On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
> >> On 01.07.2020 15:45, Ming Lei wrote:
> >>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
> >>>> On 29.06.2020 11:47, Ming Lei wrote:
> >>>>> It is natural to release driver tag when this request is completed by
> >>>>> LLD or device since its purpose is for LLD use.
> >>>>>
> >>>>> One big benefit is that the released tag can be re-used quicker since
> >>>>> bio_endio() may take too long.
> >>>>>
> >>>>> Meantime we don't need to release driver tag for flush request.
> >>>>>
> >>>>> Cc: Christoph Hellwig <hch@lst.de>
> >>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
> >>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
> >>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
> >>>> and then after a few seconds every executed command hangs. No
> >>>> panic/ops/any other message. I will try to provide more information asap
> >>>> I find something to share. Simple reverting it in linux-next is not
> >>>> possible due to dependencies.
> >>> What is the exact eMMC's driver code(include the host driver)?
> >> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
> > Hi,
> >
> > Just take a quick look at mmc code, there are only two req->tag
> > consumers:
> >
> > 1) cqhci_tag
> > cqhci_tag
> > 	cqhci_request
> > 		host->cqe_ops->cqe_request
> > 			mmc_cqe_start_req
> > 	cqhci_timeout
> >
> > 2) mmc_hsq_request
> > mmc_hsq_request
> > 	host->cqe_ops->cqe_request
> > 		mmc_cqe_start_req
> >
> > mmc_cqe_start_req() is called before issuing this request to hardware,
> > so completion won't happen when the tag is used in mmc_cqe_start_req().
> >
> > cqhci_timeout() may race with normal completion, however looks the
> > following code can handle the race correctly:
> >
> >          spin_lock_irqsave(&cq_host->lock, flags);
> >          timed_out = slot->mrq == mrq;
> >
> > So still no idea why the commit causes the trouble for mmc.
> >
> > Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
> > And can you apply the following patch and see if warning can be
> > triggered?
> >
> > diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> > index 75934f3c117e..2cb49ecfbf34 100644
> > --- a/drivers/mmc/host/cqhci.c
> > +++ b/drivers/mmc/host/cqhci.c
> > @@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >   		goto out_unlock;
> >   	}
> >   
> > +	WARN_ON_ONCE(cq_host->slot[tag].mrq);
> >   	cq_host->slot[tag].mrq = mrq;
> >   	cq_host->slot[tag].flags = 0;
> >   
> > diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> > index a5e05ed0fda3..11a4c1f3a970 100644
> > --- a/drivers/mmc/host/mmc_hsq.c
> > +++ b/drivers/mmc/host/mmc_hsq.c
> > @@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >   		return -EBUSY;
> >   	}
> >   
> > +	WARN_ON_ONCE(hsq->slot[tag].mrq);
> >   	hsq->slot[tag].mrq = mrq;
> >   
> >   	/*
> 
> None of the above is even compiled for my system (I'm using 
> arm/exynos_defconfig), so this must be something else.

Hello Marek,

Or can you boot the system with one workable disk(usb, nand, ...)?
then run some IO test on this eMMC, and collect debugfs log via the following
command after the hang is triggered:

(cd /sys/kernel/debug/block/$MMC && find . -type f -exec grep -aH . {} \;)

$MMC is this mmc disk name.

Thanks,
Ming


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02  9:23             ` Ming Lei
@ 2020-07-02 10:19               ` Marek Szyprowski
  2020-07-02 11:48                 ` Ming Lei
  0 siblings, 1 reply; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-02 10:19 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

On 02.07.2020 11:23, Ming Lei wrote:
> On Thu, Jul 02, 2020 at 10:04:38AM +0200, Marek Szyprowski wrote:
>> On 02.07.2020 03:22, Ming Lei wrote:
>>> On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
>>>> On 01.07.2020 15:45, Ming Lei wrote:
>>>>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>>>>>> On 29.06.2020 11:47, Ming Lei wrote:
>>>>>>> It is natural to release driver tag when this request is completed by
>>>>>>> LLD or device since its purpose is for LLD use.
>>>>>>>
>>>>>>> One big benefit is that the released tag can be re-used quicker since
>>>>>>> bio_endio() may take too long.
>>>>>>>
>>>>>>> Meantime we don't need to release driver tag for flush request.
>>>>>>>
>>>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>>>>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>>>>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>>>>>> and then after a few seconds every executed command hangs. No
>>>>>> panic/ops/any other message. I will try to provide more information asap
>>>>>> I find something to share. Simple reverting it in linux-next is not
>>>>>> possible due to dependencies.
>>>>> What is the exact eMMC's driver code(include the host driver)?
>>>> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
>>> Hi,
>>>
>>> Just take a quick look at mmc code, there are only two req->tag
>>> consumers:
>>>
>>> 1) cqhci_tag
>>> cqhci_tag
>>> 	cqhci_request
>>> 		host->cqe_ops->cqe_request
>>> 			mmc_cqe_start_req
>>> 	cqhci_timeout
>>>
>>> 2) mmc_hsq_request
>>> mmc_hsq_request
>>> 	host->cqe_ops->cqe_request
>>> 		mmc_cqe_start_req
>>>
>>> mmc_cqe_start_req() is called before issuing this request to hardware,
>>> so completion won't happen when the tag is used in mmc_cqe_start_req().
>>>
>>> cqhci_timeout() may race with normal completion, however looks the
>>> following code can handle the race correctly:
>>>
>>>           spin_lock_irqsave(&cq_host->lock, flags);
>>>           timed_out = slot->mrq == mrq;
>>>
>>> So still no idea why the commit causes the trouble for mmc.
>>>
>>> Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
>>> And can you apply the following patch and see if warning can be
>>> triggered?
>>>
>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
>>> index 75934f3c117e..2cb49ecfbf34 100644
>>> --- a/drivers/mmc/host/cqhci.c
>>> +++ b/drivers/mmc/host/cqhci.c
>>> @@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>    		goto out_unlock;
>>>    	}
>>>    
>>> +	WARN_ON_ONCE(cq_host->slot[tag].mrq);
>>>    	cq_host->slot[tag].mrq = mrq;
>>>    	cq_host->slot[tag].flags = 0;
>>>    
>>> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
>>> index a5e05ed0fda3..11a4c1f3a970 100644
>>> --- a/drivers/mmc/host/mmc_hsq.c
>>> +++ b/drivers/mmc/host/mmc_hsq.c
>>> @@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>    		return -EBUSY;
>>>    	}
>>>    
>>> +	WARN_ON_ONCE(hsq->slot[tag].mrq);
>>>    	hsq->slot[tag].mrq = mrq;
>>>    
>>>    	/*
>> None of the above is even compiled for my system (I'm using
>> arm/exynos_defconfig), so this must be something else.
> Hello Marek,
>
> Or can you boot the system with one workable disk(usb, nand, ...)?
> then run some IO test on this eMMC, and collect debugfs log via the following
> command after the hang is triggered:
>
> (cd /sys/kernel/debug/block/$MMC && find . -type f -exec grep -aH . {} \;)
>
> $MMC is this mmc disk name.

I have no physical access to this system now, but I managed to run a 
screen session just after the boot to have more than one shell. Here is 
the dump when the other shell locks:

# (cd /sys/kernel/debug/block/mmcblk0 && find . -type f -exec grep -aH . 
{} \;)
./hctx0/cpu7/completed:372 0
./hctx0/cpu7/merged:0
./hctx0/cpu7/dispatched:372 0
./hctx0/cpu6/completed:385 0
./hctx0/cpu6/merged:0
./hctx0/cpu6/dispatched:385 0
./hctx0/cpu5/completed:525 0
./hctx0/cpu5/merged:0
./hctx0/cpu5/dispatched:526 0
./hctx0/cpu4/completed:707 0
./hctx0/cpu4/merged:0
./hctx0/cpu4/dispatched:708 0
./hctx0/cpu3/completed:97 1
./hctx0/cpu3/merged:0
./hctx0/cpu3/dispatched:97 2
./hctx0/cpu2/completed:180 0
./hctx0/cpu2/merged:0
./hctx0/cpu2/dispatched:181 0
./hctx0/cpu1/completed:257 0
./hctx0/cpu1/merged:0
./hctx0/cpu1/dispatched:257 2
./hctx0/cpu0/completed:45 0
./hctx0/cpu0/merged:0
./hctx0/cpu0/dispatched:45 0
./hctx0/type:default
./hctx0/dispatch_busy:0
./hctx0/active:0
./hctx0/run:2126
./hctx0/queued:2575
./hctx0/dispatched:       0     6
./hctx0/dispatched:       1     2050
./hctx0/dispatched:       2     150
./hctx0/dispatched:       4     11
./hctx0/dispatched:       8     1
./hctx0/dispatched:      16     1
./hctx0/dispatched:      32+    1
./hctx0/io_poll:considered=0
./hctx0/io_poll:invoked=0
./hctx0/io_poll:success=0
./hctx0/sched_tags_bitmap:00000000: 0020 0000 1000 0000 0000 7000 0800 0000
./hctx0/sched_tags:nr_tags=128
./hctx0/sched_tags:nr_reserved_tags=0
./hctx0/sched_tags:active_queues=0
./hctx0/sched_tags:bitmap_tags:
./hctx0/sched_tags:depth=128
./hctx0/sched_tags:busy=6
./hctx0/sched_tags:cleared=40
./hctx0/sched_tags:bits_per_word=32
./hctx0/sched_tags:map_nr=4
./hctx0/sched_tags:alloc_hint={126, 78, 98, 35, 12, 83, 11, 21}
./hctx0/sched_tags:wake_batch=8
./hctx0/sched_tags:wake_index=0
./hctx0/sched_tags:ws_active=0
./hctx0/sched_tags:ws={
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:     {.wait_cnt=8, .wait=inactive},
./hctx0/sched_tags:}
./hctx0/sched_tags:round_robin=0
./hctx0/sched_tags:min_shallow_depth=4294967295
./hctx0/tags_bitmap:00000000: ffff ffff ffff ffff
./hctx0/tags:nr_tags=64
./hctx0/tags:nr_reserved_tags=0
./hctx0/tags:active_queues=0
./hctx0/tags:bitmap_tags:
./hctx0/tags:depth=64
./hctx0/tags:busy=64
./hctx0/tags:cleared=0
./hctx0/tags:bits_per_word=16
./hctx0/tags:map_nr=4
./hctx0/tags:alloc_hint={14, 0, 0, 0, 0, 0, 13, 22}
./hctx0/tags:wake_batch=8
./hctx0/tags:wake_index=0
./hctx0/tags:ws_active=0
./hctx0/tags:ws={
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:   {.wait_cnt=8, .wait=inactive},
./hctx0/tags:}
./hctx0/tags:round_robin=0
./hctx0/tags:min_shallow_depth=4294967295
./hctx0/ctx_map:00000000: 00
./hctx0/dispatch:35e4bf48 {.op=FLUSH, .cmd_flags=PREFLUSH, 
.rq_flags=FLUSH_SEQ, .state=idle, .tag=-1, .internal_tag=84}
./hctx0/flags:alloc_policy=FIFO SHOULD_MERGE|BLOCKING
./hctx0/state:SCHED_RESTART
./sched/starved:0
./sched/batching:1
./sched/write_fifo_list:ab51b2cc {.op=WRITE, 
.cmd_flags=META|PRIO|BACKGROUND, .rq_flags=ELVPRIV|IO_STAT|HASHED, 
.state=idle, .tag=-1, .internal_tag=85}
./sched/write_fifo_list:ac854e48 {.op=WRITE, 
.cmd_flags=META|PRIO|BACKGROUND, .rq_flags=ELVPRIV|IO_STAT|HASHED, 
.state=idle, .tag=-1, .internal_tag=86}
./sched/write_fifo_list:b12e714a {.op=WRITE, .cmd_flags=, 
.rq_flags=ELVPRIV|IO_STAT|HASHED, .state=idle, .tag=-1, .internal_tag=36}
./sched/write_fifo_list:99383a87 {.op=WRITE, .cmd_flags=SYNC, 
.rq_flags=ELVPRIV|IO_STAT|HASHED, .state=idle, .tag=-1, .internal_tag=99}
./sched/read_fifo_list:29365a9b {.op=READ, 
.cmd_flags=FAILFAST_DEV|FAILFAST_TRAag=-1, .internal_tag=13}AHEAD, 
.rq_flags=ELVPRIV|IO_STAT|HASHED, .state=idle, .ta
./write_hints:hint0: 0
./write_hints:hint1: 0
./write_hints:hint2: 0
./write_hints:hint3: 0
./write_hints:hint4: 0
./state:SAME_COMP|NONROT|IO_STAT|DISCARD|SECERASE|INIT_DONE|WC|FUA|REGISTERED
./pm_only:0
./poll_stat:read  (512 Bytes): samples=0
./poll_stat:write (512 Bytes): samples=0
./poll_stat:read  (1024 Bytes): samples=0
./poll_stat:write (1024 Bytes): samples=0
./poll_stat:read  (2048 Bytes): samples=0
./poll_stat:write (2048 Bytes): samples=0
./poll_stat:read  (4096 Bytes): samples=0
./poll_stat:write (4096 Bytes): samples=0
./poll_stat:read  (8192 Bytes): samples=0
./poll_stat:write (8192 Bytes): samples=0
./poll_stat:read  (16384 Bytes): samples=0
./poll_stat:write (16384 Bytes): samples=0
./poll_stat:read  (32768 Bytes): samples=0
./poll_stat:write (32768 Bytes): samples=0
./poll_stat:read  (65536 Bytes): samples=0
./poll_stat:write (65536 Bytes): samples=0
root@target:~#

I hope it helps.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02 10:19               ` Marek Szyprowski
@ 2020-07-02 11:48                 ` Ming Lei
  2020-07-02 12:12                   ` Marek Szyprowski
  0 siblings, 1 reply; 15+ messages in thread
From: Ming Lei @ 2020-07-02 11:48 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

On Thu, Jul 02, 2020 at 12:19:08PM +0200, Marek Szyprowski wrote:
> On 02.07.2020 11:23, Ming Lei wrote:
> > On Thu, Jul 02, 2020 at 10:04:38AM +0200, Marek Szyprowski wrote:
> >> On 02.07.2020 03:22, Ming Lei wrote:
> >>> On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
> >>>> On 01.07.2020 15:45, Ming Lei wrote:
> >>>>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
> >>>>>> On 29.06.2020 11:47, Ming Lei wrote:
> >>>>>>> It is natural to release driver tag when this request is completed by
> >>>>>>> LLD or device since its purpose is for LLD use.
> >>>>>>>
> >>>>>>> One big benefit is that the released tag can be re-used quicker since
> >>>>>>> bio_endio() may take too long.
> >>>>>>>
> >>>>>>> Meantime we don't need to release driver tag for flush request.
> >>>>>>>
> >>>>>>> Cc: Christoph Hellwig <hch@lst.de>
> >>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
> >>>>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
> >>>>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
> >>>>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
> >>>>>> and then after a few seconds every executed command hangs. No
> >>>>>> panic/ops/any other message. I will try to provide more information asap
> >>>>>> I find something to share. Simple reverting it in linux-next is not
> >>>>>> possible due to dependencies.
> >>>>> What is the exact eMMC's driver code(include the host driver)?
> >>>> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
> >>> Hi,
> >>>
> >>> Just take a quick look at mmc code, there are only two req->tag
> >>> consumers:
> >>>
> >>> 1) cqhci_tag
> >>> cqhci_tag
> >>> 	cqhci_request
> >>> 		host->cqe_ops->cqe_request
> >>> 			mmc_cqe_start_req
> >>> 	cqhci_timeout
> >>>
> >>> 2) mmc_hsq_request
> >>> mmc_hsq_request
> >>> 	host->cqe_ops->cqe_request
> >>> 		mmc_cqe_start_req
> >>>
> >>> mmc_cqe_start_req() is called before issuing this request to hardware,
> >>> so completion won't happen when the tag is used in mmc_cqe_start_req().
> >>>
> >>> cqhci_timeout() may race with normal completion, however looks the
> >>> following code can handle the race correctly:
> >>>
> >>>           spin_lock_irqsave(&cq_host->lock, flags);
> >>>           timed_out = slot->mrq == mrq;
> >>>
> >>> So still no idea why the commit causes the trouble for mmc.
> >>>
> >>> Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
> >>> And can you apply the following patch and see if warning can be
> >>> triggered?
> >>>
> >>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
> >>> index 75934f3c117e..2cb49ecfbf34 100644
> >>> --- a/drivers/mmc/host/cqhci.c
> >>> +++ b/drivers/mmc/host/cqhci.c
> >>> @@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>>    		goto out_unlock;
> >>>    	}
> >>>    
> >>> +	WARN_ON_ONCE(cq_host->slot[tag].mrq);
> >>>    	cq_host->slot[tag].mrq = mrq;
> >>>    	cq_host->slot[tag].flags = 0;
> >>>    
> >>> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
> >>> index a5e05ed0fda3..11a4c1f3a970 100644
> >>> --- a/drivers/mmc/host/mmc_hsq.c
> >>> +++ b/drivers/mmc/host/mmc_hsq.c
> >>> @@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >>>    		return -EBUSY;
> >>>    	}
> >>>    
> >>> +	WARN_ON_ONCE(hsq->slot[tag].mrq);
> >>>    	hsq->slot[tag].mrq = mrq;
> >>>    
> >>>    	/*
> >> None of the above is even compiled for my system (I'm using
> >> arm/exynos_defconfig), so this must be something else.
> > Hello Marek,
> >
> > Or can you boot the system with one workable disk(usb, nand, ...)?
> > then run some IO test on this eMMC, and collect debugfs log via the following
> > command after the hang is triggered:
> >
> > (cd /sys/kernel/debug/block/$MMC && find . -type f -exec grep -aH . {} \;)
> >
> > $MMC is this mmc disk name.
> 
> 
> I hope it helps.

It does help, :-)

Thanks for collecting the log, now I understood the reason: flush
request's driver tag is leaked in case that request isn't done via
blk_mq_complete_request(), such as freed via blk_mq_end_request()
directly.

Please try the following patch, which should have been one two-line
change if the driver tag cleanup patch isn't merged.

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ebab8f1044cb..7d62e9e5972e 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -532,6 +532,26 @@ void blk_mq_free_request(struct request *rq)
 }
 EXPORT_SYMBOL_GPL(blk_mq_free_request);
 
+static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx,
+		struct request *rq)
+{
+	blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag);
+	rq->tag = BLK_MQ_NO_TAG;
+
+	if (rq->rq_flags & RQF_MQ_INFLIGHT) {
+		rq->rq_flags &= ~RQF_MQ_INFLIGHT;
+		atomic_dec(&hctx->nr_active);
+	}
+}
+
+static inline void blk_mq_put_driver_tag(struct request *rq)
+{
+	if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG)
+		return;
+
+	__blk_mq_put_driver_tag(rq->mq_hctx, rq);
+}
+
 inline void __blk_mq_end_request(struct request *rq, blk_status_t error)
 {
 	u64 now = 0;
@@ -551,6 +571,7 @@ inline void __blk_mq_end_request(struct request *rq, blk_status_t error)
 
 	if (rq->end_io) {
 		rq_qos_done(rq->q, rq);
+		blk_mq_put_driver_tag(rq);
 		rq->end_io(rq, error);
 	} else {
 		blk_mq_free_request(rq);
@@ -862,26 +883,6 @@ static inline bool blk_mq_complete_need_ipi(struct request *rq)
 	return cpu_online(rq->mq_ctx->cpu);
 }
 
-static void __blk_mq_put_driver_tag(struct blk_mq_hw_ctx *hctx,
-		struct request *rq)
-{
-	blk_mq_put_tag(hctx->tags, rq->mq_ctx, rq->tag);
-	rq->tag = BLK_MQ_NO_TAG;
-
-	if (rq->rq_flags & RQF_MQ_INFLIGHT) {
-		rq->rq_flags &= ~RQF_MQ_INFLIGHT;
-		atomic_dec(&hctx->nr_active);
-	}
-}
-
-static inline void blk_mq_put_driver_tag(struct request *rq)
-{
-	if (rq->tag == BLK_MQ_NO_TAG || rq->internal_tag == BLK_MQ_NO_TAG)
-		return;
-
-	__blk_mq_put_driver_tag(rq->mq_hctx, rq);
-}
-
 bool blk_mq_complete_request_remote(struct request *rq)
 {
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
@@ -1185,9 +1186,10 @@ static bool blk_mq_check_expired(struct blk_mq_hw_ctx *hctx,
 	if (blk_mq_req_expired(rq, next))
 		blk_mq_rq_timed_out(rq, reserved);
 
-	if (is_flush_rq(rq, hctx))
+	if (is_flush_rq(rq, hctx)) {
+		blk_mq_put_driver_tag(rq);
 		rq->end_io(rq, 0);
-	else if (refcount_dec_and_test(&rq->ref))
+	} else if (refcount_dec_and_test(&rq->ref))
 		__blk_mq_free_request(rq);
 
 	return true;

Thanks,
Ming


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] blk-mq: put driver tag when this request is completed
  2020-07-02 11:48                 ` Ming Lei
@ 2020-07-02 12:12                   ` Marek Szyprowski
  0 siblings, 0 replies; 15+ messages in thread
From: Marek Szyprowski @ 2020-07-02 12:12 UTC (permalink / raw)
  To: Ming Lei
  Cc: Jens Axboe, linux-block, Christoph Hellwig, Bartlomiej Zolnierkiewicz

Hi Ming,

On 02.07.2020 13:48, Ming Lei wrote:
> On Thu, Jul 02, 2020 at 12:19:08PM +0200, Marek Szyprowski wrote:
>> On 02.07.2020 11:23, Ming Lei wrote:
>>> On Thu, Jul 02, 2020 at 10:04:38AM +0200, Marek Szyprowski wrote:
>>>> On 02.07.2020 03:22, Ming Lei wrote:
>>>>> On Wed, Jul 01, 2020 at 04:16:32PM +0200, Marek Szyprowski wrote:
>>>>>> On 01.07.2020 15:45, Ming Lei wrote:
>>>>>>> On Wed, Jul 01, 2020 at 03:01:03PM +0200, Marek Szyprowski wrote:
>>>>>>>> On 29.06.2020 11:47, Ming Lei wrote:
>>>>>>>>> It is natural to release driver tag when this request is completed by
>>>>>>>>> LLD or device since its purpose is for LLD use.
>>>>>>>>>
>>>>>>>>> One big benefit is that the released tag can be re-used quicker since
>>>>>>>>> bio_endio() may take too long.
>>>>>>>>>
>>>>>>>>> Meantime we don't need to release driver tag for flush request.
>>>>>>>>>
>>>>>>>>> Cc: Christoph Hellwig <hch@lst.de>
>>>>>>>>> Signed-off-by: Ming Lei <ming.lei@redhat.com>
>>>>>>>> This patch landed recently in linux-next as commit 36a3df5a4574. Sadly
>>>>>>>> it causes a regression on one of my test systems (ARM 32bit, Samsung
>>>>>>>> Exynos5422 SoC based Odroid XU3 board with eMMC). The system boots fine
>>>>>>>> and then after a few seconds every executed command hangs. No
>>>>>>>> panic/ops/any other message. I will try to provide more information asap
>>>>>>>> I find something to share. Simple reverting it in linux-next is not
>>>>>>>> possible due to dependencies.
>>>>>>> What is the exact eMMC's driver code(include the host driver)?
>>>>>> dwmmc-exynos (drivers/mmc/host/dw_mmc-exynos.c)
>>>>> Hi,
>>>>>
>>>>> Just take a quick look at mmc code, there are only two req->tag
>>>>> consumers:
>>>>>
>>>>> 1) cqhci_tag
>>>>> cqhci_tag
>>>>> 	cqhci_request
>>>>> 		host->cqe_ops->cqe_request
>>>>> 			mmc_cqe_start_req
>>>>> 	cqhci_timeout
>>>>>
>>>>> 2) mmc_hsq_request
>>>>> mmc_hsq_request
>>>>> 	host->cqe_ops->cqe_request
>>>>> 		mmc_cqe_start_req
>>>>>
>>>>> mmc_cqe_start_req() is called before issuing this request to hardware,
>>>>> so completion won't happen when the tag is used in mmc_cqe_start_req().
>>>>>
>>>>> cqhci_timeout() may race with normal completion, however looks the
>>>>> following code can handle the race correctly:
>>>>>
>>>>>            spin_lock_irqsave(&cq_host->lock, flags);
>>>>>            timed_out = slot->mrq == mrq;
>>>>>
>>>>> So still no idea why the commit causes the trouble for mmc.
>>>>>
>>>>> Do you know it is cqhci or mmc_hsh which works for dw_mmc-exynos?
>>>>> And can you apply the following patch and see if warning can be
>>>>> triggered?
>>>>>
>>>>> diff --git a/drivers/mmc/host/cqhci.c b/drivers/mmc/host/cqhci.c
>>>>> index 75934f3c117e..2cb49ecfbf34 100644
>>>>> --- a/drivers/mmc/host/cqhci.c
>>>>> +++ b/drivers/mmc/host/cqhci.c
>>>>> @@ -612,6 +612,7 @@ static int cqhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>>>     		goto out_unlock;
>>>>>     	}
>>>>>     
>>>>> +	WARN_ON_ONCE(cq_host->slot[tag].mrq);
>>>>>     	cq_host->slot[tag].mrq = mrq;
>>>>>     	cq_host->slot[tag].flags = 0;
>>>>>     
>>>>> diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
>>>>> index a5e05ed0fda3..11a4c1f3a970 100644
>>>>> --- a/drivers/mmc/host/mmc_hsq.c
>>>>> +++ b/drivers/mmc/host/mmc_hsq.c
>>>>> @@ -227,6 +227,7 @@ static int mmc_hsq_request(struct mmc_host *mmc, struct mmc_request *mrq)
>>>>>     		return -EBUSY;
>>>>>     	}
>>>>>     
>>>>> +	WARN_ON_ONCE(hsq->slot[tag].mrq);
>>>>>     	hsq->slot[tag].mrq = mrq;
>>>>>     
>>>>>     	/*
>>>> None of the above is even compiled for my system (I'm using
>>>> arm/exynos_defconfig), so this must be something else.
>>> Hello Marek,
>>>
>>> Or can you boot the system with one workable disk(usb, nand, ...)?
>>> then run some IO test on this eMMC, and collect debugfs log via the following
>>> command after the hang is triggered:
>>>
>>> (cd /sys/kernel/debug/block/$MMC && find . -type f -exec grep -aH . {} \;)
>>>
>>> $MMC is this mmc disk name.
>>
>> I hope it helps.
> It does help, :-)
>
> Thanks for collecting the log, now I understood the reason: flush
> request's driver tag is leaked in case that request isn't done via
> blk_mq_complete_request(), such as freed via blk_mq_end_request()
> directly.
>
> Please try the following patch, which should have been one two-line
> change if the driver tag cleanup patch isn't merged.


Yes, this fixes the issue on my test system! :)

Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

to the final patch.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH] blk-mq: put driver tag when this request is completed
@ 2020-07-06 14:40 Ming Lei
  0 siblings, 0 replies; 15+ messages in thread
From: Ming Lei @ 2020-07-06 14:40 UTC (permalink / raw)
  To: Jens Axboe; +Cc: linux-block, Ming Lei, Christoph Hellwig

It is natural to release driver tag when this request is completed by
LLD or device since its purpose is for LLD use.

One big benefit is that the released tag can be re-used quicker since
bio_endio() may take too long.

.complete() is usually called for notifying block layer that this request
is completed from LLD, and it is often the last thing done wrt. completion
from LLD viewpoint. Not see rq->tag is used in driver's complete() too.

Remove the warn in flush code because the driver tag should be released
in normal completion path, however we can't kill it because request may
be done directly via blk_mq_end_request(). Meantime not necessary to
check q->elevator cause blk_mq_put_driver_tag() has run the same check
already.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <ming.lei@redhat.com>
---
 block/blk-flush.c | 5 +----
 block/blk-mq.c    | 2 ++
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 15ae0155ec07..86a8b6e747df 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -341,10 +341,7 @@ static void mq_flush_data_end_io(struct request *rq, blk_status_t error)
 	unsigned long flags;
 	struct blk_flush_queue *fq = blk_get_flush_queue(q, ctx);
 
-	if (q->elevator) {
-		WARN_ON(rq->tag < 0);
-		blk_mq_put_driver_tag(rq);
-	}
+	blk_mq_put_driver_tag(rq);
 
 	/*
 	 * After populating an empty queue, kick it to avoid stall.  Read
diff --git a/block/blk-mq.c b/block/blk-mq.c
index abcf590f6238..117dec9abace 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -670,6 +670,8 @@ bool blk_mq_complete_request_remote(struct request *rq)
 {
 	WRITE_ONCE(rq->state, MQ_RQ_COMPLETE);
 
+	blk_mq_put_driver_tag(rq);
+
 	/*
 	 * For a polled request, always complete locallly, it's pointless
 	 * to redirect the completion.
-- 
2.25.2


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, back to index

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-29  9:47 [PATCH] blk-mq: put driver tag when this request is completed Ming Lei
2020-06-29 15:04 ` Christoph Hellwig
2020-06-29 15:56 ` Jens Axboe
     [not found] ` <CGME20200701130104eucas1p1f8dcce58bf704b726aee1e89980fe19e@eucas1p1.samsung.com>
2020-07-01 13:01   ` Marek Szyprowski
2020-07-01 13:45     ` Ming Lei
2020-07-01 14:16       ` Marek Szyprowski
2020-07-01 14:58         ` Marek Szyprowski
2020-07-02  1:22         ` Ming Lei
2020-07-02  5:03           ` Jens Axboe
2020-07-02  8:04           ` Marek Szyprowski
2020-07-02  9:23             ` Ming Lei
2020-07-02 10:19               ` Marek Szyprowski
2020-07-02 11:48                 ` Ming Lei
2020-07-02 12:12                   ` Marek Szyprowski
2020-07-06 14:40 Ming Lei

Linux-Block Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-block/0 linux-block/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-block linux-block/ https://lore.kernel.org/linux-block \
		linux-block@vger.kernel.org
	public-inbox-index linux-block

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-block


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git