On Thu, Aug 3, 2017 at 11:36 PM, Paolo Bonzini <pbonz...@redhat.com> wrote:
> ----- Original Message -----
>> From: "Dr. David Alan Gilbert" <dgilb...@redhat.com>
>> To: "Alberto Garcia" <be...@igalia.com>
>> Cc: qemu-devel@nongnu.org, pbonz...@redhat.com, js...@redhat.com
>> 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 (be...@igalia.com) 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
