Re: [Qemu-block] [PATCH v11 01/19] block: Add bdrv_drain_all_{begin, end}()

[Kevin Wolf]

Am 25.10.2016 um 15:53 hat Paolo Bonzini geschrieben:
> 
> 
> On 25/10/2016 15:39, Alberto Garcia wrote:
> > On Mon 24 Oct 2016 12:53:41 PM CEST, Paolo Bonzini wrote:
> > 
> >>> My first thoughts were about how to let an unpause succeed without a
> >>> previous pause for these objects, but actually I think this isn't
> >>> what we should do. We rather want to actually do the pause instead
> >>> because even new BDSes and block jobs should probably start in a
> >>> quiesced state when created inside a drain_all section.
> >>
> >> Yes, I agree with this.  It shouldn't be too hard to implement it.  It
> >> would require a BlockDriverState to look at the global "inside
> >> bdrv_drain_all_begin" state, and ask its BlockBackend to disable
> >> itself upon bdrv_replace_child.
> > 
> > Why in bdrv_replace_child()? bdrv_drain_all_end() enables all BDSs, but
> > if you add one with "blockdev-add" it's not going to be disabled using
> > this method.
> 
> You only need to disable it when blk_insert_bs is called.  In fact...

This assumes that the block driver doesn't issue internal background I/O
by itself. Probably true for everything that we have today, but it would
probably be cleaner to quiesce it directly in bdrv_open_common().

> > In addition to that block jobs need the same, don't they? Something like
> > "job->pause_count = all_quiesce_counter" in the initialization.
> 
> ... we discussed a couple weeks ago that block jobs should register
> BlkDeviceOps that pause the job in the drained_begin callback.  So when
> the block job calls blk_insert_bs, the drained_begin callback would
> start the job as paused.

Yes, should, but doing this kind of infrastructure work isn't something
for this series.

> > I think we'd also need to add block_job_pause_point() at the beginning
> > of each one of their coroutines, in order to make sure that they really
> > start paused.
> 
> It depends on the job, for example streaming starts with
> block_job_sleep_ns.  Commit also does, except for some blk_getlength and
> blk_truncate calls that could be moved to the caller.

Kevin