Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 3/8] block: add throttle block filter driver

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3063 bytes --]

On Mon, Jun 26, 2017 at 07:01:18PM +0300, Manos Pitsidianakis wrote:
> On Mon, Jun 26, 2017 at 03:30:55PM +0100, Stefan Hajnoczi wrote:
> > > +static BlockDriver bdrv_throttle = {
> > > +    .format_name                        =   "throttle",
> > > +    .protocol_name                      =   "throttle",
> > > +    .instance_size                      =   sizeof(ThrottleGroupMember),
> > > +
> > > +    .bdrv_file_open                     =   throttle_open,
> > > +    .bdrv_close                         =   throttle_close,
> > > +    .bdrv_co_flush                      =   throttle_co_flush,
> > > +
> > > +    .bdrv_child_perm                    =   bdrv_filter_default_perms,
> > > +
> > > +    .bdrv_getlength                     =   throttle_getlength,
> > > +
> > > +    .bdrv_co_preadv                     =   throttle_co_preadv,
> > > +    .bdrv_co_pwritev                    =   throttle_co_pwritev,
> > > +
> > > +    .bdrv_co_pwrite_zeroes              =   throttle_co_pwrite_zeroes,
> > > +    .bdrv_co_pdiscard                   =   throttle_co_pdiscard,
> > > +
> > > +    .bdrv_recurse_is_first_non_filter   =   bdrv_recurse_is_first_non_filter,
> > > +
> > > +    .bdrv_attach_aio_context            =   throttle_attach_aio_context,
> > > +    .bdrv_detach_aio_context            =   throttle_detach_aio_context,
> > > +
> > > +    .bdrv_reopen_prepare                =   throttle_reopen_prepare,
> > > +    .bdrv_reopen_commit                 =   throttle_reopen_commit,
> > > +    .bdrv_reopen_abort                  =   throttle_reopen_abort,
> > > +
> > > +    .is_filter                          =   true,
> > > +};
> > 
> > Missing:
> > bdrv_co_get_block_status()
> > bdrv_truncate()
> > bdrv_get_info()
> > bdrv_probe_blocksizes()
> > bdrv_probe_geometry()
> > bdrv_media_changed()
> > bdrv_eject()
> > bdrv_lock_medium()
> > bdrv_co_ioctl()
> > 
> > See block/raw-format.c.
> > 
> > I think most of these could be modified in block.c or block/io.c to
> > automatically call bs->file's function if drv doesn't implement them.
> > This way all block drivers would transparently pass them through by
> > default and block/raw-format.c code could be eliminated.
> 
> Are these truly necessary? Because other filter drivers (ie quorum,
> blkverify) don't implement them.

Both quorum and blkverify are rarely used.  This explains why the issue
hasn't been found yet.

These are the callbacks I identified which do not automatically forward
to bs->file.  Therefore the throttle driver will break these features
when bs->file supports them.

That's why I suggest forwarding to bs->file in block.c.  Then individual
drivers do not have to implement these callbacks just to forward to
bs->file.  And if the driver wishes to prohibit a feature, it can
implement the callback and return -ENOTSUP.

You can send this fix as a separate patch series, independent of the
throttle driver.  Once it has been merged the throttle driver will gain
support for these features.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]