2018-06-15 01:59:10

by jianchao.wang

[permalink] [raw]
Subject: [PATCH 1/2] block: export __blk_complete_request

After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
to complete the timed out request, however, for blk-legacy, the
'complete' is still marked, blk_complete_request will do nothing,
we export __blk_complete_request for LLDD to complete the request
in timeout path.

Signed-off-by: Jianchao Wang <[email protected]>
---
block/blk-softirq.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index 01e2b35..15c1f5e 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)

local_irq_restore(flags);
}
+EXPORT_SYMBOL(__blk_complete_request);

/**
* blk_complete_request - end I/O on a request
--
2.7.4



2018-06-15 01:57:25

by jianchao.wang

[permalink] [raw]
Subject: [PATCH 2/2] scsi_transport_fc: use __blk_complete_request in fc_bsg_job_timeout

bsg is based on blk-legacy, so we should use blk-legacy interface
here. On the other hand, for blk-legacy, the timed out request
has 'complete' mark, so use __blk_complete_request.

Signed-off-by: Jianchao Wang <[email protected]>
---
drivers/scsi/scsi_transport_fc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 1da3d71..1394810 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3592,7 +3592,7 @@ fc_bsg_job_timeout(struct request *req)

/* the blk_end_sync_io() doesn't check the error */
if (inflight)
- blk_mq_complete_request(req);
+ __blk_complete_request(req);
return BLK_EH_DONE;
}

--
2.7.4


2018-06-15 02:18:20

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 9:57 AM, Jianchao Wang
<[email protected]> wrote:
> After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
> to complete the timed out request, however, for blk-legacy, the
> 'complete' is still marked, blk_complete_request will do nothing,
> we export __blk_complete_request for LLDD to complete the request
> in timeout path.
>
> Signed-off-by: Jianchao Wang <[email protected]>
> ---
> block/blk-softirq.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> index 01e2b35..15c1f5e 100644
> --- a/block/blk-softirq.c
> +++ b/block/blk-softirq.c
> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>
> local_irq_restore(flags);
> }
> +EXPORT_SYMBOL(__blk_complete_request);
>
> /**
> * blk_complete_request - end I/O on a request
> --
> 2.7.4
>

Looks non-blk-mq timeout code need to convert to ref-counter
based approach too?

Thanks,
Ming Lei

2018-06-15 02:23:25

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Hi Ming

On 06/15/2018 10:17 AM, Ming Lei wrote:
> On Fri, Jun 15, 2018 at 9:57 AM, Jianchao Wang
> <[email protected]> wrote:
>> After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
>> to complete the timed out request, however, for blk-legacy, the
>> 'complete' is still marked, blk_complete_request will do nothing,
>> we export __blk_complete_request for LLDD to complete the request
>> in timeout path.
>>
>> Signed-off-by: Jianchao Wang <[email protected]>
>> ---
>> block/blk-softirq.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
>> index 01e2b35..15c1f5e 100644
>> --- a/block/blk-softirq.c
>> +++ b/block/blk-softirq.c
>> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>>
>> local_irq_restore(flags);
>> }
>> +EXPORT_SYMBOL(__blk_complete_request);
>>
>> /**
>> * blk_complete_request - end I/O on a request
>> --
>> 2.7.4
>>
>
> Looks non-blk-mq timeout code need to convert to ref-counter
> based approach too?

IMO, ref-counter is just to fix the blk-mq req life recycle issue.
It cannot replace the blk_mark_rq_complete which could avoid the race between
timeout and io completion path.
Or maybe my understanding is wrong ...

Thanks
Jianchao
>

2018-06-15 02:45:14

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request



On 06/15/2018 10:22 AM, jianchao.wang wrote:
> Hi Ming
>
> On 06/15/2018 10:17 AM, Ming Lei wrote:
>> On Fri, Jun 15, 2018 at 9:57 AM, Jianchao Wang
>> <[email protected]> wrote:
>>> After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
>>> to complete the timed out request, however, for blk-legacy, the
>>> 'complete' is still marked, blk_complete_request will do nothing,
>>> we export __blk_complete_request for LLDD to complete the request
>>> in timeout path.
>>>
>>> Signed-off-by: Jianchao Wang <[email protected]>
>>> ---
>>> block/blk-softirq.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
>>> index 01e2b35..15c1f5e 100644
>>> --- a/block/blk-softirq.c
>>> +++ b/block/blk-softirq.c
>>> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>>>
>>> local_irq_restore(flags);
>>> }
>>> +EXPORT_SYMBOL(__blk_complete_request);
>>>
>>> /**
>>> * blk_complete_request - end I/O on a request
>>> --
>>> 2.7.4
>>>
>>
>> Looks non-blk-mq timeout code need to convert to ref-counter
>> based approach too?
>
> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
> It cannot replace the blk_mark_rq_complete which could avoid the race between
> timeout and io completion path.

The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
request is still in abort or eh process. What if a completion irq come during that ?

> Or maybe my understanding is wrong ...
>
> Thanks
> Jianchao
>>
>

2018-06-15 02:50:13

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 10:22 AM, jianchao.wang
<[email protected]> wrote:
> Hi Ming
>
> On 06/15/2018 10:17 AM, Ming Lei wrote:
>> On Fri, Jun 15, 2018 at 9:57 AM, Jianchao Wang
>> <[email protected]> wrote:
>>> After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
>>> to complete the timed out request, however, for blk-legacy, the
>>> 'complete' is still marked, blk_complete_request will do nothing,
>>> we export __blk_complete_request for LLDD to complete the request
>>> in timeout path.
>>>
>>> Signed-off-by: Jianchao Wang <[email protected]>
>>> ---
>>> block/blk-softirq.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
>>> index 01e2b35..15c1f5e 100644
>>> --- a/block/blk-softirq.c
>>> +++ b/block/blk-softirq.c
>>> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>>>
>>> local_irq_restore(flags);
>>> }
>>> +EXPORT_SYMBOL(__blk_complete_request);
>>>
>>> /**
>>> * blk_complete_request - end I/O on a request
>>> --
>>> 2.7.4
>>>
>>
>> Looks non-blk-mq timeout code need to convert to ref-counter
>> based approach too?
>
> IMO, ref-counter is just to fix the blk-mq req life recycle issue.

Just thought of that, it is one blk-mq specific issue.

> It cannot replace the blk_mark_rq_complete which could avoid the race between
> timeout and io completion path.
> Or maybe my understanding is wrong ...

I didn't mean that this patch is unnecessary.

But the question is that given driver has to deal with race between timeout
and normal completion, why don't you follow blk-mq's way to move
the atomic state change into __blk_complete_request()?


Thanks,
Ming Lei

2018-06-15 02:56:47

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 10:44 AM, jianchao.wang
<[email protected]> wrote:
>
>
> On 06/15/2018 10:22 AM, jianchao.wang wrote:
>> Hi Ming
>>
>> On 06/15/2018 10:17 AM, Ming Lei wrote:
>>> On Fri, Jun 15, 2018 at 9:57 AM, Jianchao Wang
>>> <[email protected]> wrote:
>>>> After f6e7d48 (block: remove BLK_EH_HANDLED), LLDD is responsible
>>>> to complete the timed out request, however, for blk-legacy, the
>>>> 'complete' is still marked, blk_complete_request will do nothing,
>>>> we export __blk_complete_request for LLDD to complete the request
>>>> in timeout path.
>>>>
>>>> Signed-off-by: Jianchao Wang <[email protected]>
>>>> ---
>>>> block/blk-softirq.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
>>>> index 01e2b35..15c1f5e 100644
>>>> --- a/block/blk-softirq.c
>>>> +++ b/block/blk-softirq.c
>>>> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>>>>
>>>> local_irq_restore(flags);
>>>> }
>>>> +EXPORT_SYMBOL(__blk_complete_request);
>>>>
>>>> /**
>>>> * blk_complete_request - end I/O on a request
>>>> --
>>>> 2.7.4
>>>>
>>>
>>> Looks non-blk-mq timeout code need to convert to ref-counter
>>> based approach too?
>>
>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>> timeout and io completion path.
>
> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
> request is still in abort or eh process. What if a completion irq come during that ?

For blk-mq, it is avoided by the atomic state change in
__blk_mq_complete_request(),
that is why I mentioned the question in my last reply.

But what if the timed-out request has been freed by EH? Then seems
req's ref_counter
is still needed for non-mq?


Thanks,
Ming Lei

2018-06-15 03:04:49

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Hi Ming

Thanks for your kindly response.

On 06/15/2018 10:56 AM, Ming Lei wrote:
>>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>>> timeout and io completion path.
>> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
>> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
>> request is still in abort or eh process. What if a completion irq come during that ?
> For blk-mq, it is avoided by the atomic state change in
> __blk_mq_complete_request(),
> that is why I mentioned the question in my last reply.
>

but blk_mq_check_expired doesn't do that.
do I miss anything ?

> But what if the timed-out request has been freed by EH? Then seems
> req's ref_counter

Thanks
Jianchao

2018-06-15 03:21:23

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 11:04 AM, jianchao.wang
<[email protected]> wrote:
> Hi Ming
>
> Thanks for your kindly response.
>
> On 06/15/2018 10:56 AM, Ming Lei wrote:
>>>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>>>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>>>> timeout and io completion path.
>>> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
>>> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
>>> request is still in abort or eh process. What if a completion irq come during that ?
>> For blk-mq, it is avoided by the atomic state change in
>> __blk_mq_complete_request(),
>> that is why I mentioned the question in my last reply.
>>
>
> but blk_mq_check_expired doesn't do that.
> do I miss anything ?

Right, that is the difference between blk-mq and legacy now, then if scsi-mq
drivers can work well, they should work well with the following change in the
non-mq mode:

diff --git a/block/blk-timeout.c b/block/blk-timeout.c
index 4b8a48d48ba1..9fce09d55652 100644
--- a/block/blk-timeout.c
+++ b/block/blk-timeout.c
@@ -88,7 +88,6 @@ static void blk_rq_timed_out(struct request *req)
switch (ret) {
case BLK_EH_RESET_TIMER:
blk_add_timer(req);
- blk_clear_rq_complete(req);
break;
case BLK_EH_DONE:
/*
@@ -115,8 +114,7 @@ static void blk_rq_check_expired(struct request
*rq, unsigned long *next_timeout
/*
* Check if we raced with end io completion
*/
- if (!blk_mark_rq_complete(rq))
- blk_rq_timed_out(rq);
+ blk_rq_timed_out(rq);
} else if (!*next_set || time_after(*next_timeout, deadline)) {
*next_timeout = deadline;
*next_set = 1;


Thanks,
Ming Lei

2018-06-15 03:26:38

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Hi Ming

On 06/15/2018 11:20 AM, Ming Lei wrote:
> On Fri, Jun 15, 2018 at 11:04 AM, jianchao.wang
> <[email protected]> wrote:
>> Hi Ming
>>
>> Thanks for your kindly response.
>>
>> On 06/15/2018 10:56 AM, Ming Lei wrote:
>>>>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>>>>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>>>>> timeout and io completion path.
>>>> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
>>>> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
>>>> request is still in abort or eh process. What if a completion irq come during that ?
>>> For blk-mq, it is avoided by the atomic state change in
>>> __blk_mq_complete_request(),
>>> that is why I mentioned the question in my last reply.
>>>
>>
>> but blk_mq_check_expired doesn't do that.
>> do I miss anything ?
>
> Right, that is the difference between blk-mq and legacy now,

Sorry, I cannot follow your point.
blk_mq_check_expired doesn't do a atmoc state change from IN-FLIGHT to COMPLETE.
__blk_mq_complete_request could still proceed to complete a timed out request
which is in scsi abort or eh process. Is it really OK ?


Thanks
Jianchao


> then if scsi-mq
> drivers can work well, they should work well with the following change in the
> non-mq mode:
>
> diff --git a/block/blk-timeout.c b/block/blk-timeout.c
> index 4b8a48d48ba1..9fce09d55652 100644
> --- a/block/blk-timeout.c
> +++ b/block/blk-timeout.c
> @@ -88,7 +88,6 @@ static void blk_rq_timed_out(struct request *req)
> switch (ret) {
> case BLK_EH_RESET_TIMER:
> blk_add_timer(req);
> - blk_clear_rq_complete(req);
> break;
> case BLK_EH_DONE:
> /*
> @@ -115,8 +114,7 @@ static void blk_rq_check_expired(struct request
> *rq, unsigned long *next_timeout
> /*
> * Check if we raced with end io completion
> */
> - if (!blk_mark_rq_complete(rq))
> - blk_rq_timed_out(rq);
> + blk_rq_timed_out(rq);
> } else if (!*next_set || time_after(*next_timeout, deadline)) {
> *next_timeout = deadline;
> *next_set = 1;
>
>
> Thanks,
> Ming Lei
>

2018-06-15 04:04:18

by Ming Lei

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 11:26 AM, jianchao.wang
<[email protected]> wrote:
> Hi Ming
>
> On 06/15/2018 11:20 AM, Ming Lei wrote:
>> On Fri, Jun 15, 2018 at 11:04 AM, jianchao.wang
>> <[email protected]> wrote:
>>> Hi Ming
>>>
>>> Thanks for your kindly response.
>>>
>>> On 06/15/2018 10:56 AM, Ming Lei wrote:
>>>>>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>>>>>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>>>>>> timeout and io completion path.
>>>>> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
>>>>> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
>>>>> request is still in abort or eh process. What if a completion irq come during that ?
>>>> For blk-mq, it is avoided by the atomic state change in
>>>> __blk_mq_complete_request(),
>>>> that is why I mentioned the question in my last reply.
>>>>
>>>
>>> but blk_mq_check_expired doesn't do that.
>>> do I miss anything ?
>>
>> Right, that is the difference between blk-mq and legacy now,
>
> Sorry, I cannot follow your point.
> blk_mq_check_expired doesn't do a atmoc state change from IN-FLIGHT to COMPLETE.
> __blk_mq_complete_request could still proceed to complete a timed out request
> which is in scsi abort or eh process. Is it really OK ?

That is the idea of Christoph's patchset of 'complete requests from ->timeout',
then drivers need to cover race between timeout and normal completeion.

But at least the request won't be completed twice because of the atomic
state change in __blk_mq_complete_request().

So what is your real concern about blk-mq's timeout?

Thanks,
Ming

2018-06-15 05:16:35

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Hi Ming

Thanks for your kindly response

On 06/15/2018 12:03 PM, Ming Lei wrote:
> On Fri, Jun 15, 2018 at 11:26 AM, jianchao.wang
> <[email protected]> wrote:
>> Hi Ming
>>
>> On 06/15/2018 11:20 AM, Ming Lei wrote:
>>> On Fri, Jun 15, 2018 at 11:04 AM, jianchao.wang
>>> <[email protected]> wrote:
>>>> Hi Ming
>>>>
>>>> Thanks for your kindly response.
>>>>
>>>> On 06/15/2018 10:56 AM, Ming Lei wrote:
>>>>>>> IMO, ref-counter is just to fix the blk-mq req life recycle issue.
>>>>>>> It cannot replace the blk_mark_rq_complete which could avoid the race between
>>>>>>> timeout and io completion path.
>>>>>> The .timeout return BLK_EH_DONE doesn't always mean the request has been completed.
>>>>>> Such as scsi-mid layer, its .timeout callback return BLK_EH_DONE but the timed out
>>>>>> request is still in abort or eh process. What if a completion irq come during that ?
>>>>> For blk-mq, it is avoided by the atomic state change in
>>>>> __blk_mq_complete_request(),
>>>>> that is why I mentioned the question in my last reply.
>>>>>
>>>>
>>>> but blk_mq_check_expired doesn't do that.
>>>> do I miss anything ?
>>>
>>> Right, that is the difference between blk-mq and legacy now,
>>
>> Sorry, I cannot follow your point.
>> blk_mq_check_expired doesn't do a atmoc state change from IN-FLIGHT to COMPLETE.
>> __blk_mq_complete_request could still proceed to complete a timed out request
>> which is in scsi abort or eh process. Is it really OK ?
> > That is the idea of Christoph's patchset of 'complete requests from ->timeout',

Yes, I used to read that mail thread.

> then drivers need to cover race between timeout and normal completeion.
>
> But at least the request won't be completed twice because of the atomic
> state change in __blk_mq_complete_request().

Yes

>
> So what is your real concern about blk-mq's timeout?
I concern whether the current drivers have bee ready for taking this task currently.
At least, for scsi, if I try to trigger timeout and completion path concurrently, system would crash.
4.17.rc7 or 4.18 with a patch that change state in blk_mq_check_expired will survive.

Thanks
jianchao

>
> Thanks,
> Ming
>

2018-06-15 11:51:39

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

On Fri, Jun 15, 2018 at 11:20:40AM +0800, Ming Lei wrote:
> > but blk_mq_check_expired doesn't do that.
> > do I miss anything ?
>
> Right, that is the difference between blk-mq and legacy now, then if scsi-mq
> drivers can work well, they should work well with the following change in the
> non-mq mode:

We'll still need referene counting against reuse and/or premature
freeing of requests.

2018-06-19 14:02:40

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Does the patch below fix your FC issue?

---
From 5e5b4fc51c84a0f5c27f2f770be7a4eaed0f6e8c Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Tue, 19 Jun 2018 13:59:52 +0200
Subject: block: fix timeout changes for legacy request drivers

blk_mq_complete_request can only be called for blk-mq drivers, but when
removing the BLK_EH_HANDLED return value, two legacy request timeout
methods incorrectly got switched to call blk_mq_complete_request.
Call __blk_complete_request instead to reinstance the previous behavior.
For that __blk_complete_request needs to be exported.

Fixes: 1fc2b62e ("scsi_transport_fc: complete requests from ->timeout")
Fixes: 0df0bb08 ("null_blk: complete requests from ->timeout")
Reported-by: Jianchao Wang <[email protected]>
Signed-off-by: Christoph Hellwig <[email protected]>
---
block/blk-softirq.c | 1 +
drivers/block/null_blk.c | 2 +-
drivers/scsi/scsi_transport_fc.c | 2 +-
3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/blk-softirq.c b/block/blk-softirq.c
index 01e2b353a2b9..15c1f5e12eb8 100644
--- a/block/blk-softirq.c
+++ b/block/blk-softirq.c
@@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)

local_irq_restore(flags);
}
+EXPORT_SYMBOL(__blk_complete_request);

/**
* blk_complete_request - end I/O on a request
diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
index 7948049f6c43..042c778e5a4e 100644
--- a/drivers/block/null_blk.c
+++ b/drivers/block/null_blk.c
@@ -1365,7 +1365,7 @@ static blk_qc_t null_queue_bio(struct request_queue *q, struct bio *bio)
static enum blk_eh_timer_return null_rq_timed_out_fn(struct request *rq)
{
pr_info("null: rq %p timed out\n", rq);
- blk_mq_complete_request(rq);
+ __blk_complete_request(rq);
return BLK_EH_DONE;
}

diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 1da3d71e9f61..13948102ca29 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -3592,7 +3592,7 @@ fc_bsg_job_timeout(struct request *req)

/* the blk_end_sync_io() doesn't check the error */
if (inflight)
- blk_mq_complete_request(req);
+ __blk_complete_request(req);
return BLK_EH_DONE;
}

--
2.17.1


2018-06-19 14:53:28

by jianchao.wang

[permalink] [raw]
Subject: Re: [PATCH 1/2] block: export __blk_complete_request

Hi Christoph

Thanks for your kindly response.
The patch I posted ('scsi_transport_fc: use __blk_complete_request in fc_bsg_job_timeout')
is just based on code review. I don't have actual issue on it. :)

Thanks
Jianchao

On 06/19/2018 10:09 PM, Christoph Hellwig wrote:
> Does the patch below fix your FC issue?
>
> ---
>>From 5e5b4fc51c84a0f5c27f2f770be7a4eaed0f6e8c Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <[email protected]>
> Date: Tue, 19 Jun 2018 13:59:52 +0200
> Subject: block: fix timeout changes for legacy request drivers
>
> blk_mq_complete_request can only be called for blk-mq drivers, but when
> removing the BLK_EH_HANDLED return value, two legacy request timeout
> methods incorrectly got switched to call blk_mq_complete_request.
> Call __blk_complete_request instead to reinstance the previous behavior.
> For that __blk_complete_request needs to be exported.
>
> Fixes: 1fc2b62e ("scsi_transport_fc: complete requests from ->timeout")
> Fixes: 0df0bb08 ("null_blk: complete requests from ->timeout")
> Reported-by: Jianchao Wang <[email protected]>
> Signed-off-by: Christoph Hellwig <[email protected]>
> ---
> block/blk-softirq.c | 1 +
> drivers/block/null_blk.c | 2 +-
> drivers/scsi/scsi_transport_fc.c | 2 +-
> 3 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/block/blk-softirq.c b/block/blk-softirq.c
> index 01e2b353a2b9..15c1f5e12eb8 100644
> --- a/block/blk-softirq.c
> +++ b/block/blk-softirq.c
> @@ -144,6 +144,7 @@ void __blk_complete_request(struct request *req)
>
> local_irq_restore(flags);
> }
> +EXPORT_SYMBOL(__blk_complete_request);
>
> /**
> * blk_complete_request - end I/O on a request
> diff --git a/drivers/block/null_blk.c b/drivers/block/null_blk.c
> index 7948049f6c43..042c778e5a4e 100644
> --- a/drivers/block/null_blk.c
> +++ b/drivers/block/null_blk.c
> @@ -1365,7 +1365,7 @@ static blk_qc_t null_queue_bio(struct request_queue *q, struct bio *bio)
> static enum blk_eh_timer_return null_rq_timed_out_fn(struct request *rq)
> {
> pr_info("null: rq %p timed out\n", rq);
> - blk_mq_complete_request(rq);
> + __blk_complete_request(rq);
> return BLK_EH_DONE;
> }
>
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 1da3d71e9f61..13948102ca29 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -3592,7 +3592,7 @@ fc_bsg_job_timeout(struct request *req)
>
> /* the blk_end_sync_io() doesn't check the error */
> if (inflight)
> - blk_mq_complete_request(req);
> + __blk_complete_request(req);
> return BLK_EH_DONE;
> }
>
>