Re: [Qemu-block] [PATCH 00/29] qed: Convert to coroutines

[Kevin Wolf]

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