Re: [Qemu-devel] [PATCH v0 2/2] block: postpone the coroutine executing if the BDS's is drained

[Kevin Wolf <kwolf@redhat.com>]

Am 29.06.2018 um 14:40 hat Denis Plotnikov geschrieben:
> Fixes the problem of ide request appearing when the BDS is in
> the "drained section".
> 
> Without the patch the request can come and be processed by the main
> event loop, as the ide requests are processed by the main event loop
> and the main event loop doesn't stop when its context is in the
> "drained section".
> The request execution is postponed until the end of "drained section".
> 
> The patch doesn't modify ide specific code, as well as any other
> device code. Instead, it modifies the infrastructure of asynchronous
> Block Backend requests, in favor of postponing the requests arisen
> when in "drained section" to remove the possibility of request appearing
> for all the infrastructure clients.
> 
> This approach doesn't make vCPU processing the request wait untill
> the end of request processing.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

I generally agree with the idea that requests should be queued during a
drained section. However, I think there are a few fundamental problems
with the implementation in this series:

1) aio_disable_external() is already a layering violation and we'd like
   to get rid of it (by replacing it with a BlockDevOps callback from
   BlockBackend to the devices), so adding more functionality there
   feels like a step in the wrong direction.

2) Only blk_aio_* are fixed, while we also have synchronous public
   interfaces (blk_pread/pwrite) as well as coroutine-based ones
   (blk_co_*). They need to be postponed as well.

   blk_co_preadv/pwritev() are the common point in the call chain for
   all of these variants, so this is where the fix needs to live.

3) Within a drained section, you want requests from other users to be
   blocked, but not your own ones (essentially you want exclusive
   access). We don't have blk_drained_begin/end() yet, so this is not
   something to implement right now, but let's keep this requirement in
   mind and choose a design that allows this.

I believe the whole logic should be kept local to BlockBackend, and
blk_root_drained_begin/end() should be the functions that start queuing
requests or let queued requests resume.

As we are already in coroutine context in blk_co_preadv/pwritev(), after
checking that blk->quiesce_counter > 0, we can enter the coroutine
object into a list and yield. blk_root_drained_end() calls aio_co_wake()
for each of the queued coroutines. This should be all that we need to
manage.

Kevin