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

[Vladimir Sementsov-Ogievskiy]

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