1. Patchew
  2. QEMU
  3. [RFC PATCH 0/3] block: Synchronous bdrv_*() from coroutine in different AioContext
  4. View patch

[RFC PATCH 3/3] block: Assert we're running in the right thread

Kevin Wolf posted 3 patches 5 years, 9 months ago
Maintainers: Kevin Wolf <kwolf@redhat.com>, Max Reitz <mreitz@redhat.com>, Fam Zheng <fam@euphon.net>, Stefan Hajnoczi <stefanha@redhat.com>
[RFC PATCH 3/3] block: Assert we're running in the right thread
Posted by Kevin Wolf 5 years, 9 months ago 
tracked_request_begin() is called for most I/O operations, so it's a
good place to assert that we're indeed running in the home thread of the
node's AioContext.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/io.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/block/io.c b/block/io.c
index 7808e8bdc0..924bf5ba46 100644
--- a/block/io.c
+++ b/block/io.c
@@ -695,14 +695,17 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
                                   uint64_t bytes,
                                   enum BdrvTrackedRequestType type)
 {
+    Coroutine *self = qemu_coroutine_self();
+
     assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
+    assert(bs->aio_context == qemu_coroutine_get_aio_context(self));
 
     *req = (BdrvTrackedRequest){
         .bs = bs,
         .offset         = offset,
         .bytes          = bytes,
         .type           = type,
-        .co             = qemu_coroutine_self(),
+        .co             = self,
         .serialising    = false,
         .overlap_offset = offset,
         .overlap_bytes  = bytes,
-- 
2.25.3
Re: [RFC PATCH 3/3] block: Assert we're running in the right thread
Posted by Stefan Reiter 5 years, 9 months ago 
On 5/12/20 4:43 PM, Kevin Wolf wrote:
> tracked_request_begin() is called for most I/O operations, so it's a
> good place to assert that we're indeed running in the home thread of the
> node's AioContext.
> 

Is this patch supposed to be always correct or only together with nr. 2?

I changed our code to call bdrv_flush_all from the main AIO context and 
it certainly works just fine (even without this series, so I suppose 
that would be the 'correct' way to fix it you mention on the cover), 
though of course it trips this assert without patch 2.

> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/io.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 7808e8bdc0..924bf5ba46 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -695,14 +695,17 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
>                                     uint64_t bytes,
>                                     enum BdrvTrackedRequestType type)
>   {
> +    Coroutine *self = qemu_coroutine_self();
> +
>       assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
> +    assert(bs->aio_context == qemu_coroutine_get_aio_context(self));
>   
>       *req = (BdrvTrackedRequest){
>           .bs = bs,
>           .offset         = offset,
>           .bytes          = bytes,
>           .type           = type,
> -        .co             = qemu_coroutine_self(),
> +        .co             = self,
>           .serialising    = false,
>           .overlap_offset = offset,
>           .overlap_bytes  = bytes,
>
Re: [RFC PATCH 3/3] block: Assert we're running in the right thread
Posted by Kevin Wolf 5 years, 9 months ago 
Am 14.05.2020 um 15:52 hat Stefan Reiter geschrieben:
> On 5/12/20 4:43 PM, Kevin Wolf wrote:
> > tracked_request_begin() is called for most I/O operations, so it's a
> > good place to assert that we're indeed running in the home thread of the
> > node's AioContext.
> > 
> 
> Is this patch supposed to be always correct or only together with nr. 2?
> 
> I changed our code to call bdrv_flush_all from the main AIO context and it
> certainly works just fine (even without this series, so I suppose that would
> be the 'correct' way to fix it you mention on the cover), though of course
> it trips this assert without patch 2.

Yes, I think this is a basic assumption that should always be true.
This series shouldn't fix anything for upstream QEMU (at least I'm not
aware of anything that needs it), so the assertion could be added even
without the other patches.

In fact, I'm currently thinking that committing just patch 1 (because
it's a nice cleanup anyway) and patch 3 (because it will let us know
when we mess up) might make sense.

Kevin

> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >   block/io.c | 5 ++++-
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/block/io.c b/block/io.c
> > index 7808e8bdc0..924bf5ba46 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -695,14 +695,17 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
> >                                     uint64_t bytes,
> >                                     enum BdrvTrackedRequestType type)
> >   {
> > +    Coroutine *self = qemu_coroutine_self();
> > +
> >       assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
> > +    assert(bs->aio_context == qemu_coroutine_get_aio_context(self));
> >       *req = (BdrvTrackedRequest){
> >           .bs = bs,
> >           .offset         = offset,
> >           .bytes          = bytes,
> >           .type           = type,
> > -        .co             = qemu_coroutine_self(),
> > +        .co             = self,
> >           .serialising    = false,
> >           .overlap_offset = offset,
> >           .overlap_bytes  = bytes,
> > 
>
Re: [RFC PATCH 3/3] block: Assert we're running in the right thread
Posted by Philippe Mathieu-Daudé 5 years, 8 months ago 
On 5/14/20 4:30 PM, Kevin Wolf wrote:
> Am 14.05.2020 um 15:52 hat Stefan Reiter geschrieben:
>> On 5/12/20 4:43 PM, Kevin Wolf wrote:
>>> tracked_request_begin() is called for most I/O operations, so it's a
>>> good place to assert that we're indeed running in the home thread of the
>>> node's AioContext.
>>>
>>
>> Is this patch supposed to be always correct or only together with nr. 2?
>>
>> I changed our code to call bdrv_flush_all from the main AIO context and it
>> certainly works just fine (even without this series, so I suppose that would
>> be the 'correct' way to fix it you mention on the cover), though of course
>> it trips this assert without patch 2.
> 
> Yes, I think this is a basic assumption that should always be true.
> This series shouldn't fix anything for upstream QEMU (at least I'm not
> aware of anything that needs it), so the assertion could be added even
> without the other patches.
> 
> In fact, I'm currently thinking that committing just patch 1 (because
> it's a nice cleanup anyway) and patch 3 (because it will let us know
> when we mess up) might make sense.

FWIW applying 1+3 as no-RFC LGTM.

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> 
> Kevin
> 
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>    block/io.c | 5 ++++-
>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/io.c b/block/io.c
>>> index 7808e8bdc0..924bf5ba46 100644
>>> --- a/block/io.c
>>> +++ b/block/io.c
>>> @@ -695,14 +695,17 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
>>>                                      uint64_t bytes,
>>>                                      enum BdrvTrackedRequestType type)
>>>    {
>>> +    Coroutine *self = qemu_coroutine_self();
>>> +
>>>        assert(bytes <= INT64_MAX && offset <= INT64_MAX - bytes);
>>> +    assert(bs->aio_context == qemu_coroutine_get_aio_context(self));
>>>        *req = (BdrvTrackedRequest){
>>>            .bs = bs,
>>>            .offset         = offset,
>>>            .bytes          = bytes,
>>>            .type           = type,
>>> -        .co             = qemu_coroutine_self(),
>>> +        .co             = self,
>>>            .serialising    = false,
>>>            .overlap_offset = offset,
>>>            .overlap_bytes  = bytes,
>>>
>>
> 
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.