Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1524588 > unrolled thread

Re: [RFC PATCH] scsi: libsas: fix WARN on device removal

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] scsi: libsas: fix WARN on device removal John Garry <john.garry@huawei.com> - 2016-11-17 18:40 +0100
    Re: [RFC PATCH] scsi: libsas: fix WARN on device removal Dan Williams <dan.j.williams@intel.com> - 2016-11-18 03:00 +0100
      Re: [RFC PATCH] scsi: libsas: fix WARN on device removal John Garry <john.garry@huawei.com> - 2016-11-18 10:10 +0100
        Re: [RFC PATCH] scsi: libsas: fix WARN on device removal Dan Williams <dan.j.williams@intel.com> - 2016-11-18 20:10 +0100
          Re: [RFC PATCH] scsi: libsas: fix WARN on device removal John Garry <john.garry@huawei.com> - 2016-11-21 16:20 +0100
            Re: [RFC PATCH] scsi: libsas: fix WARN on device removal Dan Williams <dan.j.williams@intel.com> - 2016-11-21 18:20 +0100
              Re: [RFC PATCH] scsi: libsas: fix WARN on device removal John Garry <john.garry@huawei.com> - 2016-11-22 18:00 +0100
                Re: [RFC PATCH] scsi: libsas: fix WARN on device removal wangyijing <wangyijing@huawei.com> - 2016-11-23 02:20 +0100
    Re: [RFC PATCH] scsi: libsas: fix WARN on device removal "Martin K. Petersen" <martin.petersen@oracle.com> - 2016-11-18 03:00 +0100

#1524588 — Re: [RFC PATCH] scsi: libsas: fix WARN on device removal

On 11/11/2016 08:49, wangyijing wrote:
>>>> I have not seen the flutter issue. I am just trying to solve the horrible WARN dump.
>>>> However I do understand that there may be a issue related to how we queue the events; there was a recent attempt to fix this, but it came to nothing:
>>>> https://www.spinics.net/lists/linux-scsi/msg99991.html
>>>
>>> We found libsas hotplug several problems:
>>> 1. sysfs warning calltrace(like the case you found);
>>
>> Maybe you can then review my patch.
>
> I did it, I think your solution to fix the sysfs calltrace issue is ok, and what I worried about is we still need to fix
> the rest issues. So it's better if we could fix all issues one time.
>

@Maintainers, would you be willing to accept this patch as an interim 
fix for the dastardly WARN while we try to fix the flutter issue?

>>
>>> 2. hot-add and hot-remove work events may process out of order;
>>> 3. in some extreme cases, libsas may miss some events, if the same event is still pending in workqueue.
>>>
>>
>> Can you tell me how to recreate #2 and #3?
>
> Qilin Chen and Yousong He help me to reproduce it, I told them to reply this mail to tell you the test steps.
> Some tests we did is make sas phy link flutter, so hardware would post phy down and phy up events sequentially.
>
> 1. scsi host workqueue receive phy down and phy up events.                                             in process                 new added
> 2. sas_deform_port would post a new destruct event to scsi host workqueue, so things in workqueue like [phy down-----phy up -----destruct]
>
> So the phy down logic is separated by phy up, and it's not atomic, not safe, something unexpected would happen.
>
> For case 3, we make hardware burst post lots pair of phy up and phy down events, so if libsas is processing the phy up event, the next
> phy up event can not queue to scsi host workqueue again, it will lost, it's not we expect.
>
>>
>>> It's a complex issue, we posted two patches, try to fix these issues, but now few people are interested in it  :(
>>>
>>
>> IIRC, you sent as RFC and got a "reviewed-by" from Hannes, so I'm not sure what else you want. BTW, I thought that the changes were quite drastic.
>
> I agree, the changes seems something drastic. But I think current libsas hotplug framework has a big flaw.
>
>>
>> John
>>
>>>>
>>>>>
>>>>> Alternatively we need a mechanism to cancel in-flight port shutdown
>>>>> requests when we start re-attaching devices before queued port
>>>>> destruction events have run.
>>>>>
>>>>> .
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> linuxarm mailing list
>>>> linuxarm@huawei.com
>>>> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm
>>>>
>>>> .
>>>>
>>>
>>>
>>> .
>>>
>>
>>
>>
>> .
>>
>
>
> .
>

[toc] | [next] | [standalone]

#1524963

On Thu, Nov 17, 2016 at 7:23 AM, John Garry <john.garry@huawei.com> wrote:
> On 11/11/2016 08:49, wangyijing wrote:
>>>>>
>>>>> I have not seen the flutter issue. I am just trying to solve the
>>>>> horrible WARN dump.
>>>>> However I do understand that there may be a issue related to how we
>>>>> queue the events; there was a recent attempt to fix this, but it came to
>>>>> nothing:
>>>>> https://www.spinics.net/lists/linux-scsi/msg99991.html
>>>>
>>>>
>>>> We found libsas hotplug several problems:
>>>> 1. sysfs warning calltrace(like the case you found);
>>>
>>>
>>> Maybe you can then review my patch.
>>
>>
>> I did it, I think your solution to fix the sysfs calltrace issue is ok,
>> and what I worried about is we still need to fix
>> the rest issues. So it's better if we could fix all issues one time.
>>
>
> @Maintainers, would you be willing to accept this patch as an interim fix
> for the dastardly WARN while we try to fix the flutter issue?

To me this adds a bug to quiet a benign, albeit noisy, warning.

[toc] | [prev] | [next] | [standalone]

#1525101

On 18/11/2016 01:53, Dan Williams wrote:
> On Thu, Nov 17, 2016 at 7:23 AM, John Garry <john.garry@huawei.com> wrote:
>> On 11/11/2016 08:49, wangyijing wrote:
>>>>>>
>>>>>> I have not seen the flutter issue. I am just trying to solve the
>>>>>> horrible WARN dump.
>>>>>> However I do understand that there may be a issue related to how we
>>>>>> queue the events; there was a recent attempt to fix this, but it came to
>>>>>> nothing:
>>>>>> https://www.spinics.net/lists/linux-scsi/msg99991.html
>>>>>
>>>>>
>>>>> We found libsas hotplug several problems:
>>>>> 1. sysfs warning calltrace(like the case you found);
>>>>
>>>>
>>>> Maybe you can then review my patch.
>>>
>>>
>>> I did it, I think your solution to fix the sysfs calltrace issue is ok,
>>> and what I worried about is we still need to fix
>>> the rest issues. So it's better if we could fix all issues one time.
>>>
>>
>> @Maintainers, would you be willing to accept this patch as an interim fix
>> for the dastardly WARN while we try to fix the flutter issue?
>
> To me this adds a bug to quiet a benign, albeit noisy, warning.
>

What is the bug which is being added?

And it's a very noisy warning, as in 6K lines on the console when an 
expander is unplugged.

> .
>

[toc] | [prev] | [next] | [standalone]

#1525656

On Fri, Nov 18, 2016 at 1:00 AM, John Garry <john.garry@huawei.com> wrote:
> On 18/11/2016 01:53, Dan Williams wrote:
>>
>> On Thu, Nov 17, 2016 at 7:23 AM, John Garry <john.garry@huawei.com> wrote:
>>>
>>> On 11/11/2016 08:49, wangyijing wrote:
>>>>>>>
>>>>>>>
>>>>>>> I have not seen the flutter issue. I am just trying to solve the
>>>>>>> horrible WARN dump.
>>>>>>> However I do understand that there may be a issue related to how we
>>>>>>> queue the events; there was a recent attempt to fix this, but it came
>>>>>>> to
>>>>>>> nothing:
>>>>>>> https://www.spinics.net/lists/linux-scsi/msg99991.html
>>>>>>
>>>>>>
>>>>>>
>>>>>> We found libsas hotplug several problems:
>>>>>> 1. sysfs warning calltrace(like the case you found);
>>>>>
>>>>>
>>>>>
>>>>> Maybe you can then review my patch.
>>>>
>>>>
>>>>
>>>> I did it, I think your solution to fix the sysfs calltrace issue is ok,
>>>> and what I worried about is we still need to fix
>>>> the rest issues. So it's better if we could fix all issues one time.
>>>>
>>>
>>> @Maintainers, would you be willing to accept this patch as an interim fix
>>> for the dastardly WARN while we try to fix the flutter issue?
>>
>>
>> To me this adds a bug to quiet a benign, albeit noisy, warning.
>>
>
> What is the bug which is being added?

The bug where we queue a port teardown, but see a port formation event
in the meantime.

> And it's a very noisy warning, as in 6K lines on the console when an
> expander is unplugged.

Does something like this modulate the failure?

diff --git a/drivers/scsi/scsi_transport_sas.c
b/drivers/scsi/scsi_transport_sas.c            index
60b651bfaa01..11401e5c88ba 100644
                 --- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -262,9 +262,10 @@ static void sas_bsg_remove(struct Scsi_Host
*shost, struct sas_rphy *rphy
 {
        struct request_queue *q;

-       if (rphy)
+       if (rphy) {
                q = rphy->q;
-       else
+               rphy->q = NULL;
+       } else
                q = to_sas_host_attrs(shost)->q;

        if (!q)

[toc] | [prev] | [next] | [standalone]

#1526771

>>>> @Maintainers, would you be willing to accept this patch as an interim fix
>>>> for the dastardly WARN while we try to fix the flutter issue?
>>>
>>>
>>> To me this adds a bug to quiet a benign, albeit noisy, warning.
>>>
>>
>> What is the bug which is being added?
>
> The bug where we queue a port teardown, but see a port formation event
> in the meantime.

As I understand, this vulnerability already exists:
http://marc.info/?l=linux-scsi&m=143801026028006&w=2

I actually don't understand how libsas dealt with flutter (which I take 
to mean a burst of up and down events) before these changes, as it can 
only queue simultaneously one up and one down event per port. So, if we 
get a flutter, then the events are lost and we get indeterminate state.

>
>> And it's a very noisy warning, as in 6K lines on the console when an
>> expander is unplugged.
>
> Does something like this modulate the failure?
>
> diff --git a/drivers/scsi/scsi_transport_sas.c
> b/drivers/scsi/scsi_transport_sas.c            index
> 60b651bfaa01..11401e5c88ba 100644
>                  --- a/drivers/scsi/scsi_transport_sas.c
> +++ b/drivers/scsi/scsi_transport_sas.c
> @@ -262,9 +262,10 @@ static void sas_bsg_remove(struct Scsi_Host
> *shost, struct sas_rphy *rphy
>  {
>         struct request_queue *q;
>
> -       if (rphy)
> +       if (rphy) {
>                 q = rphy->q;
> -       else
> +               rphy->q = NULL;
> +       } else
>                 q = to_sas_host_attrs(shost)->q;
>
>         if (!q)
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1526933

On Mon, Nov 21, 2016 at 7:16 AM, John Garry <john.garry@huawei.com> wrote:
>>>>> @Maintainers, would you be willing to accept this patch as an interim
>>>>> fix
>>>>> for the dastardly WARN while we try to fix the flutter issue?
>>>>
>>>>
>>>>
>>>> To me this adds a bug to quiet a benign, albeit noisy, warning.
>>>>
>>>
>>> What is the bug which is being added?
>>
>>
>> The bug where we queue a port teardown, but see a port formation event
>> in the meantime.
>
>
> As I understand, this vulnerability already exists:
> http://marc.info/?l=linux-scsi&m=143801026028006&w=2
>
> I actually don't understand how libsas dealt with flutter (which I take to
> mean a burst of up and down events) before these changes, as it can only
> queue simultaneously one up and one down event per port. So, if we get a
> flutter, then the events are lost and we get indeterminate state.
>

The events are not lost.  The new problem this patch introduces is
delaying sas port deletion where it was previously immediate.  So now
we can get into a situation where the port has gone down and can start
processing a port up event before the previous deletion work has run.

>>
>>> And it's a very noisy warning, as in 6K lines on the console when an
>>> expander is unplugged.
>>
>>
>> Does something like this modulate the failure?

I'm curious if we simply need to fix the double deletion of the
sas_port bsg queue, could you try the changes below?

>>
>> diff --git a/drivers/scsi/scsi_transport_sas.c
>> b/drivers/scsi/scsi_transport_sas.c            index
>> 60b651bfaa01..11401e5c88ba 100644
>>                  --- a/drivers/scsi/scsi_transport_sas.c
>> +++ b/drivers/scsi/scsi_transport_sas.c
>> @@ -262,9 +262,10 @@ static void sas_bsg_remove(struct Scsi_Host
>> *shost, struct sas_rphy *rphy
>>  {
>>         struct request_queue *q;
>>
>> -       if (rphy)
>> +       if (rphy) {
>>                 q = rphy->q;
>> -       else
>> +               rphy->q = NULL;
>> +       } else
>>                 q = to_sas_host_attrs(shost)->q;
>>
>>         if (!q)
>>
>> .
>>
>
>

[toc] | [prev] | [next] | [standalone]

#1527708

On 21/11/2016 17:13, Dan Williams wrote:
> On Mon, Nov 21, 2016 at 7:16 AM, John Garry <john.garry@huawei.com> wrote:
>>>>>> @Maintainers, would you be willing to accept this patch as an interim
>>>>>> fix
>>>>>> for the dastardly WARN while we try to fix the flutter issue?
>>>>>
>>>>>
>>>>>
>>>>> To me this adds a bug to quiet a benign, albeit noisy, warning.
>>>>>
>>>>
>>>> What is the bug which is being added?
>>>
>>>
>>> The bug where we queue a port teardown, but see a port formation event
>>> in the meantime.
>>
>>
>> As I understand, this vulnerability already exists:
>> http://marc.info/?l=linux-scsi&m=143801026028006&w=2
>>
>> I actually don't understand how libsas dealt with flutter (which I take to
>> mean a burst of up and down events) before these changes, as it can only
>> queue simultaneously one up and one down event per port. So, if we get a
>> flutter, then the events are lost and we get indeterminate state.
>>
>
> The events are not lost.

In sas_queue_event(), if there is a particular event pending for a 
port/PHY, we cannot queue further same event types for that port/PHY. I 
think my colleagues found issue where we try to enqueue multiple 
complementary events.

> The new problem this patch introduces is
> delaying sas port deletion where it was previously immediate.  So now
> we can get into a situation where the port has gone down and can start
> processing a port up event before the previous deletion work has run.
>
>>>
>>>> And it's a very noisy warning, as in 6K lines on the console when an
>>>> expander is unplugged.
>>>
>>>
>>> Does something like this modulate the failure?
>
> I'm curious if we simply need to fix the double deletion of the
> sas_port bsg queue, could you try the changes below?
>

No, I just tested it on a root port and we get the same WARN.

>>>
>>> diff --git a/drivers/scsi/scsi_transport_sas.c
>>> b/drivers/scsi/scsi_transport_sas.c            index
>>> 60b651bfaa01..11401e5c88ba 100644
>>>                  --- a/drivers/scsi/scsi_transport_sas.c
>>> +++ b/drivers/scsi/scsi_transport_sas.c
>>> @@ -262,9 +262,10 @@ static void sas_bsg_remove(struct Scsi_Host
>>> *shost, struct sas_rphy *rphy
>>>  {
>>>         struct request_queue *q;
>>>
>>> -       if (rphy)
>>> +       if (rphy) {
>>>                 q = rphy->q;
>>> -       else
>>> +               rphy->q = NULL;
>>> +       } else
>>>                 q = to_sas_host_attrs(shost)->q;
>>>
>>>         if (!q)
>>>
>>> .
>>>
>>
>>
>
> .
>

[toc] | [prev] | [next] | [standalone]

#1528063

>>
>> The events are not lost.
> 
> In sas_queue_event(), if there is a particular event pending for a port/PHY, we cannot queue further same event types for that port/PHY. I think my colleagues found issue where we try to enqueue multiple complementary events.

Yes， we found this issue in our local tests.

> 
>> The new problem this patch introduces is
>> delaying sas port deletion where it was previously immediate.  So now
>> we can get into a situation where the port has gone down and can start
>> processing a port up event before the previous deletion work has run.
>>
>>>>
>>>>> And it's a very noisy warning, as in 6K lines on the console when an
>>>>> expander is unplugged.
>>>>
>>>>
>>>> Does something like this modulate the failure?
>>
>> I'm curious if we simply need to fix the double deletion of the
>> sas_port bsg queue, could you try the changes below?
>>
> 
> No, I just tested it on a root port and we get the same WARN.
> 
>>>>
>>>> diff --git a/drivers/scsi/scsi_transport_sas.c
>>>> b/drivers/scsi/scsi_transport_sas.c            index
>>>> 60b651bfaa01..11401e5c88ba 100644
>>>>                  --- a/drivers/scsi/scsi_transport_sas.c
>>>> +++ b/drivers/scsi/scsi_transport_sas.c
>>>> @@ -262,9 +262,10 @@ static void sas_bsg_remove(struct Scsi_Host
>>>> *shost, struct sas_rphy *rphy
>>>>  {
>>>>         struct request_queue *q;
>>>>
>>>> -       if (rphy)
>>>> +       if (rphy) {
>>>>                 q = rphy->q;
>>>> -       else
>>>> +               rphy->q = NULL;
>>>> +       } else
>>>>                 q = to_sas_host_attrs(shost)->q;
>>>>
>>>>         if (!q)
>>>>
>>>> .
>>>>
>>>
>>>
>>
>> .
>>
> 
> 
> 
> .
> 

[toc] | [prev] | [next] | [standalone]

#1524964

>>>>> "John" == John Garry <john.garry@huawei.com> writes:

John> @Maintainers, would you be willing to accept this patch as an
John> interim fix for the dastardly WARN while we try to fix the flutter
John> issue?

I'll defer to James since I don't have much libsas experience.

-- 
Martin K. Petersen	Oracle Linux Engineering

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web