On 16.04.19 09:18, Vladimir Sementsov-Ogievskiy wrote: > 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 <mre...@redhat.com> >> --- >> 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?
See the commit message: > The new option's default has been chosen to keep backwards > compatibility. I can’t let it default to any because I’d like to add the block-status I/O type, and the current behavior is not to inject errors then. Changing that would break a range of iotests, and I’d rather avoid that. I think it makes sense to require users to specify @iotype if they want anything but the basic I/O types fail. Well, in fact, I think users always want to specify this option, but, well, we didn’t have it. >> +# (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? I think that was done for blkdebug events simply because the BLKDBG_* constants were used internally already, so it had to get that prefix so the code could remain unchanged. I can be persuaded to give it a BLKDEBUG_IO_TYPE prefix, because that would make sense. >> + 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 ? Would be correct now but not once I add block-status. > [..] > > 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 <vsement...@virtuozzo.com> Thanks! Max
signature.asc
Description: OpenPGP digital signature