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

[Kevin Wolf]

Am 02.07.2014 um 10:18 hat Paolo Bonzini geschrieben:
> 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.

This is my understanding of how things work, yes. It means that
plug applies only to the very first I/O that is made against the lower
layer. This means that we may queue L2 table reads, but the actual
payload requests are sent one by one.

In fact, currently, only one request would do the metadata read and hold
a coroutine lock for it. The request would be delayed until the unplug,
but we wouldn't get any batching in this case. We may actually benefit
from it when we get metadata cache hits and the first thing to happen is
the payload request.

Okay, looks safe to enable it even for image formats.

> 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).

Yes, enabling it for bs->backing_hd was my alternative option for the
case that we keep bs->file. What can of worms do you see there? From the
reasoning above, I can't see why bs->backing_hd should be any different
from bs->file.

> >>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).

Agreed.

Another point I was thinking about (not for this series, but in a
follow-up) is what to do with bdrv_aio_multiwrite(). We could either
leave the two different interfaces there, just that multiwrite should
probably call plug/unplug before it sends its requests (this would apply
the improvements to virtio-blk without dataplane; could be done easily
even now), or we try to implement request merging with the plug/unplug
interface and convert the callers. Not sure how we would do the latter,
though, because we don't have a list of requests any more, except in the
lowest layer that actually queues.

Or should we have a default plug/unplug implementation in block.c that
queues any reads and writes, and on unplug merges requests, then plugs
bs->file and sends the requests to the driver?

Kevin