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

v4:
  forward only for block filters
  new patch: remove bdrv_media_changed
  dropped commit `block: Use defaults of bdrv_* callbacks in raw`, since raw is
  not a filter driver and is incompatible with the changes.

v3:
  minor changes by Eric Blake's suggestion
  new patch: remove bdrv_truncate method from blkdebug

v2:
  do not pass to bs->file if bs->drv is NULL
  move bs->file check outside of bdrv_inc_in_flight() area in bdrv_co_ioctl()
  new patch: remove duplicate code from block/raw-format.c

Manos Pitsidianakis (4):
  block: pass bdrv_* methods to bs->file by default in block filters
  block: remove bdrv_media_changed
  block: remove bdrv_truncate callback in blkdebug
  block: add default implementations for bdrv_co_get_block_status()

 block.c                   | 35 +++++++++++++++++++----------------
 block/blkdebug.c          | 19 ++-----------------
 block/commit.c            | 12 +-----------
 block/io.c                | 26 ++++++++++++++++++++++++++
 block/mirror.c            | 12 +-----------
 block/raw-format.c        |  6 ------
 include/block/block.h     |  1 -
 include/block/block_int.h | 25 +++++++++++++++++++++++--
 8 files changed, 72 insertions(+), 64 deletions(-)

-- 
2.11.0

Manos Pitsidianakis <el13635@mail.ntua.gr> 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?

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

On Wed, Jul 12, 2017 at 09:49:20AM +0200, Markus Armbruster wrote:
>Manos Pitsidianakis <el13635@mail.ntua.gr> 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.

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 <el13635@mail.ntua.gr> 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