Re: [Qemu-devel] [PATCH 5/5] disk_deadlines: add info disk-deadlines option

["Denis V. Lunev" <den@openvz.org>]

On 09/08/2015 07:20 PM, Eric Blake wrote:
> On 09/08/2015 02:00 AM, Denis V. Lunev wrote:
>> From: Raushaniya Maksudova <rmaksudova@virtuozzo.com>
>>
>> This patch adds "info disk-deadlines" qemu-monitor option that prints
>> dump of all disk requests which caused a disk deadline in Guest OS
>> from the very start of Virtual Machine:
>>
>>     disk_id  type       size total_time        start_time
>> .--------------------------------------------------------
>>    ide0-hd1 FLUSH         0b 46.403s     22232930059574ns
>>    ide0-hd1 FLUSH         0b 57.591s     22451499241285ns
>>    ide0-hd1 FLUSH         0b 103.482s    22574100547397ns
>>
>> Signed-off-by: Raushaniya Maksudova <rmaksudova@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Luiz Capitulino <lcapitulino@redhat.com>
>> ---
> qapi interface review only:
>
>
>> +++ b/qapi-schema.json
>> @@ -3808,3 +3808,36 @@
>>   
>>   # Rocker ethernet network switch
>>   { 'include': 'qapi/rocker.json' }
>> +
>> +## @DiskDeadlinesInfo
>> +#
>> +# Contains info about late requests which caused VM stopping
>> +#
>> +# @disk-id: name of disk (unique for each disk)
> Mark this with '#optional', and maybe describe why it would be missing.
>   Does this correspond to the BDS node name where the deadline expired,
> in which case 'node' might be a nicer name than 'disk-id'?
as far as I could understand the code this is not BDS node name. This
name is bound to a name of a hardware device under which we could
have several block drivers at the moment. There is no query by
this name at the moment and the identifier collected is good enough
for us for a while to understand and debug the code.

Exact name is to be defined though. Originally I would like to bind
it to device name deadlines are attached to and as it would not be
good to calculate them in each BDS.

Anyway, exact meaning of this 'id' will be defined when we will decide
on proper attach point, whether it will be generic block code or filter
driver or whatever also.

Does it sound good for you? All your suggestions are welcome.

>> +#
>> +# @type: type of request could be READ, WRITE or FLUSH
> Likewise for using #optional. Please make this an enum type, not an
> open-coded string.
ok

>> +#
>> +# @size: size in bytes
> of the failed request? Should you also mention which offset the failed
> request started at?
I'll add this. This info is not accessible in stats and that's why it
was not added. If the code will be a part of the block layer
or will be in the driver filter that would be not a problem.

>> +#
>> +# @total-time-ns: total time of request execution
>> +#
>> +# @start-time-ns: indicates the start of request execution
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'struct': 'DiskDeadlinesInfo',
>> +  'data'  : { '*disk-id': 'str',
>> +              '*type': 'str',
>> +              'size': 'uint64',
>> +              'total-time-ns': 'uint64',
>> +              'start-time-ns': 'uint64' } }
>> +##
>> +# @query-disk-deadlines:
>> +#
>> +# Returns information about last late disk requests.
>> +#
>> +# Returns: a list of @DiskDeadlinesInfo
>> +#
>> +# Since: 2.5
>> +##
>> +{ 'command': 'query-disk-deadlines', 'returns': ['DiskDeadlinesInfo'] }
> Should it be possible to filter to deadlines missed for a specific node,
> by having an arguments with an optional node name?
ok, this seems quite reasonable.

> Should any of the existing query-block or similar commands be modified
> to make it obvious that there are missed deadline stats, and that it
> would be useful to call query-disk-deadlines to learn more about them?
>
What do you think if we will also provide the list of pending requests using
similar API? Would it be useful for others?

Den