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>
---
 block/block-backend.c | 58 ++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 12 deletions(-)

diff --git a/block/block-backend.c b/block/block-backend.c
index d55c328736..68dcd704d2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1318,6 +1318,7 @@ typedef struct BlkAioEmAIOCB {
     BlkRwCo rwco;
     int bytes;
     bool has_returned;
+    CoroutineEntry *co_entry;
 } BlkAioEmAIOCB;
 
 static const AIOCBInfo blk_aio_em_aiocb_info = {
@@ -1340,16 +1341,55 @@ static void blk_aio_complete_bh(void *opaque)
     blk_aio_complete(acb);
 }
 
+static void blk_aio_create_co(void *opaque)
+{
+    BlockDriverState *current_bs;
+    AioContext *ctx;
+    BlkAioEmAIOCB *acb = (BlkAioEmAIOCB *) opaque;
+    BlockBackend *blk = acb->rwco.blk;
+
+    /* The check makes sense if the action was postponed until the context
+     * is enabled for external requests: if a BlockDriverState of a BlockBackend
+     * was changed, for example on making a new snapshot, update BlockDriverState
+     * in ACB and try to run the coroutine in the changed BDS context
+     */
+    current_bs = blk_bs(blk);
+
+    if (current_bs != acb->common.bs) {
+        acb->common.bs = current_bs;
+    }
+
+    ctx = blk_get_aio_context(blk);
+    /* If a request comes from device (e.g. ide controller) when
+     * the context disabled, postpone the request until the context is
+     * enabled for external requests.
+     * Otherwise, create a couroutine and enter it right now
+     */
+    aio_context_acquire(ctx);
+    if (aio_external_disabled(ctx)) {
+        AioPostponedAction *action = aio_create_postponed_action(
+                                                blk_aio_create_co, acb);
+        aio_postpone_action(ctx, action);
+    } else {
+        Coroutine *co = qemu_coroutine_create(acb->co_entry, acb);
+        blk_inc_in_flight(blk);
+        bdrv_coroutine_enter(acb->common.bs, co);
+
+        acb->has_returned = true;
+        if (acb->rwco.ret != NOT_DONE) {
+            aio_bh_schedule_oneshot(ctx, blk_aio_complete_bh, acb);
+        }
+    }
+    aio_context_release(ctx);
+}
+
 static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
                                 void *iobuf, CoroutineEntry co_entry,
                                 BdrvRequestFlags flags,
                                 BlockCompletionFunc *cb, void *opaque)
 {
-    BlkAioEmAIOCB *acb;
-    Coroutine *co;
 
-    blk_inc_in_flight(blk);
-    acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
+    BlkAioEmAIOCB *acb = blk_aio_get(&blk_aio_em_aiocb_info, blk, cb, opaque);
     acb->rwco = (BlkRwCo) {
         .blk    = blk,
         .offset = offset,
@@ -1359,15 +1399,9 @@ static BlockAIOCB *blk_aio_prwv(BlockBackend *blk, int64_t offset, int bytes,
     };
     acb->bytes = bytes;
     acb->has_returned = false;
+    acb->co_entry = co_entry;
 
-    co = qemu_coroutine_create(co_entry, acb);
-    bdrv_coroutine_enter(blk_bs(blk), co);
-
-    acb->has_returned = true;
-    if (acb->rwco.ret != NOT_DONE) {
-        aio_bh_schedule_oneshot(blk_get_aio_context(blk),
-                                blk_aio_complete_bh, acb);
-    }
+    blk_aio_create_co(acb);
 
     return &acb->common;
 }
-- 
2.17.0

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

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 <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.
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

Am 12.09.2018 um 14:03 hat Denis Plotnikov geschrieben:
> 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 <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.
> 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:

I don't understand what you mean here (possibly because I fail to
understand the word "mane") and what completions have to do with
queueing of requests.

Just to clarify, we are talking about the following situation, right?
bdrv_drain_all_begin() has returned, so all the old requests have
already been drained and their completion callback has already been
called. For any new requests that come in, we need to queue them until
the drained section ends. In other words, they won't reach the point
where they could possibly complete before .drained_end.

> 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

It is possible, but then the management becomes a bit more complicated
because you have more than just a list of Coroutines that you need to
wake up.

One thing that could be problematic in blk_co_preadv/pwritev is that
blk->in_flight would count even requests that are queued if we're not
careful. Then a nested drain would deadlock because the BlockBackend
would never say that it's quiesced.

>                               >
> > 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 don't think that would work because the accesses can be nested quite
deeply in functions that can be called from anywhere.

But possibly all of the interesting cases are directly calling BDS
functions anyway and not BlockBackend.

> > 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?

It's the other way round, actually.

In order for a BDS to be fully drained, it must make sure that it
doesn't get new requests from its parents any more. So drain propagates
towards the parents, not towards the children.

blk_root_drained_begin/end() are functions that are called when
blk->root.bs is drained.

Kevin

On 09/12/2018 04:15 PM, Kevin Wolf wrote:
> Am 12.09.2018 um 14:03 hat Denis Plotnikov geschrieben:
>> 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 <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.
>> Good point! Thanks!

Should we really prohibit all public interfaces, as they are reused
inside block
level?

There is also a problem which is not stated in the clear words yet.
We have potential deadlock in the code under the following
conditions, which should be also taken into the consideration.

<path from the controller>
bdrv_co_pwritev
    bdrv_inc_in_flight
    bdrv_aligned_pwritev
        notifier_list_with_return_notify
             backup_before_write_notify
                 backup_do_cow
                     backup_cow_with_bounce_buffer
                         blk_co_preadv

Here blk_co_preadv() must finish its work before we
will release the notifier and finish request initiated
from the controller and which has incremented
in-fligh counter.

Thus we should differentiate requests initiated at the
controller level and requests initiated in the block layer.
This is sad but true.

The idea to touch only these interfaces was to avoid
interference with block jobs code. It is revealed that
the approach is a mistake and we should have a
segregation by request kinds. Thus the idea of the
flag for use in the controller code should not be that
awful.


>>>     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:
> I don't understand what you mean here (possibly because I fail to
> understand the word "mane") and what completions have to do with
> queueing of requests.
>
> Just to clarify, we are talking about the following situation, right?
> bdrv_drain_all_begin() has returned, so all the old requests have
> already been drained and their completion callback has already been
> called. For any new requests that come in, we need to queue them until
> the drained section ends. In other words, they won't reach the point
> where they could possibly complete before .drained_end.

Such requests should not reach the point once they will start to
execute EXCEPT notifiers. There is a big problem with synchronous
which can queue new requests and that requests are to be finished.

Den

Am 12.09.2018 um 19:03 hat Denis V. Lunev geschrieben:
> On 09/12/2018 04:15 PM, Kevin Wolf wrote:
> > Am 12.09.2018 um 14:03 hat Denis Plotnikov geschrieben:
> >> 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 <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.
> >> Good point! Thanks!
> 
> Should we really prohibit all public interfaces, as they are reused
> inside block level?

We need to fix that. blk_*() should never be called from inside the BDS
layer.

> There is also a problem which is not stated in the clear words yet.
> We have potential deadlock in the code under the following
> conditions, which should be also taken into the consideration.
> 
> <path from the controller>
> bdrv_co_pwritev
>     bdrv_inc_in_flight
>     bdrv_aligned_pwritev
>         notifier_list_with_return_notify
>              backup_before_write_notify
>                  backup_do_cow
>                      backup_cow_with_bounce_buffer
>                          blk_co_preadv
> 
> Here blk_co_preadv() must finish its work before we will release the
> notifier and finish request initiated from the controller and which
> has incremented in-fligh counter.

Yes, before_write notifiers are evil. I've objected to them since the
day they were introduced and I'm surprised it's becoming a problem only
now.

We should probably change the backup job to insert a job node rather
sooner than later. Then it doesn't need to call blk_*() any more.

> Thus we should differentiate requests initiated at the controller
> level and requests initiated in the block layer.  This is sad but
> true.

The difference is supposed to be whether a request goes through a
BlockBackend or not.

Kevin

On 12.09.2018 16:15, Kevin Wolf wrote:
> Am 12.09.2018 um 14:03 hat Denis Plotnikov geschrieben:
>> 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 <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.
>> 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:
> 
> I don't understand what you mean here (possibly because I fail to
> understand the word "mane") and what completions have to do with
mane = make
> queueing of requests.
> 
> Just to clarify, we are talking about the following situation, right?
> bdrv_drain_all_begin() has returned, so all the old requests have
> already been drained and their completion callback has already been
> called. For any new requests that come in, we need to queue them until
> the drained section ends. In other words, they won't reach the point
> where they could possibly complete before .drained_end.
Yes

To make it clear: I'm trying to defend the idea that putting the 
postponing routine in blk_co_preadv/pwritev is not the best choice and 
that's why:

If I understood your idea correctly, if we do the postponing inside
blk_co_p(write|read)v we don't know whether we do synchronous or 
asynchronous request.
We need to know this because if we postpone an async request then, 
later, on the postponed requests processing, we must to make "a 
completion" for that request stating that it's finally "done".

Furthermore, for sync requests if we postpone them, we must block the 
clients issued them until the requests postponed have been processed on 
drained section leaving. This would ask an additional notification 
mechanism. Instead, we can just check whether we could proceed in 
blk_p(write|read) and if not (we're in drained) to wait there.

We avoid the things above if we postponing in blk_aio_prwv and waiting 
in blk_prw without postponing.

What do you think?

> 
>> 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
> 
> It is possible, but then the management becomes a bit more complicated
> because you have more than just a list of Coroutines that you need to
> wake up.
> 
> One thing that could be problematic in blk_co_preadv/pwritev is that
> blk->in_flight would count even requests that are queued if we're not
> careful. Then a nested drain would deadlock because the BlockBackend
> would never say that it's quiesced.
> 
>>                                >
>>> 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 don't think that would work because the accesses can be nested quite
> deeply in functions that can be called from anywhere.
> 
> But possibly all of the interesting cases are directly calling BDS
> functions anyway and not BlockBackend.
I hope it's so but what If not, fixing everywhere?
> 
>>> 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?
> 
> It's the other way round, actually.
> 
> In order for a BDS to be fully drained, it must make sure that it
> doesn't get new requests from its parents any more. So drain propagates
> towards the parents, not towards the children.
> 
> blk_root_drained_begin/end() are functions that are called when
> blk->root.bs is drained.
Make sense. Now I understand.

Denis
> 
> Kevin
> 

-- 
Best,
Denis

Am 12.09.2018 um 16:53 hat Denis Plotnikov geschrieben:
> On 12.09.2018 16:15, Kevin Wolf wrote:
> > Am 12.09.2018 um 14:03 hat Denis Plotnikov geschrieben:
> > > 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 <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.
> > > 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:
> > 
> > I don't understand what you mean here (possibly because I fail to
> > understand the word "mane") and what completions have to do with
> mane = make
> > queueing of requests.
> > 
> > Just to clarify, we are talking about the following situation, right?
> > bdrv_drain_all_begin() has returned, so all the old requests have
> > already been drained and their completion callback has already been
> > called. For any new requests that come in, we need to queue them until
> > the drained section ends. In other words, they won't reach the point
> > where they could possibly complete before .drained_end.
> Yes
> 
> To make it clear: I'm trying to defend the idea that putting the postponing
> routine in blk_co_preadv/pwritev is not the best choice and that's why:
> 
> If I understood your idea correctly, if we do the postponing inside
> blk_co_p(write|read)v we don't know whether we do synchronous or
> asynchronous request.
> We need to know this because if we postpone an async request then, later, on
> the postponed requests processing, we must to make "a completion" for that
> request stating that it's finally "done".

Yes, for AIO requests, the completion callback must be called
eventually. This is not different between normal and postponed requests,
though. This is why blk_aio_read/write_entry() call blk_aio_complete()
before they return. This call will be made for postponed requests, too,
so there is nothing that you would need to do additionally inside
blk_co_preadv/pwritev().

> Furthermore, for sync requests if we postpone them, we must block the
> clients issued them until the requests postponed have been processed on
> drained section leaving. This would ask an additional notification
> mechanism. Instead, we can just check whether we could proceed in
> blk_p(write|read) and if not (we're in drained) to wait there.

Again, this is the same for normal requests. The BDRV_POLL_WHILE() in
blk_prw() already implements the waiting. You don't need another
mechanism.

> We avoid the things above if we postponing in blk_aio_prwv and waiting in
> blk_prw without postponing.
> 
> What do you think?
> 
> > 
> > > 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
> > 
> > It is possible, but then the management becomes a bit more complicated
> > because you have more than just a list of Coroutines that you need to
> > wake up.
> > 
> > One thing that could be problematic in blk_co_preadv/pwritev is that
> > blk->in_flight would count even requests that are queued if we're not
> > careful. Then a nested drain would deadlock because the BlockBackend
> > would never say that it's quiesced.
> > 
> > >                                >
> > > > 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 don't think that would work because the accesses can be nested quite
> > deeply in functions that can be called from anywhere.
> > 
> > But possibly all of the interesting cases are directly calling BDS
> > functions anyway and not BlockBackend.
> I hope it's so but what If not, fixing everywhere?

If you keep things local to the BlockBackend (instead of involving the
AioContext), you can block requests for all other BlockBackends, but
still allow them on the BlockBackend whose user requested draining
(i.e. exclusive access).

Kevin