Re: [PATCH 5/6] test-bdrv-drain.c: remove test_detach_by_parent_cb()

[Paolo Bonzini]

On 2/11/22 16:38, Kevin Wolf wrote:
> The callback of an I/O function isn't I/O, though. It is code_after_
> the I/O has completed. If this doesn't work any more, it feels like this
> is a bug.

The issue is that the I/O has *not* completed yet.  blk_aio_preadv(..., 
cb, opaque) is not equivalent to

        ret = blk_co_preadv(...);
        cb(ret, opaque);

but rather to

        blk_inc_in_flight(blk);
        ret = blk_co_preadv(...);
        cb(ret, opaque);
        blk_dec_in_flight(blk);

Your own commit message (yeah I know we've all been through that :)) 
explains why, and notes that it is now invalid to drain in a callback:

    commit 46aaf2a566e364a62315219255099cbf1c9b990d
    Author: Kevin Wolf <kwolf@redhat.com>
    Date:   Thu Sep 6 17:47:22 2018 +0200

    block-backend: Decrease in_flight only after callback

    Request callbacks can do pretty much anything, including operations
    that will yield from the coroutine (such as draining the backend).
    In that case, a decreased in_flight would be visible to other code
    and could lead to a drain completing while the callback hasn't
    actually completed yet.

    Note that reordering these operations forbids calling drain directly
    inside an AIO callback.

So the questions are:

1) is the above commit wrong though well-intentioned?

2) is it unexpected that bdrv_replace_child_noperm() drains (thus 
becoming invalid from the callback, albeit valid through a bottom half)?


My answer is respectively 1) it's correct, many coroutines do 
inc_in_flight before creation and dec_in_flight at the end, we're just 
saying that it's _always_ the case for callback-based operations; 2) no, 
it's not unexpected and therefore the test is the incorrect one.

Paolo