Re: [Qemu-devel] [PATCH v2 1/3] block: block: introduce bdrv_io_plug() and bdrv_io_unplug()

[Paolo Bonzini]

Il 02/07/2014 02:46, Ming Lei ha scritto:
> On Wed, Jul 2, 2014 at 12:56 AM, Paolo Bonzini <address@hidden> wrote:
> > Il 01/07/2014 17:21, Kevin Wolf ha scritto:
> > > > > > Does this bs->file forwarding work for more than the raw driver? For
> > > > > > example, if drv is an image format driver that needs to read some
> > > > > > metadata from the image before it can submit the payload, does this
> > > > > > still do what you were intending?
> > > >
> > > >
> > > > Sorry for not understanding the problem, and you are right, these
> > > > patches can't support other formats, and for solving the dependency,
> > > > changes to image format driver should be needed.
> > >
> > >
> > > Then let's drop the bs->file recursion here and add an explicit
> > > .bdrv_io_plug/unplug callback to the raw driver.
> >
> >
> > Actually I thought about this in my review, and there's no reason for this
> > not to work for image formats.
> >
> > While bs->file is plugged, image formats will start executing their
> > bdrv_co_readv/bdrv_co_writev callbacks, and issue reads or writes as
> > necessary.  The reads and writes will accumulate in bs->file until it is
> > unplugged, which is exactly the effect we want.
>
> For some image formats, meta data need to be read first before
> the payload can be read since how/what to read payload might
> depend on content of meta data.

Yes, but everything is done asynchronously so it should just work.  You have

    plug(bs)
      plug(bs->file)
    read
      start coroutine for image format read
        issue metadata read on bs->file
          bdrv_read ultimately calls bdrv_aio_readv on bs->file
            I/O not submitted because bs->file is plugged
        coroutine yields
    unplug(bs)
      unplug(bs->file)
        bs->file now submits metadata read
    ...
    metadata read completes
      coroutine re-entered
        image format read restarts, unaware of plug/unplug

If things do _not_ work as above I would like to understand where my 
reasoning is wrong.

On top of this, there _could_ be reasons for formats to implement 
plug/unplug themselves.  They could coalesce metadata reads or 
copy-on-write operations, for example.  This however is independent from 
the default behavior, which IMO is "plugging should propagate along 
bs->file" (and possibly bs->backing_hd too, but not now because that 
opens more cans of worms).

> > The change in bdrv_drain_all is ugly though.  I don't have a better idea,
> > but I would like to understand better why it is needed.  Ming Lei, did you
> > see a deadlock without it?
>
> Not yet, just for safe reason to make sure all queued data has chance
> to be flushed. If you think it isn't necessary, I can remove it.

If you could make it an assertion, that would also be great.  (BTW, it's 
probably best to add a nesting count to plug/unplug).

Paolo