Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Kevin Wolf]

Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:
> On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&
> > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> > implemented at the same time. How about we be completely explicit:
> > 
> >  bdrv_co_drain_begin is called if implemented in the beggining of a
> >  drain operation to drain and stop any internal sources of requests in
> >  the driver.
> >  bdrv_co_drain_end is called if implemented at the end of the drain.
> > 
> >  They should be used by the driver to e.g. manage scheduled I/O
> >  requests, or toggle an internal state. After the end of the drain new
> >  requests will continue normally.
> > 
> > I hope this is easier for a reader to understand!
> 
> I don't like the inconsistent semantics of when the drained section
> ends, if we allow drivers to implement bdrv_co_drain_begin but omit
> bdrv_co_drained_end.  Currently the point where the section ends is,
> as said in the comment, when next I/O callback is invoked. Now we are
> adding the explicit ".bdrv_co_drain_end" into the fomular, if we still
> keep the previous convention, the interface contract is just mixed of
> two things for no good reason. I don't think it's technically
> necessary.

We don't keep the convention with the next I/O callback. We just allow
drivers to omit an empty implementation of either callback, which seems
to be a very sensible default to me.

> Let's just add the assert:
> 
>     assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);
> 
> in bdrv_drain_invoke, add a comment here

I'm not in favour of this, but if we do want to have it, wouldn't
bdrv_register() be a much better place for the assertion?

> then add an empty .bdrv_co_drain_end in qed.

If you need empty functions anywhere, it's a sign that we have a bad
default behaviour.

Kevin