Currently, block layer APIs like block-backend.h contain a mix of
functions that are either running in the main loop and under the
BQL, or are thread-safe functions and run in iothreads performing I/O.
The functions running under BQL also take care of modifying the
block graph, by using drain and/or aio_context_acquire/release.
This makes it very confusing to understand where each function
runs, and what assumptions it provided with regards to thread
safety.

We call the functions running under BQL "graph API", and 
distinguish them from the thread-safe "I/O API".

The aim of this series is to split the relevant block headers in 
graph and I/O sub-headers. The division will be done in this way:
header.h will be split in header-graph.h, header-io.h and
header-common.h. The latter will just contain the data structures
needed by header-graph and header-io. header.h will remain for
legacy and to avoid changing all includes in all QEMU c files,
but will only include the two new headers.
Once we split all relevant headers, it will be much easier to see what
uses the AioContext lock and remove it, which is the overall main
goal of this and other series that I posted/will post.

RFC: I am happy to change all includes, if you think that it would
be better than leaving an empty header.h file.

The relevant headers I found so far are:
- block-backend.h
- block.h
- block_int.h
- backup-top.h and blockdev.h (but contain very few functions)
Once these are done, we would also need to audit the callback
offered by struct Jobdriver, BlockDevOps and BlockDriver.

Each function in the graph API will have an assertion, checking
that it is always running under BQL.
I/O functions are instead thread safe (or so should be), meaning
that they *can* run under BQL, but also in an iothread in another
AioContext. Therefore they do not provide any assertion, and
need to be audited manually to verify the correctness.
RFC: Any idea on how to guarantee I/O functions is welcome.

Adding assetions has helped finding a bug already, as shown in
patch 2.

RFC: Because this task is very time consuming and requires a lot
of auditing, this series only provide block-backend.h split,
to gather initial feedbacks.

Tested this series by running unit tests, qemu-iotests and qtests 
(x86_64)
Some functions in the graph API are used everywhere but not
properly tested. Therefore their assertion is never actually run in
the tests, so despite my very careful auditing, it is not impossible
to exclude that some will trigger while actually using QEMU.

Patch 1 introduces qemu_in_main_thread(), the function used in
all assertions. This had to be introduced otherwise all unit tests
would fail, since they run in the main loop but use the code in
stubs/iothread.c
Patch 2 fixes a bug that came up when auditing the code.
Patch 3 splits block-backend header, and to reduce the diff 
I moved the assertions in patch 4.

Next steps:
1) if this series gets positive comments, convert block.h and block_int.h
2) audit graph API and replace the AioContext lock with drains,
or remove them when not necessary (requires further discussion,
not necessary now).
3) [optional as it should be already the case] audit the I/O API
and check that thread safety is guaranteed

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>

Emanuele Giuseppe Esposito (4):
  main-loop.h: introduce qemu_in_main_thread()
  migration: block-dirty-bitmap: add missing qemu_mutex_lock_iothread
  include/sysemu/block-backend: split header into I/O and graph API
  block/block-backend.c: assertions for block-backend

 block/block-backend.c                 | 102 +++++++++-
 include/qemu/main-loop.h              |  13 ++
 include/sysemu/block-backend-common.h |  74 ++++++++
 include/sysemu/block-backend-graph.h  | 132 +++++++++++++
 include/sysemu/block-backend-io.h     | 123 ++++++++++++
 include/sysemu/block-backend.h        | 262 +-------------------------
 migration/block-dirty-bitmap.c        |   5 +-
 softmmu/cpus.c                        |   5 +
 softmmu/qdev-monitor.c                |   2 +
 stubs/iothread-lock.c                 |   5 +
 10 files changed, 459 insertions(+), 264 deletions(-)
 create mode 100644 include/sysemu/block-backend-common.h
 create mode 100644 include/sysemu/block-backend-graph.h
 create mode 100644 include/sysemu/block-backend-io.h

-- 
2.27.0

On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito wrote:
> Currently, block layer APIs like block-backend.h contain a mix of
> functions that are either running in the main loop and under the
> BQL, or are thread-safe functions and run in iothreads performing I/O.
> The functions running under BQL also take care of modifying the
> block graph, by using drain and/or aio_context_acquire/release.
> This makes it very confusing to understand where each function
> runs, and what assumptions it provided with regards to thread
> safety.
> 
> We call the functions running under BQL "graph API", and 
> distinguish them from the thread-safe "I/O API".

Maybe "BQL" is clearer than "graph" because not all functions classified
as "graph" need to traverse/modify the graph.

Stefan

On 13/09/21 15:10, Stefan Hajnoczi wrote:
> On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito wrote:
>> Currently, block layer APIs like block-backend.h contain a mix of
>> functions that are either running in the main loop and under the
>> BQL, or are thread-safe functions and run in iothreads performing I/O.
>> The functions running under BQL also take care of modifying the
>> block graph, by using drain and/or aio_context_acquire/release.
>> This makes it very confusing to understand where each function
>> runs, and what assumptions it provided with regards to thread
>> safety.
>>
>> We call the functions running under BQL "graph API", and
>> distinguish them from the thread-safe "I/O API".
> 
> Maybe "BQL" is clearer than "graph" because not all functions classified
> as "graph" need to traverse/modify the graph.

Bikeshedding, I like it! :)

... on the other hand, qemu-storage-daemon does not have a BQL (see 
patch 1); "graph API" functions run from the main (monitor) thread.

The characteristic of the "graph API" is that they affect global state, 
so another possibility could be "global state API".  But is there any 
global state apart from the BlockDriverState graph and the associated 
BlockBackends?

Paolo

On Wed, Sep 15, 2021 at 02:11:41PM +0200, Paolo Bonzini wrote:
> On 13/09/21 15:10, Stefan Hajnoczi wrote:
> > On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito wrote:
> > > Currently, block layer APIs like block-backend.h contain a mix of
> > > functions that are either running in the main loop and under the
> > > BQL, or are thread-safe functions and run in iothreads performing I/O.
> > > The functions running under BQL also take care of modifying the
> > > block graph, by using drain and/or aio_context_acquire/release.
> > > This makes it very confusing to understand where each function
> > > runs, and what assumptions it provided with regards to thread
> > > safety.
> > > 
> > > We call the functions running under BQL "graph API", and
> > > distinguish them from the thread-safe "I/O API".
> > 
> > Maybe "BQL" is clearer than "graph" because not all functions classified
> > as "graph" need to traverse/modify the graph.
> 
> Bikeshedding, I like it! :)
> 
> ... on the other hand, qemu-storage-daemon does not have a BQL (see patch
> 1); "graph API" functions run from the main (monitor) thread.
> 
> The characteristic of the "graph API" is that they affect global state, so
> another possibility could be "global state API".  But is there any global
> state apart from the BlockDriverState graph and the associated
> BlockBackends?

I would be happy with that name too.

Stefan

On 15/09/2021 16:43, Stefan Hajnoczi wrote:
> On Wed, Sep 15, 2021 at 02:11:41PM +0200, Paolo Bonzini wrote:
>> On 13/09/21 15:10, Stefan Hajnoczi wrote:
>>> On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito wrote:
>>>> Currently, block layer APIs like block-backend.h contain a mix of
>>>> functions that are either running in the main loop and under the
>>>> BQL, or are thread-safe functions and run in iothreads performing I/O.
>>>> The functions running under BQL also take care of modifying the
>>>> block graph, by using drain and/or aio_context_acquire/release.
>>>> This makes it very confusing to understand where each function
>>>> runs, and what assumptions it provided with regards to thread
>>>> safety.
>>>>
>>>> We call the functions running under BQL "graph API", and
>>>> distinguish them from the thread-safe "I/O API".
>>>
>>> Maybe "BQL" is clearer than "graph" because not all functions classified
>>> as "graph" need to traverse/modify the graph.
>>
>> Bikeshedding, I like it! :)
>>
>> ... on the other hand, qemu-storage-daemon does not have a BQL (see patch
>> 1); "graph API" functions run from the main (monitor) thread.
>>
>> The characteristic of the "graph API" is that they affect global state, so
>> another possibility could be "global state API".  But is there any global
>> state apart from the BlockDriverState graph and the associated
>> BlockBackends?
> 
> I would be happy with that name too.
> 

Sounds good to me too, thanks.
One more minor cosmetic thing: should I name the header 
block-backend-global-state.h or using block-backend-gs.h is 
straightforward enough?

Thank you,
Emanuele

I think either -global or -global-state.

Paolo


Il gio 16 set 2021, 16:03 Emanuele Giuseppe Esposito <eesposit@redhat.com>
ha scritto:

>
>
> On 15/09/2021 16:43, Stefan Hajnoczi wrote:
> > On Wed, Sep 15, 2021 at 02:11:41PM +0200, Paolo Bonzini wrote:
> >> On 13/09/21 15:10, Stefan Hajnoczi wrote:
> >>> On Wed, Sep 08, 2021 at 09:10:17AM -0400, Emanuele Giuseppe Esposito
> wrote:
> >>>> Currently, block layer APIs like block-backend.h contain a mix of
> >>>> functions that are either running in the main loop and under the
> >>>> BQL, or are thread-safe functions and run in iothreads performing I/O.
> >>>> The functions running under BQL also take care of modifying the
> >>>> block graph, by using drain and/or aio_context_acquire/release.
> >>>> This makes it very confusing to understand where each function
> >>>> runs, and what assumptions it provided with regards to thread
> >>>> safety.
> >>>>
> >>>> We call the functions running under BQL "graph API", and
> >>>> distinguish them from the thread-safe "I/O API".
> >>>
> >>> Maybe "BQL" is clearer than "graph" because not all functions
> classified
> >>> as "graph" need to traverse/modify the graph.
> >>
> >> Bikeshedding, I like it! :)
> >>
> >> ... on the other hand, qemu-storage-daemon does not have a BQL (see
> patch
> >> 1); "graph API" functions run from the main (monitor) thread.
> >>
> >> The characteristic of the "graph API" is that they affect global state,
> so
> >> another possibility could be "global state API".  But is there any
> global
> >> state apart from the BlockDriverState graph and the associated
> >> BlockBackends?
> >
> > I would be happy with that name too.
> >
>
> Sounds good to me too, thanks.
> One more minor cosmetic thing: should I name the header
> block-backend-global-state.h or using block-backend-gs.h is
> straightforward enough?
>
> Thank you,
> Emanuele
>
>