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

[Denis Plotnikov]

On 10.09.2018 15:41, Kevin Wolf wrote:
> 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 <address@hidden>
>
> 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.
Good point! Thanks!
>     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.
Using the common point might be a good idea, but in case aio requests we 
also have to mane completions which out of the scope of 
blk_co_p(read|write)v:

static void blk_aio_write_entry(void *opaque) {
    ...
    rwco->ret = blk_co_pwritev(...);

    blk_aio_complete(acb);
    ...
}

This makes the difference.
I would suggest adding waiting until "drained_end" is done on the 
synchronous read/write at blk_prw

                              >
> 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.
There is an idea to distinguish the requests that should be done without 
respect to "drained section" by using a flag in BdrvRequestFlags. The 
requests with a flag set should be processed anyway.
> 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.
In my understanding by using brdv_drained_begin/end we want to protect a 
certain BlockDriverState from external access but not the whole 
BlockBackend which may involve using a number of BlockDriverState-s.
I though it because we could possibly change a backing file for some 
BlockDriverState. And for the time of changing we need to prevent 
external access to it but keep the io going.
By using blk_root_drained_begin/end() we put to "drained section" all 
the BlockDriverState-s linked to that root.
Does it have to be so?

Denis

> Kevin

--
Best,
Denis