Re: [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option

[Max Reitz]

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 <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?

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 <address@hidden>

Thanks!

Max

signature.asc
Description: OpenPGP digital signature