Re: [Qemu-block] [Qemu-devel] [PATCH v4 0/4] block: Block driver callbacks fixes

[Kevin Wolf]

Am 12.07.2017 um 10:10 hat Manos Pitsidianakis geschrieben:
> On Wed, Jul 12, 2017 at 09:49:20AM +0200, Markus Armbruster wrote:
> >Manos Pitsidianakis <address@hidden> writes:
> >
> >>This series makes implementing some of the bdrv_* callbacks easier for block
> >>filters by passing requests to bs->file if bs->drv doesn't implement it 
> >>instead
> >>of failing, and adding default bdrv_co_get_block_status() implementations.
> >>
> >>This is based against Kevin Wolf's block branch, commit
> >>da4bd74d2450ab72a7c26bbabb10c6a287dd043e
> >
> >Haven't seen BlockDriver member is_filter before.  Interesting.  It's
> >documentation
> >
> >   /* set to true if the BlockDriver is a block filter */
> >   bool is_filter;
> >
> >is seriously lacking.  What does it *mean* to be a block filter?  Which
> >block layer facilities are affected, and how?
> 
> Currently it is only used in bdrv_recurse_is_first_non_filter.
> 
> >Observation: driver "raw" is filter-like in the sense that all it does
> >is pass along method arguments and results.  Can't say whether that
> >makes it a filter in the sense of is_filter, because "the sense of
> >is_filter" is nebulous to me :)
> 
> I'm not very acquainted with raw, so I can't really comment. But the
> drivers I'm working on, throttle and before write notifier have that
> exact semantic, ie they do something when IO is intercepted and pass
> everything to the BDSes below. There was a mini discussion about raw
> and filters in the previous version's thread.
> 
> I might add documentation to block_int.h in the future. When I first
> read it it felt nebulous to me too.

Not something that should stop this series, but while you're adding some
implications of being a filter to block_int.h, we still don't have a
definition of which block drivers qualify as filters.

"do something when IO is intercepted and pass everything to the BDSes
below" is probably not enough, every driver does this unless it is a
protocol driver.

We could further qualify it such that reads/writes on filter always
result in reads/writes on the same offset in bs->file (this disqualifies
the raw format, which you seem to want) and the block status is always
taken from bs->file.

We could also add that the data read/written must be the same - it's not
quite clear to me yet if we want to require this or can make use of the
property.

I'm open for other suggestion of what makes a filter a filter in the
sense of this field. Ideally it would contain enough (or maybe better:
the right) conditions that you can use it to justify why for each of the
functions that you pass down by default, this is the right behaviour.

For example, passing through bdrv_probe_geometry() makes sense because
all I/O is passed through unmodified and the image size is the same,
etc.

Kevin

pgpRYZrsrQWi7.pgp
Description: PGP signature