Re: [Qemu-devel] [PATCH RFC] block: add throttle block filter driver

[Stefan Hajnoczi]

On Thu, Jun 08, 2017 at 10:41:49AM +0300, Manos Pitsidianakis wrote:
> On Thu, Jun 08, 2017 at 08:22:28AM +0100, Stefan Hajnoczi wrote:
> > On Wed, Jun 07, 2017 at 04:06:19PM +0300, Manos Pitsidianakis wrote:
> > > +#define QEMU_OPT_IOPS_TOTAL "iops-total"
> > > +#define QEMU_OPT_IOPS_TOTAL_MAX "iops-total-max"
> > > +#define QEMU_OPT_IOPS_TOTAL_MAX_LENGTH "iops-total-max-length"
> > > +#define QEMU_OPT_IOPS_READ "iops-read"
> > > +#define QEMU_OPT_IOPS_READ_MAX "iops-read-max"
> > > +#define QEMU_OPT_IOPS_READ_MAX_LENGTH "iops-read-max-length"
> > > +#define QEMU_OPT_IOPS_WRITE "iops-write"
> > > +#define QEMU_OPT_IOPS_WRITE_MAX "iops-write-max"
> > > +#define QEMU_OPT_IOPS_WRITE_MAX_LENGTH "iops-write-max-length"
> > > +#define QEMU_OPT_BPS_TOTAL "bps-total"
> > > +#define QEMU_OPT_BPS_TOTAL_MAX "bps-total-max"
> > > +#define QEMU_OPT_BPS_TOTAL_MAX_LENGTH "bps-total-max-length"
> > > +#define QEMU_OPT_BPS_READ "bps-read"
> > > +#define QEMU_OPT_BPS_READ_MAX "bps-read-max"
> > > +#define QEMU_OPT_BPS_READ_MAX_LENGTH "bps-read-max-length"
> > > +#define QEMU_OPT_BPS_WRITE "bps-write"
> > > +#define QEMU_OPT_BPS_WRITE_MAX "bps-write-max"
> > > +#define QEMU_OPT_BPS_WRITE_MAX_LENGTH "bps-write-max-length"
> > > +#define QEMU_OPT_IOPS_SIZE "iops-size"
> > > +#define QEMU_OPT_THROTTLE_GROUP_NAME "throttle-group"
> > 
> > Please reuse include/qemu/throttle-options.h.
> 
> This would make the options too verbose, ie -drive
> driver=throttle,file=..,throttling.iops-total=..,throttling. Eventually of
> course throttle-options.h will be replaced.
> 
> However, this can't be done at this point anyway, since the throttling.[...]
> options are consumed by the already existing throttling code, and are
> unavailable for the driver.

There are ways of reusing include/qemu/throttle-options.h, I'm just not
sure if they are worth it.

In block/block-backend.c:

  #define THROTTLE_OPTION_PREFIX "throttling."
  #include "qemu/throttle-options.h"

In block/throttle.c:

  #include "qemu/throttle-options.h"

In include/qemu/throttle-options.h:

  #define THROTTLE_OPTION_IOPS_TOTAL (THROTTLE_OPTION_PREFIX "iops-total")

  #define THROTTLE_OPTS \
        { \
            .name = THROTTLE_OPTION_IOPS_TOTAL,\
            .type = QEMU_OPT_NUMBER,\
            .help = "limit total I/O operations per second",\
        },{ \

to generate either "throttling.iops-total" or "iops-total", depending on
the environment in which the file was included.

This reuses the same .h file but it's also a little ugly because of the
C macro (ab)use.

> > > diff --git a/include/block/throttle.h b/include/block/throttle.h
> > > new file mode 100644
> > > index 0000000000..fed24f02a6
> > > --- /dev/null
> > > +++ b/include/block/throttle.h
> > 
> > What is the purpose of this header?
> > 
> > Header files in include/* are "public" APIs that other parts of QEMU can
> > use.  There is no user other than block/throttle.c so please keep it
> > private in the .c file.
> 
> Eventually the qmp/hmp/qom related stuff will be in a header file, and this
> is why I put them from the beginning separately. But yes, in this case it is
> not needed. Also, these probably should go to include/qemu/throttle.h
> instead of a separate header file.

Patches must be self-contained because reviewers don't necessarily know
future plans and if those future patches are never merged then the
source tree is in an incomplete state.

There are rare cases where it's really necessary for a patch to add
something that is unused.  In that case the intention should be clear
from the patch or commit description (e.g. "let's change this now so
the next patch can take advantage of it").

signature.asc
Description: PGP signature