Re: [Qemu-devel] [PATCH] vl.c/exit: pause cpus before closing block devices

[Stefan Hajnoczi]

On Thu, Aug 3, 2017 at 11:36 PM, Paolo Bonzini <address@hidden> wrote:
> ----- Original Message -----
>> From: "Dr. David Alan Gilbert" <address@hidden>
>> To: "Alberto Garcia" <address@hidden>
>> Cc: address@hidden, address@hidden, address@hidden
>> Sent: Thursday, August 3, 2017 6:45:17 PM
>> Subject: Re: [Qemu-devel] [PATCH] vl.c/exit: pause cpus before closing block 
>> devices
>>
>> * Alberto Garcia (address@hidden) wrote:
>> > On Thu, Jul 13, 2017 at 08:01:16PM +0100, Dr. David Alan Gilbert (git)
>> > wrote:
>> > > --- a/vl.c
>> > > +++ b/vl.c
>> > > @@ -4787,8 +4787,8 @@ int main(int argc, char **argv, char **envp)
>> > >      replay_disable_events();
>> > >      iothread_stop_all();
>> > >
>> > > -    bdrv_close_all();
>> > >      pause_all_vcpus();
>> > > +    bdrv_close_all();
>> > >      res_free();
>> >
>> > I haven't debugged it yet, but in my computer iotest 093 stops working
>> > (it never finishes) after this change.
>>
>> Yes, I can reproduce that here (I've got to explicitly run 093 - it
>> doesn't do it automatically for me):
>
> The culprit so to speak is this:
>
>         if (qtest_enabled()) {
>             /* For testing block IO throttling only */
>             tg->clock_type = QEMU_CLOCK_VIRTUAL;
>         }
>
> So after pause_all_vcpus(), the clock doesn't advance and bdrv_close_all
> hangs.  Should throttling be disabled by the time bdrv_close drains the
> BlockDriverState, and likewise for bdrv_close_all?

bdrv_drain_all() is called before blk_remove_all_bs(), so throttling
is still enabled while draining:

void bdrv_close_all(void)
{
    block_job_cancel_sync_all();
    nbd_export_close_all();

    /* Drop references from requests still in flight, such as canceled block
     * jobs whose AIO context has not been polled yet */
    bdrv_drain_all();

    blk_remove_all_bs();

Perhaps we need a bdrv_throttle_disable_all() function.  Manos is
moving throttling to a block driver so there is no longer a 1:1
relationship between BB and throttling - there might be multiple
throtting nodes in a BDS graph.

Stefan