[Qemu-devel] [PATCH 0/3] ahci: fix completion race condition

John Snow posted 3 patches 7 years, 5 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20180531004323.4611-1-jsnow@redhat.com
Test checkpatch passed
Test docker-mingw@fedora passed
Test docker-quick@centos7 passed
Test s390x passed
hw/ide/ahci.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
[Qemu-devel] [PATCH 0/3] ahci: fix completion race condition
Posted by John Snow 7 years, 5 months ago
Commit d759c951f changed the main thread lock release/reacquisition,
and in so doing apparently jostled loose a race condition in the AHCI
code.

Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
little trivial fixes.

This might be sufficient to fix the bug as reported at
https://bugs.launchpad.net/qemu/+bug/1769189
but the nature of the timing changes make it difficult to confirm,
so I am posting this patchset for the reporters to help test.

John Snow (3):
  ahci: trim signatures on raise/lower
  ahci: fix PxCI register race
  ahci: don't schedule unnecessary BH

 hw/ide/ahci.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

-- 
2.14.3


Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] ahci: fix completion race condition
Posted by Stefan Hajnoczi 7 years, 5 months ago
On Thu, May 31, 2018 at 1:43 AM, John Snow <jsnow@redhat.com> wrote:
> Commit d759c951f changed the main thread lock release/reacquisition,
> and in so doing apparently jostled loose a race condition in the AHCI
> code.
>
> Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
> little trivial fixes.
>
> This might be sufficient to fix the bug as reported at
> https://bugs.launchpad.net/qemu/+bug/1769189
> but the nature of the timing changes make it difficult to confirm,
> so I am posting this patchset for the reporters to help test.
>
> John Snow (3):
>   ahci: trim signatures on raise/lower
>   ahci: fix PxCI register race
>   ahci: don't schedule unnecessary BH
>
>  hw/ide/ahci.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Nice find!

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH 0/3] ahci: fix completion race condition
Posted by Bruce Rogers 7 years, 5 months ago
>>> On 5/30/2018 at 6:43 PM, John Snow <jsnow@redhat.com> wrote:
> Commit d759c951f changed the main thread lock release/reacquisition,
> and in so doing apparently jostled loose a race condition in the AHCI
> code.
> 
> Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
> little trivial fixes.
> 
> This might be sufficient to fix the bug as reported at
> https://bugs.launchpad.net/qemu/+bug/1769189 
> but the nature of the timing changes make it difficult to confirm,
> so I am posting this patchset for the reporters to help test.
> 
> John Snow (3):
>   ahci: trim signatures on raise/lower
>   ahci: fix PxCI register race
>   ahci: don't schedule unnecessary BH
> 
>  hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑
>  1 file changed, 11 insertions(+), 13 deletions(‑)
> 
> ‑‑ 
> 2.14.3

In my case, I applied these 3 patches on top of v2.12 qemu under which
I can fairly reliably reproduce Windows10 disk delays. With these patches,
after quite a number of attempts, I no longer can reproduce the failure
case, so from my perspective it solves the issue.

Thanks!

Bruce

Re: [Qemu-devel] [PATCH 0/3] ahci: fix completion race condition
Posted by Philippe Mathieu-Daudé 7 years, 5 months ago
Hi Bruce,

On 05/31/2018 09:21 AM, Bruce Rogers wrote:
>>>> On 5/30/2018 at 6:43 PM, John Snow <jsnow@redhat.com> wrote:
>> Commit d759c951f changed the main thread lock release/reacquisition,
>> and in so doing apparently jostled loose a race condition in the AHCI
>> code.
>>
>> Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
>> little trivial fixes.
>>
>> This might be sufficient to fix the bug as reported at
>> https://bugs.launchpad.net/qemu/+bug/1769189 
>> but the nature of the timing changes make it difficult to confirm,
>> so I am posting this patchset for the reporters to help test.
>>
>> John Snow (3):
>>   ahci: trim signatures on raise/lower
>>   ahci: fix PxCI register race
>>   ahci: don't schedule unnecessary BH
>>
>>  hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑
>>  1 file changed, 11 insertions(+), 13 deletions(‑)
>>
>> ‑‑ 
>> 2.14.3
> 
> In my case, I applied these 3 patches on top of v2.12 qemu under which
> I can fairly reliably reproduce Windows10 disk delays. With these patches,
> after quite a number of attempts, I no longer can reproduce the failure
> case, so from my perspective it solves the issue.

Does that implicitly mean John can use your "Tested-by: Bruce Rogers
<brogers@suse.com>" tag?

Re: [Qemu-devel] [PATCH 0/3] ahci: fix completion race condition
Posted by Bruce Rogers 7 years, 5 months ago
>>> On 5/31/2018 at 7:06 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi Bruce,
> 
> On 05/31/2018 09:21 AM, Bruce Rogers wrote:
>>>>> On 5/30/2018 at 6:43 PM, John Snow <jsnow@redhat.com> wrote:
>>> Commit d759c951f changed the main thread lock release/reacquisition,
>>> and in so doing apparently jostled loose a race condition in the AHCI
>>> code.
>>>
>>> Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
>>> little trivial fixes.
>>>
>>> This might be sufficient to fix the bug as reported at
>>> https://bugs.launchpad.net/qemu/+bug/1769189 
>>> but the nature of the timing changes make it difficult to confirm,
>>> so I am posting this patchset for the reporters to help test.
>>>
>>> John Snow (3):
>>>   ahci: trim signatures on raise/lower
>>>   ahci: fix PxCI register race
>>>   ahci: don't schedule unnecessary BH
>>>
>>>  hw/ide/ahci.c | 24 +++++++++++‑‑‑‑‑‑‑‑‑‑‑‑‑
>>>  1 file changed, 11 insertions(+), 13 deletions(‑)
>>>
>>> ‑‑ 
>>> 2.14.3
>> 
>> In my case, I applied these 3 patches on top of v2.12 qemu under which
>> I can fairly reliably reproduce Windows10 disk delays. With these patches,
>> after quite a number of attempts, I no longer can reproduce the failure
>> case, so from my perspective it solves the issue.
> 
> Does that implicitly mean John can use your "Tested-by: Bruce Rogers
> <brogers@suse.com>" tag?

Yes. I guess I should have added that.
Bruce

Re: [Qemu-devel] [PATCH 0/3] ahci: fix completion race condition
Posted by John Snow 7 years, 5 months ago

On 05/30/2018 08:43 PM, John Snow wrote:
> Commit d759c951f changed the main thread lock release/reacquisition,
> and in so doing apparently jostled loose a race condition in the AHCI
> code.
> 
> Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
> little trivial fixes.
> 
> This might be sufficient to fix the bug as reported at
> https://bugs.launchpad.net/qemu/+bug/1769189
> but the nature of the timing changes make it difficult to confirm,
> so I am posting this patchset for the reporters to help test.
> 
> John Snow (3):
>   ahci: trim signatures on raise/lower
>   ahci: fix PxCI register race
>   ahci: don't schedule unnecessary BH
> 
>  hw/ide/ahci.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)
> 

Thanks for the testing and reviews, everyone!

Thanks, applied to my IDE tree:

https://github.com/jnsnow/qemu/commits/ide
https://github.com/jnsnow/qemu.git

--js

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] ahci: fix completion race condition
Posted by Stefan Hajnoczi 7 years, 4 months ago
On Thu, May 31, 2018 at 08:00:34PM -0400, John Snow wrote:
> On 05/30/2018 08:43 PM, John Snow wrote:
> > Commit d759c951f changed the main thread lock release/reacquisition,
> > and in so doing apparently jostled loose a race condition in the AHCI
> > code.
> > 
> > Patch 2 should be sufficient to fix this, and patches 1 and 3 are just
> > little trivial fixes.
> > 
> > This might be sufficient to fix the bug as reported at
> > https://bugs.launchpad.net/qemu/+bug/1769189
> > but the nature of the timing changes make it difficult to confirm,
> > so I am posting this patchset for the reporters to help test.
> > 
> > John Snow (3):
> >   ahci: trim signatures on raise/lower
> >   ahci: fix PxCI register race
> >   ahci: don't schedule unnecessary BH
> > 
> >  hw/ide/ahci.c | 24 +++++++++++-------------
> >  1 file changed, 11 insertions(+), 13 deletions(-)
> > 
> 
> Thanks for the testing and reviews, everyone!
> 
> Thanks, applied to my IDE tree:
> 
> https://github.com/jnsnow/qemu/commits/ide
> https://github.com/jnsnow/qemu.git

Also CCing stable.  This should go into QEMU 2.12.1.

Stefan