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 <[email protected]> >> --- >> 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 <[email protected]>
Thanks!
Max
signature.asc
Description: OpenPGP digital signature
