[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: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v3 3/7] blkdebug: Add @iotype error option |
Date: |
Wed, 17 Apr 2019 18:36:23 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 |
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
- [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
- [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