[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option
From: |
Vladimir Sementsov-Ogievskiy |
Subject: |
Re: [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option |
Date: |
Tue, 16 Apr 2019 07:18:56 +0000 |
13.04.2019 19:53, Max Reitz wrote:
> This new error option allows users of blkdebug to inject errors only on
> certain kinds of I/O operations. Users usually want to make a very
> specific operation fail, not just any; but right now they simply hope
> that the event that triggers the error injection is followed up with
> that very operation. That may not be true, however, because the block
> layer is changing (including blkdebug, which may increase the number of
> types of I/O operations on which to inject errors).
>
> The new option's default has been chosen to keep backwards
> compatibility.
>
> Note that similar to the internal representation, we could choose to
> expose this option as a list of I/O types. But there is no practical
> use for this, because as described above, users usually know exactly
> which kind of operation they want to make fail, so there is no need to
> specify multiple I/O types at once. In addition, exposing this option
> as a list would require non-trivial changes to qemu_opts_absorb_qdict().
>
> Signed-off-by: Max Reitz <address@hidden>
> ---
> qapi/block-core.json | 26 +++++++++++++++++++++++
> block/blkdebug.c | 50 ++++++++++++++++++++++++++++++++++++--------
> 2 files changed, 67 insertions(+), 9 deletions(-)
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..5141e91172 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -3235,6 +3235,26 @@
> 'l1_shrink_write_table', 'l1_shrink_free_l2_clusters',
> 'cor_write'] }
>
> +##
> +# @BlkdebugIOType:
> +#
> +# Kinds of I/O that blkdebug can inject errors in.
> +#
> +# @read: .bdrv_co_preadv()
> +#
> +# @write: .bdrv_co_pwritev()
> +#
> +# @write-zeroes: .bdrv_co_pwrite_zeroes()
> +#
> +# @discard: .bdrv_co_pdiscard()
> +#
> +# @flush: .bdrv_co_flush_to_disk()
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'BlkdebugIOType',
> + 'data': [ 'read', 'write', 'write-zeroes', 'discard', 'flush' ] }
> +
> ##
> # @BlkdebugInjectErrorOptions:
> #
> @@ -3245,6 +3265,11 @@
> # @state: the state identifier blkdebug needs to be in to
> # actually trigger the event; defaults to "any"
> #
> +# @iotype: the type of I/O operations on which this error should
> +# be injected; defaults to "all read, write,
> +# write-zeroes, discard, and flush operations"
Don't you want defaults to any?
> +# (since: 4.1)
> +#
> # @errno: error identifier (errno) to be returned; defaults to
> # EIO
> #
> @@ -3262,6 +3287,7 @@
> { 'struct': 'BlkdebugInjectErrorOptions',
> 'data': { 'event': 'BlkdebugEvent',
> '*state': 'int',
> + '*iotype': 'BlkdebugIOType',
> '*errno': 'int',
> '*sector': 'int',
> '*once': 'bool',
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index efd9441625..ca84b12e32 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -75,6 +75,7 @@ typedef struct BlkdebugRule {
> int state;
> union {
> struct {
> + uint64_t iotype_mask;
> int error;
> int immediately;
> int once;
> @@ -91,6 +92,9 @@ typedef struct BlkdebugRule {
> QSIMPLEQ_ENTRY(BlkdebugRule) active_next;
> } BlkdebugRule;
>
> +QEMU_BUILD_BUG_MSG(BLKDEBUGIO_TYPE__MAX > 64,
> + "BlkdebugIOType mask does not fit into an uint64_t");
> +
> static QemuOptsList inject_error_opts = {
> .name = "inject-error",
> .head = QTAILQ_HEAD_INITIALIZER(inject_error_opts.head),
> @@ -103,6 +107,10 @@ static QemuOptsList inject_error_opts = {
> .name = "state",
> .type = QEMU_OPT_NUMBER,
> },
> + {
> + .name = "iotype",
> + .type = QEMU_OPT_STRING,
> + },
> {
> .name = "errno",
> .type = QEMU_OPT_NUMBER,
> @@ -162,6 +170,8 @@ static int add_rule(void *opaque, QemuOpts *opts, Error
> **errp)
> int event;
> struct BlkdebugRule *rule;
> int64_t sector;
> + BlkdebugIOType iotype;
> + Error *local_error = NULL;
>
> /* Find the right event for the rule */
> event_name = qemu_opt_get(opts, "event");
> @@ -192,6 +202,26 @@ static int add_rule(void *opaque, QemuOpts *opts, Error
> **errp)
> sector = qemu_opt_get_number(opts, "sector", -1);
> rule->options.inject.offset =
> sector == -1 ? -1 : sector * BDRV_SECTOR_SIZE;
> +
> + iotype = qapi_enum_parse(&BlkdebugIOType_lookup,
> + qemu_opt_get(opts, "iotype"),
> + BLKDEBUGIO_TYPE__MAX, &local_error);
Prefix BLKDEBUGIO seems strange. Looks like a bug in qapi, I think it should
make BLKDEBUG prefix in this case. Don't you want use "'prefix': 'BLKDBG'" like
it is done for blkdebug events?
> + if (local_error) {
> + error_propagate(errp, local_error);
> + return -1;
> + }
> + if (iotype != BLKDEBUGIO_TYPE__MAX) {
> + rule->options.inject.iotype_mask = (1ull << iotype);
> + } else {
> + /* Apply the default */
> + rule->options.inject.iotype_mask =
> + (1ull << BLKDEBUGIO_TYPE_READ)
> + | (1ull << BLKDEBUGIO_TYPE_WRITE)
> + | (1ull << BLKDEBUGIO_TYPE_WRITE_ZEROES)
> + | (1ull << BLKDEBUGIO_TYPE_DISCARD)
> + | (1ull << BLKDEBUGIO_TYPE_FLUSH);
and here (1ull << BLKDEBUGIO_TYPE__MAX) - 1 ?
[..]
It is my first my first look at blkdebug internals, but patch seems OK for me,
so, with or without my tiny suggestions:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
--
Best regards,
Vladimir
- [Qemu-block] [PATCH v3 0/7] qemu-img: Add salvaging mode to convert, Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 1/7] qemu-img: Move quiet into ImgConvertState, Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 2/7] qemu-img: Add salvaging mode to convert, Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option, Max Reitz, 2019/04/13
- Re: [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option,
Vladimir Sementsov-Ogievskiy <=
- [Qemu-block] [PATCH v3 4/7] blkdebug: Add "none" event, Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 5/7] blkdebug: Inject errors on .bdrv_co_block_status(), Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 6/7] iotests: Test qemu-img convert --salvage, Max Reitz, 2019/04/13
- [Qemu-block] [PATCH v3 7/7] iotests: Test qemu-img convert -C --salvage, Max Reitz, 2019/04/13