mbox series

[v4,0/3] SCSI: Fix issues between removing device and error handle

Message ID 20240307144311.73735-1-haowenchao2@huawei.com
Headers show
Series SCSI: Fix issues between removing device and error handle | expand

Message

Wenchao Hao March 7, 2024, 2:43 p.m. UTC
I am testing SCSI error handle with my previous scsi_debug error
injection patches, and found some issues when removing device and
error handler happened together.

These issues are triggered because devices in removing would be skipped
when calling shost_for_each_device().

The issues are found:
1. statistic info printed at beginning of scsi_error_handler is wrong
2. device reset is not triggered

V4:
 - Remove the forth patch which fix IO hang when device removing
   becaust the issue is fixed by commit '6df0e077d76bd (scsi: core:
   Kick the requeue list after inserting when flushing)'

V3:
  - Update patch description
  - Update comments of functions added

V2:
  - Fix IO hang by run all devices' queue after error handler
  - Do not modify shost_for_each_device() directly but add a new
    helper to iterate devices but do not skip devices in removing

Wenchao Hao (3):
  scsi: core: Add new helper to iterate all devices of host
  scsi: scsi_error: Fix wrong statistic when print error info
  scsi: scsi_error: Fix device reset is not triggered

 drivers/scsi/scsi.c        | 46 ++++++++++++++++++++++++++------------
 drivers/scsi/scsi_error.c  |  4 ++--
 include/scsi/scsi_device.h | 25 ++++++++++++++++++---
 3 files changed, 56 insertions(+), 19 deletions(-)

Comments

Wenchao Hao March 26, 2024, 6:37 a.m. UTC | #1
On 2024/3/7 22:43, Wenchao Hao wrote:
> I am testing SCSI error handle with my previous scsi_debug error
> injection patches, and found some issues when removing device and
> error handler happened together.
> 
> These issues are triggered because devices in removing would be skipped
> when calling shost_for_each_device().
> 

Ping...

> The issues are found:
> 1. statistic info printed at beginning of scsi_error_handler is wrong
> 2. device reset is not triggered
> 
> V4:
>   - Remove the forth patch which fix IO hang when device removing
>     becaust the issue is fixed by commit '6df0e077d76bd (scsi: core:
>     Kick the requeue list after inserting when flushing)'
> 
> V3:
>    - Update patch description
>    - Update comments of functions added
> 
> V2:
>    - Fix IO hang by run all devices' queue after error handler
>    - Do not modify shost_for_each_device() directly but add a new
>      helper to iterate devices but do not skip devices in removing
> 
> Wenchao Hao (3):
>    scsi: core: Add new helper to iterate all devices of host
>    scsi: scsi_error: Fix wrong statistic when print error info
>    scsi: scsi_error: Fix device reset is not triggered
> 
>   drivers/scsi/scsi.c        | 46 ++++++++++++++++++++++++++------------
>   drivers/scsi/scsi_error.c  |  4 ++--
>   include/scsi/scsi_device.h | 25 ++++++++++++++++++---
>   3 files changed, 56 insertions(+), 19 deletions(-)
>
Wenchao Hao April 12, 2024, 2:07 a.m. UTC | #2
On 2024/3/7 22:43, Wenchao Hao wrote:
> I am testing SCSI error handle with my previous scsi_debug error
> injection patches, and found some issues when removing device and
> error handler happened together.
> 
> These issues are triggered because devices in removing would be skipped
> when calling shost_for_each_device().
> 

Friendly ping...


> The issues are found:
> 1. statistic info printed at beginning of scsi_error_handler is wrong
> 2. device reset is not triggered
> 
> V4:
>  - Remove the forth patch which fix IO hang when device removing
>    becaust the issue is fixed by commit '6df0e077d76bd (scsi: core:
>    Kick the requeue list after inserting when flushing)'
> 
> V3:
>   - Update patch description
>   - Update comments of functions added
> 
> V2:
>   - Fix IO hang by run all devices' queue after error handler
>   - Do not modify shost_for_each_device() directly but add a new
>     helper to iterate devices but do not skip devices in removing
> 
> Wenchao Hao (3):
>   scsi: core: Add new helper to iterate all devices of host
>   scsi: scsi_error: Fix wrong statistic when print error info
>   scsi: scsi_error: Fix device reset is not triggered
> 
>  drivers/scsi/scsi.c        | 46 ++++++++++++++++++++++++++------------
>  drivers/scsi/scsi_error.c  |  4 ++--
>  include/scsi/scsi_device.h | 25 ++++++++++++++++++---
>  3 files changed, 56 insertions(+), 19 deletions(-)
>
Markus Elfring April 15, 2024, 3:07 p.m. UTC | #3
> These issues are triggered because devices in removing would be skipped
> when calling shost_for_each_device().
…

How do you think about to add the tag “Fixes” to any of your patches?

Regards,
Markus
Wenchao Hao April 17, 2024, 3 p.m. UTC | #4
On 4/15/24 11:07 PM, Markus Elfring wrote:
> …
>> These issues are triggered because devices in removing would be skipped
>> when calling shost_for_each_device().
> …
> 
> How do you think about to add the tag “Fixes” to any of your patches?
> 

Thanks for your suggestion, but I don't know how to add this tag. Could you
please tell me how to do? 

I just added "fix" in my cover letter and some patch's subject.

> Regards,
> Markus
>
Julia Lawall April 17, 2024, 3:06 p.m. UTC | #5
On Wed, 17 Apr 2024, Wenchao Hao wrote:

> On 4/15/24 11:07 PM, Markus Elfring wrote:
> > …
> >> These issues are triggered because devices in removing would be skipped
> >> when calling shost_for_each_device().
> > …
> >
> > How do you think about to add the tag “Fixes” to any of your patches?
> >
>
> Thanks for your suggestion, but I don't know how to add this tag. Could you
> please tell me how to do?
>
> I just added "fix" in my cover letter and some patch's subject.

Search in the git history for other patches that contain Fixes:

Search in process/submitting-patches.rst for Fixes:

julia

>
> > Regards,
> > Markus
> >
>
>
>
Wenchao Hao April 17, 2024, 3:18 p.m. UTC | #6
On 4/17/24 11:06 PM, Julia Lawall wrote:
> 
> 
> On Wed, 17 Apr 2024, Wenchao Hao wrote:
> 
>> On 4/15/24 11:07 PM, Markus Elfring wrote:
>>> …
>>>> These issues are triggered because devices in removing would be skipped
>>>> when calling shost_for_each_device().
>>> …
>>>
>>> How do you think about to add the tag “Fixes” to any of your patches?
>>>
>>
>> Thanks for your suggestion, but I don't know how to add this tag. Could you
>> please tell me how to do?
>>
>> I just added "fix" in my cover letter and some patch's subject.
> 
> Search in the git history for other patches that contain Fixes:
> 
> Search in process/submitting-patches.rst for Fixes:
> 

Thank you, I would add this tag in my next post.

> julia
> 
>>
>>> Regards,
>>> Markus
>>>
>>
>>
>>
Wenchao Hao April 17, 2024, 3:29 p.m. UTC | #7
On 4/17/24 11:06 PM, Julia Lawall wrote:
> 
> 
> On Wed, 17 Apr 2024, Wenchao Hao wrote:
> 
>> On 4/15/24 11:07 PM, Markus Elfring wrote:
>>> …
>>>> These issues are triggered because devices in removing would be skipped
>>>> when calling shost_for_each_device().
>>> …
>>>
>>> How do you think about to add the tag “Fixes” to any of your patches?
>>>
>>
>> Thanks for your suggestion, but I don't know how to add this tag. Could you
>> please tell me how to do?
>>
>> I just added "fix" in my cover letter and some patch's subject.
> 
> Search in the git history for other patches that contain Fixes:
> 
> Search in process/submitting-patches.rst for Fixes:
> 

These issues are introduced at first version of git record, which is
1da177e4c3f4 ("Linux-2.6.12-rc2"). I think "Fixes" tag should not added
for this commit.

> julia
> 
>>
>>> Regards,
>>> Markus
>>>
>>
>>
>>
Markus Elfring April 17, 2024, 3:48 p.m. UTC | #8
>> Search in process/submitting-patches.rst for Fixes:
>
> These issues are introduced at first version of git record, which is
> 1da177e4c3f4 ("Linux-2.6.12-rc2"). I think "Fixes" tag should not added
> for this commit.

I suggest to take also another look at information in a table
like “Releases fixed in v6.8” from the article “Development statistics for 6.8”
by Jonathan Corbet.
https://lwn.net/Articles/964106/

Regards,
Markus
Wenchao Hao April 17, 2024, 4:39 p.m. UTC | #9
On 4/17/24 11:48 PM, Markus Elfring wrote:
>>> Search in process/submitting-patches.rst for Fixes:
>>
>> These issues are introduced at first version of git record, which is
>> 1da177e4c3f4 ("Linux-2.6.12-rc2"). I think "Fixes" tag should not added
>> for this commit.
> 
> I suggest to take also another look at information in a table
> like “Releases fixed in v6.8” from the article “Development statistics for 6.8”
> by Jonathan Corbet.
> https://lwn.net/Articles/964106/
> 

Thank you, I found some Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") in this article.
Then I searched in git log and found these "Fixes" tag too. 

> Regards,
> Markus