Re: [Qemu-devel] [PATCH v3 3/3] blkdebug: Add latency injection rule type

[Marc Olson <marcolso@amazon.com>]

On 2/13/19 7:48 AM, Max Reitz wrote:
> On 12.02.19 22:21, Marc Olson wrote:
>> On 1/11/19 7:00 AM, Max Reitz wrote:
>>> On 12.11.18 08:06, Marc Olson wrote:
> [...]
>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index d4fe710..72f7861 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -3057,6 +3057,34 @@
>>>>                '*immediately': 'bool' } }
>>>>      ##
>>>> +# @BlkdebugDelayOptions:
>>>> +#
>>>> +# Describes a single latency injection for blkdebug.
>>>> +#
>>>> +# @event:       trigger event
>>>> +#
>>>> +# @state:       the state identifier blkdebug needs to be in to
>>>> +#               actually trigger the event; defaults to "any"
>>>> +#
>>>> +# @latency:     The delay to add to an I/O, in microseconds.
>>>> +#
>>>> +# @sector:      specifies the sector index which has to be affected
>>>> +#               in order to actually trigger the event; defaults to
>>>> "any
>>>> +#               sector"
>>>> +#
>>>> +# @once:        disables further events after this one has been
>>>> +#               triggered; defaults to false
>>>> +#
>>>> +# Since: 3.1
>>> Well, 4.0 now, sorry...
>> Baking version numbers into code is downright silly. Every single
>> version of this patch has made a comment along the lines of, "oops, it
>> didn't get reviewed in time for the next version bump, so you have to
>> resubmit." With a fast moving project, this is nonsense. If you're
>> looking at the code, you should have access to the git history as well
>> to determine the version.
> True, but these comments are used to generate documentation (e.g.
> docs/interopt/qemu-qmp-ref.7 in the build directory).  So they are used
> by people who don't have access to the git history.
>
> It might be possible to generate that information from git-blame when
> generating the documentation, but how would trivial fixes be handled
> that are no functional changes?  For instance, it seems difficult to me
> to distinguish between a spelling change for some parameter description
> and a change in behavior.
>
> (Then again, we shouldn't have such behavioral changes, hm.)
>
> To me personally, the easiest thing would seem to be some convention to
> write ***INSERT VERSION HERE*** into the code and then the maintainer
> can just find an replace all instances of that when applying the
> patches.  But that sounds a bit silly...
>
> (I don't have an issue with adjusting the version numbers myself as they
> are, but it's just as hard for me to find and replace all of them as it
> is for you.  And since you're probably going to send a v4 anyway...)
>
> In the end, it's up to the QAPI maintainers.

IFF I submit by the end of the week, what version number shall I choose?


> [...]
>
>>>> diff --git a/tests/qemu-iotests/071 b/tests/qemu-iotests/071
>>>> index 48b4955..976f747 100755
>>>> --- a/tests/qemu-iotests/071
>>>> +++ b/tests/qemu-iotests/071
>>>> @@ -100,6 +100,69 @@ $QEMU_IO -c "open -o
>>>> driver=$IMGFMT,file.driver=blkdebug,file.inject-error.event
>>>>             -c 'read -P 42 0x38000 512'
>>>>      echo
>>>> +echo "=== Testing blkdebug latency through filename ==="
>>>> +echo
>>>> +
>>>> +$QEMU_IO -c "open -o
>>>> file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000
>>>> $TEST_IMG" \
>>>> +         -c 'aio_write -P 42 0x28000 512' \
>>>> +         -c 'aio_read -P 42 0x38000 512' \
>>>> +         | _filter_qemu_io
>>>> +
>>>> +echo
>>>> +echo "=== Testing blkdebug latency through file blockref ==="
>>>> +echo
>>>> +
>>>> +$QEMU_IO -c "open -o
>>>> driver=$IMGFMT,file.driver=blkdebug,file.inject-delay.event=write_aio,file.inject-delay.latency=10000,file.image.filename=$TEST_IMG"
>>>> \
>>>> +         -c 'aio_write -P 42 0x28000 512' \
>>>> +         -c 'aio_read -P 42 0x38000 512' \
>>>> +         | _filter_qemu_io
>>>> +
>>>> +# Using QMP is synchronous by default, so even though we would
>>>> +# expect reordering due to using the aio_* commands, they are
>>>> +# not. The purpose of this test is to verify that the driver
>>>> +# can be setup via QMP, and IO can complete. See the qemu-io
>>>> +# test above to prove delay functionality
>>> But it doesn't prove that because the output is filtered.  To prove it,
>>> you'd probably need to use null-co as the protocol (so there is as
>>> little noise as possible) and then parse the qemu-io output to show that
>>> the time is always above 10 ms.
>>>
>>> I leave it to you whether you'd like to go through that pain.
>> There's not a great way to prove it without doing a lot of parsing
>> changes in testing. I'll consider an update to this patch, but I think
>> this series has carried on long enough.
> I agree in this instance, but I'd like to note still that "this series
> has carried on long enough" is not an argument to merge bad code (or
> something incomplete without promise of a follow-up).  This is just a
> test for blkdebug, though, so it's OK (with the comment fixed, because
> it doesn't prove anything).
I wasn't implying that it's ok to merge bad code, but that at some point 
we have to just paint the shed a color and move on. At the risk of 
excess drama, at what point does a small addition become a complete 
tear-down and rebuild?
> (I'm sorry for not having looked at this series for so long, but qemu is
> not my own, so it isn't like I could pay for my wrong by accepting
> something incomplete -- if it were more important than this single test
> case.)
>
> Also, we do support Python for iotests, and parsing should be simpler
> there.  Since blkdebug is just a debugging driver, I'm fine with not
> knowing whether its features work, though.
>
> Maybe I'll write the test, that would kind of pay for my wrong...

I think the real fix is to make QMP support async IO, but if you'd like 
to do more parsing, that's fine too.

/marc