Re: [PATCH 2/2] block: add logging facility for long standing IO requests

[Denis V. Lunev]

On 7/29/20 1:30 PM, Stefan Hajnoczi wrote:
> On Fri, Jul 10, 2020 at 08:27:11PM +0300, Denis V. Lunev wrote:
>> +static void block_acct_report_long(BlockAcctStats *stats,
>> +                                   BlockAcctCookie *cookie, int64_t 
>> latency_ns)
>> +{
>> +    unsigned latency_ms = latency_ns / 1000000;
> Please use SCALE_MS.
sure

>
>> +    int64_t start_time_host_s;
>> +    char buf[64];
>> +    struct tm t;
>> +    BlockDriverState *bs;
>> +
>> +    if (cookie->type == BLOCK_ACCT_NONE) {
>> +        return;
>> +    }
>> +    if (stats->latency_log_threshold_ms == 0) {
>> +        return;
>> +    }
>> +    if (latency_ms < stats->latency_log_threshold_ms) {
>> +        return;
>> +    }
>> +
>> +    start_time_host_s = cookie->start_time_ns / 1000000000ull;
> Please use NANOSECONDS_PER_SECOND.
sure

>> diff --git a/blockdev.c b/blockdev.c
>> index 31d5eaf6bf..d87260958c 100644
>> --- a/blockdev.c
>> +++ b/blockdev.c
>> @@ -625,7 +625,8 @@ static BlockBackend *blockdev_init(const char *file, 
>> QDict *bs_opts,
>>  
>>          bs->detect_zeroes = detect_zeroes;
>>  
>> -        block_acct_setup(blk_get_stats(blk), account_invalid, 
>> account_failed);
>> +        block_acct_setup(blk_get_stats(blk), account_invalid, 
>> account_failed,
>> +                qemu_opt_get_number(opts, "latency-log-threshold", 0));
>>  
>>          if (!parse_stats_intervals(blk_get_stats(blk), interval_list, 
>> errp)) {
>>              blk_unref(blk);
>> @@ -3740,6 +3741,10 @@ QemuOptsList qemu_common_drive_opts = {
>>              .type = QEMU_OPT_BOOL,
>>              .help = "whether to account for failed I/O operations "
>>                      "in the statistics",
>> +        },{
>> +            .name = "latency-log-threshold",
>> +            .type = QEMU_OPT_STRING,
> QEMU_OPT_NUMBER?
good catch! we have this parameter on by default (10 seconds) and never
passed it from the command line :)

Thus should we keep this off be default or on? :)

Den