[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v10 1/3] block: add the command line support
|
From: |
Zhi Yong Wu |
|
Subject: |
Re: [Qemu-devel] [PATCH v10 1/3] block: add the command line support |
|
Date: |
Wed, 2 Nov 2011 11:44:10 +0800 |
On Wed, Nov 2, 2011 at 6:43 AM, Ryan Harper <address@hidden> wrote:
> * Zhi Yong Wu <address@hidden> [2011-11-01 02:44]:
>> Signed-off-by: Zhi Yong Wu <address@hidden>
>> ---
>> block.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> block.h | 4 ++++
>> block_int.h | 29 +++++++++++++++++++++++++++++
>> blockdev.c | 32 ++++++++++++++++++++++++++++++++
>> qemu-config.c | 24 ++++++++++++++++++++++++
>> qemu-options.hx | 1 +
>> 6 files changed, 130 insertions(+), 0 deletions(-)
>
> I suggest changing the Subject of the patch to include something about
> blkio limits. This will in searching git log later that this patch was
> introducing blkio command line support.
Good suggestion
>
>>
>> diff --git a/block.c b/block.c
>> index 9bb236c..8f08dc5 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -30,6 +30,7 @@
>> #include "qjson.h"
>> #include "qemu-coroutine.h"
>> #include "qmp-commands.h"
>> +#include "qemu-timer.h"
>>
>> #ifdef CONFIG_BSD
>> #include <sys/types.h>
>> @@ -105,6 +106,37 @@ int is_windows_drive(const char *filename)
>> }
>> #endif
>>
>> +/* throttling disk I/O limits */
>> +static void bdrv_block_timer(void *opaque)
>> +{
>> + BlockDriverState *bs = opaque;
>> +
>> + qemu_co_queue_next(&bs->throttled_reqs);
>> +}
>> +
>> +void bdrv_io_limits_enable(BlockDriverState *bs)
>> +{
>> + bs->io_limits_enabled = true;
>
> We may want to move this to the end of the function so we don't raise
> the flag that we're enabled until we're done setting everything up.
> I don't think we'll race right now, but no reason not to move this to
> the end.
OK.
>
>> + qemu_co_queue_init(&bs->throttled_reqs);
>> +
>> + bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
>> + bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
>> + bs->slice_start = qemu_get_clock_ns(vm_clock);
>> + bs->slice_end = bs->slice_start + bs->slice_time;
>> + memset(&bs->io_disps, 0, sizeof(bs->io_disps));
>> +}
>> +
>> +bool bdrv_io_limits_enabled(BlockDriverState *bs)
>> +{
>> + BlockIOLimit *io_limits = &bs->io_limits;
>> + return io_limits->bps[BLOCK_IO_LIMIT_READ]
>> + || io_limits->bps[BLOCK_IO_LIMIT_WRITE]
>> + || io_limits->bps[BLOCK_IO_LIMIT_TOTAL]
>> + || io_limits->iops[BLOCK_IO_LIMIT_READ]
>> + || io_limits->iops[BLOCK_IO_LIMIT_WRITE]
>> + || io_limits->iops[BLOCK_IO_LIMIT_TOTAL];
>> +}
>> +
>> /* check if the path starts with "<protocol>:" */
>> static int path_has_protocol(const char *path)
>> {
>> @@ -1519,6 +1551,14 @@ void bdrv_get_geometry_hint(BlockDriverState *bs,
>> *psecs = bs->secs;
>> }
>>
>> +/* throttling disk io limits */
>> +void bdrv_set_io_limits(BlockDriverState *bs,
>> + BlockIOLimit *io_limits)
>> +{
>> + bs->io_limits = *io_limits;
>> + bs->io_limits_enabled = bdrv_io_limits_enabled(bs);
>> +}
>> +
>> /* Recognize floppy formats */
>> typedef struct FDFormat {
>> FDriveType drive;
>> diff --git a/block.h b/block.h
>> index 38cd748..bc8315d 100644
>> --- a/block.h
>> +++ b/block.h
>> @@ -89,6 +89,10 @@ void bdrv_info(Monitor *mon, QObject **ret_data);
>> void bdrv_stats_print(Monitor *mon, const QObject *data);
>> void bdrv_info_stats(Monitor *mon, QObject **ret_data);
>>
>> +/* disk I/O throttling */
>> +void bdrv_io_limits_enable(BlockDriverState *bs);
>> +bool bdrv_io_limits_enabled(BlockDriverState *bs);
>> +
>> void bdrv_init(void);
>> void bdrv_init_with_whitelist(void);
>> BlockDriver *bdrv_find_protocol(const char *filename);
>> diff --git a/block_int.h b/block_int.h
>> index f4547f6..b835ef6 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -34,6 +34,12 @@
>> #define BLOCK_FLAG_ENCRYPT 1
>> #define BLOCK_FLAG_COMPAT6 4
>>
>> +#define BLOCK_IO_LIMIT_READ 0
>> +#define BLOCK_IO_LIMIT_WRITE 1
>> +#define BLOCK_IO_LIMIT_TOTAL 2
>> +
>> +#define BLOCK_IO_SLICE_TIME 100000000
>> +
>> #define BLOCK_OPT_SIZE "size"
>> #define BLOCK_OPT_ENCRYPT "encryption"
>> #define BLOCK_OPT_COMPAT6 "compat6"
>> @@ -50,6 +56,16 @@ typedef struct AIOPool {
>> BlockDriverAIOCB *free_aiocb;
>> } AIOPool;
>>
>> +typedef struct BlockIOLimit {
>> + uint64_t bps[3];
>> + uint64_t iops[3];
>> +} BlockIOLimit;
>> +
>> +typedef struct BlockIODisp {
>> + uint64_t bytes[2];
>> + uint64_t ios[2];
>> +} BlockIODisp;
>> +
>> struct BlockDriver {
>> const char *format_name;
>> int instance_size;
>> @@ -184,6 +200,16 @@ struct BlockDriverState {
>>
>> void *sync_aiocb;
>>
>> + /* the time for latest disk I/O */
>> + int64_t slice_time;
>> + int64_t slice_start;
>> + int64_t slice_end;
>> + BlockIOLimit io_limits;
>> + BlockIODisp io_disps;
>> + CoQueue throttled_reqs;
>> + QEMUTimer *block_timer;
>> + bool io_limits_enabled;
>> +
>> /* I/O stats (display with "info blockstats"). */
>> uint64_t nr_bytes[BDRV_MAX_IOTYPE];
>> uint64_t nr_ops[BDRV_MAX_IOTYPE];
>> @@ -227,6 +253,9 @@ void *qemu_aio_get(AIOPool *pool, BlockDriverState *bs,
>> BlockDriverCompletionFunc *cb, void *opaque);
>> void qemu_aio_release(void *p);
>>
>> +void bdrv_set_io_limits(BlockDriverState *bs,
>> + BlockIOLimit *io_limits);
>> +
>> #ifdef _WIN32
>> int is_windows_drive(const char *filename);
>> #endif
>> diff --git a/blockdev.c b/blockdev.c
>> index 0827bf7..faf8c56 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -235,6 +235,9 @@ DriveInfo *drive_init(QemuOpts *opts, int
>> default_to_scsi)
>> int on_read_error, on_write_error;
>> const char *devaddr;
>> DriveInfo *dinfo;
>> + BlockIOLimit io_limits;
>> + bool bps_iol;
>> + bool iops_iol;
>> int snapshot = 0;
>> int ret;
>>
>> @@ -353,6 +356,32 @@ DriveInfo *drive_init(QemuOpts *opts, int
>> default_to_scsi)
>> }
>> }
>>
>> + /* disk I/O throttling */
>> + io_limits.bps[BLOCK_IO_LIMIT_TOTAL] =
>> + qemu_opt_get_number(opts, "bps", 0);
>> + io_limits.bps[BLOCK_IO_LIMIT_READ] =
>> + qemu_opt_get_number(opts, "bps_rd", 0);
>> + io_limits.bps[BLOCK_IO_LIMIT_WRITE] =
>> + qemu_opt_get_number(opts, "bps_wr", 0);
>> + io_limits.iops[BLOCK_IO_LIMIT_TOTAL] =
>> + qemu_opt_get_number(opts, "iops", 0);
>> + io_limits.iops[BLOCK_IO_LIMIT_READ] =
>> + qemu_opt_get_number(opts, "iops_rd", 0);
>> + io_limits.iops[BLOCK_IO_LIMIT_WRITE] =
>> + qemu_opt_get_number(opts, "iops_wr", 0);
>> +
>> + bps_iol = (io_limits.bps[BLOCK_IO_LIMIT_TOTAL] != 0)
>> + && ((io_limits.bps[BLOCK_IO_LIMIT_READ] != 0)
>> + || (io_limits.bps[BLOCK_IO_LIMIT_WRITE] != 0));
>> + iops_iol = (io_limits.iops[BLOCK_IO_LIMIT_TOTAL] != 0)
>> + && ((io_limits.iops[BLOCK_IO_LIMIT_READ] != 0)
>> + || (io_limits.iops[BLOCK_IO_LIMIT_WRITE] != 0));
>> + if (bps_iol || iops_iol) {
>> + error_report("bps(iops) and bps_rd/bps_wr(iops_rd/iops_wr)"
>> + "cannot be used at the same time");
>> + return NULL;
>> + }
>> +
>
>
> How about extracting this section into the bdrv_set_io_limits() function or
> some
> other common setting function. This should be the same code that's used when
Good idea.
> modifying/setting these values via the monitor as well (minus how you obtain
> the
> input values - option parsings vs. monitor values).
For monitor, they are obtained via the way below:
In blockdev.c:do_block_set_io_throttle(),
int64_t bps = qdict_get_try_int(qdict, "bps", -1);
int64_t bps_rd = qdict_get_try_int(qdict, "bps_rd", -1);
int64_t bps_wr = qdict_get_try_int(qdict, "bps_wr", -1);
int64_t iops = qdict_get_try_int(qdict, "iops", -1);
int64_t iops_rd = qdict_get_try_int(qdict, "iops_rd", -1);
int64_t iops_wr = qdict_get_try_int(qdict, "iops_wr", -1);
When they are not specified, the handling method is a bit different.
Actually they are also limited when these values are modified.
if ((bps != 0 && (bps_rd != 0 || bps_wr != 0))
|| (iops != 0 && (iops_rd != 0 || iops_wr != 0))) {
qerror_report(QERR_INVALID_PARAMETER_COMBINATION);
return -1;
}
But i also think that it is one good idea to put them in one common function.
>
>> on_write_error = BLOCK_ERR_STOP_ENOSPC;
>> if ((buf = qemu_opt_get(opts, "werror")) != NULL) {
>> if (type != IF_IDE && type != IF_SCSI && type != IF_VIRTIO && type
>> != IF_NONE) {
>> @@ -460,6 +489,9 @@ DriveInfo *drive_init(QemuOpts *opts, int
>> default_to_scsi)
>>
>> bdrv_set_on_error(dinfo->bdrv, on_read_error, on_write_error);
>>
>> + /* disk I/O throttling */
>> + bdrv_set_io_limits(dinfo->bdrv, &io_limits);
>> +
>> switch(type) {
>> case IF_IDE:
>> case IF_SCSI:
>> diff --git a/qemu-config.c b/qemu-config.c
>> index 597d7e1..1aa080f 100644
>> --- a/qemu-config.c
>> +++ b/qemu-config.c
>> @@ -85,6 +85,30 @@ static QemuOptsList qemu_drive_opts = {
>> .name = "readonly",
>> .type = QEMU_OPT_BOOL,
>> .help = "open drive file as read-only",
>> + },{
>> + .name = "iops",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit total I/O operations per second",
>> + },{
>> + .name = "iops_rd",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit read operations per second",
>> + },{
>> + .name = "iops_wr",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit write operations per second",
>> + },{
>> + .name = "bps",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit total bytes per second",
>> + },{
>> + .name = "bps_rd",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit read bytes per second",
>> + },{
>> + .name = "bps_wr",
>> + .type = QEMU_OPT_NUMBER,
>> + .help = "limit write bytes per second",
>> },
>> { /* end of list */ }
>> },
>> diff --git a/qemu-options.hx b/qemu-options.hx
>> index 681eaf1..25a7be7 100644
>> --- a/qemu-options.hx
>> +++ b/qemu-options.hx
>> @@ -136,6 +136,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>> "
>> [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>> " [,serial=s][,addr=A][,id=name][,aio=threads|native]\n"
>> " [,readonly=on|off]\n"
>> + "
>> [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]][[,iops=i]|[[,iops_rd=r][,iops_wr=w]]\n"
>> " use 'file' as a drive image\n", QEMU_ARCH_ALL)
>> STEXI
>> address@hidden -drive @var{option}[,@var{option}[,@var{option}[,...]]]
>> --
>> 1.7.6
>>
>
> --
> Ryan Harper
> Software Engineer; Linux Technology Center
> IBM Corp., Austin, Tx
> address@hidden
>
>
>
--
Regards,
Zhi Yong Wu