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

[Kevin Wolf]

Am 25.10.2016 um 16:41 hat Paolo Bonzini geschrieben:
> 
> 
> On 25/10/2016 16:38, Kevin Wolf wrote:
> > 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().
> 
> So
> 
>       bs->quiesce_counter = all_quiesce_counter;
> 
> ?  That would work.

Yes, that's what I had in mind.

> Should bdrv_close assert bs->quiesce_counter==0
> (which implies all_quiesce_counter == 0), since it usually has to do I/O?

Hm... Not sure about that. We're still using bdrv_drain_all_begin/end as
a function to isolate the BDSes, so some I/O from the caller of
drain_all is expected, and that could involve deleting a BDS.

But once we fully implemented what you proposed, probably yes.

Kevin