The qed block driver is one of the last remaining block drivers that use the
AIO callback style interfaces. This series converts it to the coroutine model
that other drivers are using and removes some AIO functions from the block
layer API afterwards.

If this isn't compelling enough, the diffstat should speak for itself.

This series is relatively long, but it consists mostly of mechanical
conversions of one function per patch, so it should be easy to review.

Kevin Wolf (29):
  qed: Use bottom half to resume waiting requests
  qed: Make qed_read_table() synchronous
  qed: Remove callback from qed_read_table()
  qed: Remove callback from qed_read_l2_table()
  qed: Remove callback from qed_find_cluster()
  qed: Make qed_read_backing_file() synchronous
  qed: Make qed_copy_from_backing_file() synchronous
  qed: Remove callback from qed_copy_from_backing_file()
  qed: Make qed_write_header() synchronous
  qed: Remove callback from qed_write_header()
  qed: Make qed_write_table() synchronous
  qed: Remove GenericCB
  qed: Remove callback from qed_write_table()
  qed: Make qed_aio_read_data() synchronous
  qed: Make qed_aio_write_main() synchronous
  qed: Inline qed_commit_l2_update()
  qed: Add return value to qed_aio_write_l1_update()
  qed: Add return value to qed_aio_write_l2_update()
  qed: Add return value to qed_aio_write_main()
  qed: Add return value to qed_aio_write_cow()
  qed: Add return value to qed_aio_write_inplace/alloc()
  qed: Add return value to qed_aio_read/write_data()
  qed: Remove ret argument from qed_aio_next_io()
  qed: Remove recursion in qed_aio_next_io()
  qed: Implement .bdrv_co_readv/writev
  qed: Use CoQueue for serialising allocations
  qed: Simplify request handling
  qed: Use a coroutine for need_check_timer
  block: Remove bdrv_aio_readv/writev_flush()

 block/Makefile.objs   |   2 +-
 block/io.c            | 171 -----------
 block/qed-cluster.c   | 123 ++++----
 block/qed-gencb.c     |  33 ---
 block/qed-table.c     | 261 ++++++-----------
 block/qed.c           | 763 +++++++++++++++++++-------------------------------
 block/qed.h           |  53 +---
 include/block/block.h |   8 -
 8 files changed, 432 insertions(+), 982 deletions(-)
 delete mode 100644 block/qed-gencb.c

-- 
1.8.3.1

On 26/05/2017 22:21, Kevin Wolf wrote:
> The qed block driver is one of the last remaining block drivers that use the
> AIO callback style interfaces. This series converts it to the coroutine model
> that other drivers are using and removes some AIO functions from the block
> layer API afterwards.
> 
> If this isn't compelling enough, the diffstat should speak for itself.
> 
> This series is relatively long, but it consists mostly of mechanical
> conversions of one function per patch, so it should be easy to review.

Looks great, just a few notes:

- There are a couple "Callback from qed_find_cluster()" comments that 
are obsolete.

- qed_acquire/qed_release can be removed as you inline stuff, but this
should not cause bugs so you can either do it as a final patch or let
me remove it later.

- coroutine_fn annotations are missing.  Also can be done as a final
patch.  It's a bit ugly that L1/L2 table access can happen both in a
coroutine (for regular I/O) and outside (for create/check).

- qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
course a lot more cleanup could be done (I will do a couple more such
changes when adding a CoMutex to make the driver thread-safe) but this
one is pretty low-hanging fruit and seems to be in line with the rest
of the patches.

- qed_co_request doesn't need g_new for the QEDAIOCB.

Paolo

> Kevin Wolf (29):
>   qed: Use bottom half to resume waiting requests
>   qed: Make qed_read_table() synchronous
>   qed: Remove callback from qed_read_table()
>   qed: Remove callback from qed_read_l2_table()
>   qed: Remove callback from qed_find_cluster()
>   qed: Make qed_read_backing_file() synchronous
>   qed: Make qed_copy_from_backing_file() synchronous
>   qed: Remove callback from qed_copy_from_backing_file()
>   qed: Make qed_write_header() synchronous
>   qed: Remove callback from qed_write_header()
>   qed: Make qed_write_table() synchronous
>   qed: Remove GenericCB
>   qed: Remove callback from qed_write_table()
>   qed: Make qed_aio_read_data() synchronous
>   qed: Make qed_aio_write_main() synchronous
>   qed: Inline qed_commit_l2_update()
>   qed: Add return value to qed_aio_write_l1_update()
>   qed: Add return value to qed_aio_write_l2_update()
>   qed: Add return value to qed_aio_write_main()
>   qed: Add return value to qed_aio_write_cow()
>   qed: Add return value to qed_aio_write_inplace/alloc()
>   qed: Add return value to qed_aio_read/write_data()
>   qed: Remove ret argument from qed_aio_next_io()
>   qed: Remove recursion in qed_aio_next_io()
>   qed: Implement .bdrv_co_readv/writev
>   qed: Use CoQueue for serialising allocations
>   qed: Simplify request handling
>   qed: Use a coroutine for need_check_timer
>   block: Remove bdrv_aio_readv/writev_flush()
> 
>  block/Makefile.objs   |   2 +-
>  block/io.c            | 171 -----------
>  block/qed-cluster.c   | 123 ++++----
>  block/qed-gencb.c     |  33 ---
>  block/qed-table.c     | 261 ++++++-----------
>  block/qed.c           | 763 +++++++++++++++++++-------------------------------
>  block/qed.h           |  53 +---
>  include/block/block.h |   8 -
>  8 files changed, 432 insertions(+), 982 deletions(-)
>  delete mode 100644 block/qed-gencb.c
> 

Am 29.05.2017 um 13:22 hat Paolo Bonzini geschrieben:
> On 26/05/2017 22:21, Kevin Wolf wrote:
> > The qed block driver is one of the last remaining block drivers that use the
> > AIO callback style interfaces. This series converts it to the coroutine model
> > that other drivers are using and removes some AIO functions from the block
> > layer API afterwards.
> > 
> > If this isn't compelling enough, the diffstat should speak for itself.
> > 
> > This series is relatively long, but it consists mostly of mechanical
> > conversions of one function per patch, so it should be easy to review.
> 
> Looks great, just a few notes:
> 
> - There are a couple "Callback from qed_find_cluster()" comments that 
> are obsolete.

Fixed.

> - qed_acquire/qed_release can be removed as you inline stuff, but this
> should not cause bugs so you can either do it as a final patch or let
> me remove it later.

To be honest, I don't completely understand what they are protecting in
the first place. The places where they are look quite strange to me. So
I tried to simply leave them alone.

What is the reason that we can remove them when we inline stuff?
Shouldn't the inlining be semantically equivalent?

I guess the answer is important for the case in patch 5, where Stefan
commented that I moved some code out of the lock.

> - coroutine_fn annotations are missing.  Also can be done as a final
> patch.  It's a bit ugly that L1/L2 table access can happen both in a
> coroutine (for regular I/O) and outside (for create/check).

Create is coroutine-based these days. Maybe we should convert check,
too.

> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
> course a lot more cleanup could be done (I will do a couple more such
> changes when adding a CoMutex to make the driver thread-safe) but this
> one is pretty low-hanging fruit and seems to be in line with the rest
> of the patches.
> 
> - qed_co_request doesn't need g_new for the QEDAIOCB.

I stopped intentionally when the thing was fully coroutine based
because I thought the series is already long enough. There are many more
cleanups that could be done (e.g. QEDAIOCB should be completely
avoided), but let's do that on top.

Kevin

On 01/06/2017 18:28, Kevin Wolf wrote:
>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>> should not cause bugs so you can either do it as a final patch or let
>> me remove it later.
> To be honest, I don't completely understand what they are protecting in
> the first place. The places where they are look quite strange to me. So
> I tried to simply leave them alone.
> 
> What is the reason that we can remove them when we inline stuff?
> Shouldn't the inlining be semantically equivalent?

You're right, they can be removed when going from callback to direct
call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
does it for the callbacks).

>> - coroutine_fn annotations are missing.  Also can be done as a final
>> patch.  It's a bit ugly that L1/L2 table access can happen both in a
>> coroutine (for regular I/O) and outside (for create/check).
>
> Create is coroutine-based these days. Maybe we should convert check,
> too.

Good idea, separate series. :)

>> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
>> course a lot more cleanup could be done (I will do a couple more such
>> changes when adding a CoMutex to make the driver thread-safe) but this
>> one is pretty low-hanging fruit and seems to be in line with the rest
>> of the patches.
>>
>> - qed_co_request doesn't need g_new for the QEDAIOCB.
>
> I stopped intentionally when the thing was fully coroutine based
> because I thought the series is already long enough. There are many more
> cleanups that could be done (e.g. QEDAIOCB should be completely
> avoided), but let's do that on top.

Agreed about qed_is_allocated_cb.  Regarding qed_co_request you're
already doing an almost complete rewrite of it in patch 27, and making
QEDAIOCB not heap allocated would be a very small change removing half a
dozen lines of code.

Paolo

Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
> On 01/06/2017 18:28, Kevin Wolf wrote:
> >> - qed_acquire/qed_release can be removed as you inline stuff, but this
> >> should not cause bugs so you can either do it as a final patch or let
> >> me remove it later.
> > To be honest, I don't completely understand what they are protecting in
> > the first place. The places where they are look quite strange to me. So
> > I tried to simply leave them alone.
> > 
> > What is the reason that we can remove them when we inline stuff?
> > Shouldn't the inlining be semantically equivalent?
> 
> You're right, they can be removed when going from callback to direct
> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
> does it for the callbacks).

So if we take qed_read_table_cb() for example:

    qed_acquire(s);
    for (i = 0; i < noffsets; i++) {
        table->offsets[i] = le64_to_cpu(table->offsets[i]);
    }
    qed_release(s);

First of all, I don't see what it protects. If we wanted to avoid that
someone else sees table->offsets with wrong endianness, we would be
taking the lock much too late. And if nobody else knows about the table
yet, what is there to be locked?

But anyway, given your explanation that acquiring the AioContext lock is
getting replaced by coroutine magic, the qed_acquire/release() pair
actually can't be removed in patch 2 when the callback is converted to a
direct call, but only when the whole call path between .bdrv_aio_readv/
writev and this specific callback is converted, so that we never drop
out of coroutine context before reaching this code. Correct?

This happens only very late in the series, so it probably also means
that patch 5 is indeed wrong because it removes the lock too early?

> >> - coroutine_fn annotations are missing.  Also can be done as a final
> >> patch.  It's a bit ugly that L1/L2 table access can happen both in a
> >> coroutine (for regular I/O) and outside (for create/check).
> >
> > Create is coroutine-based these days. Maybe we should convert check,
> > too.
> 
> Good idea, separate series. :)
> 
> >> - qed_is_allocated_cb can be inlined and QEDIsAllocatedCB removed.  Of
> >> course a lot more cleanup could be done (I will do a couple more such
> >> changes when adding a CoMutex to make the driver thread-safe) but this
> >> one is pretty low-hanging fruit and seems to be in line with the rest
> >> of the patches.
> >>
> >> - qed_co_request doesn't need g_new for the QEDAIOCB.
> >
> > I stopped intentionally when the thing was fully coroutine based
> > because I thought the series is already long enough. There are many more
> > cleanups that could be done (e.g. QEDAIOCB should be completely
> > avoided), but let's do that on top.
> 
> Agreed about qed_is_allocated_cb.  Regarding qed_co_request you're
> already doing an almost complete rewrite of it in patch 27, and making
> QEDAIOCB not heap allocated would be a very small change removing half a
> dozen lines of code.

You're right, wasn't aware of this change any more. :-)

I'll do that then.

Kevin

On 01/06/2017 19:08, Kevin Wolf wrote:
> Am 01.06.2017 um 18:40 hat Paolo Bonzini geschrieben:
>> On 01/06/2017 18:28, Kevin Wolf wrote:
>>>> - qed_acquire/qed_release can be removed as you inline stuff, but this
>>>> should not cause bugs so you can either do it as a final patch or let
>>>> me remove it later.
>>> To be honest, I don't completely understand what they are protecting in
>>> the first place. The places where they are look quite strange to me. So
>>> I tried to simply leave them alone.
>>>
>>> What is the reason that we can remove them when we inline stuff?
>>> Shouldn't the inlining be semantically equivalent?
>>
>> You're right, they can be removed when going from callback to direct
>> call.  Callbacks are invoked without the AioContext acquire (aio_co_wake
>> does it for the callbacks).
> 
> So if we take qed_read_table_cb() for example:
> 
>     qed_acquire(s);
>     for (i = 0; i < noffsets; i++) {
>         table->offsets[i] = le64_to_cpu(table->offsets[i]);
>     }
>     qed_release(s);
> 
> First of all, I don't see what it protects. If we wanted to avoid that
> someone else sees table->offsets with wrong endianness, we would be
> taking the lock much too late. And if nobody else knows about the table
> yet, what is there to be locked?

That is the product of a mechanical conversion where all callbacks grew
a qed_acquire/qed_release pair (commit b9e413dd37, "block: explicitly
acquire aiocontext in aio callbacks that need it", 2017-02-21).

In this case:

        qed_acquire(s);
        bdrv_aio_flush(write_table_cb->s->bs, qed_write_table_cb,
                       write_table_cb);
        qed_release(s);

the AioContext protects write_table_cb->s->bs.

> But anyway, given your explanation that acquiring the AioContext lock is
> getting replaced by coroutine magic, the qed_acquire/release() pair
> actually can't be removed in patch 2 when the callback is converted to a
> direct call, but only when the whole call path between .bdrv_aio_readv/
> writev and this specific callback is converted, so that we never drop
> out of coroutine context before reaching this code. Correct?

Yes.

> This happens only very late in the series, so it probably also means
> that patch 5 is indeed wrong because it removes the lock too early?

bdrv_qed_co_get_block_status is entirely in coroutine context, so I
think that one is fine.

Paolo