[Devel] [PATCH VZ10 03/11] drivers/md/dm-stats: add new hist_total_latency option
Andrey Zhadchenko
andrey.zhadchenko at virtuozzo.com
Thu Jul 23 23:38:30 MSK 2026
On 7/20/26 16:37, Pavel Tikhomirov wrote:
>
>
> On 7/13/26 02:36, Andrey Zhadchenko wrote:
>> The dm-stats latency histogram accounts the service time of an I/O:
>> the time between the moment device-mapper starts the request and its
>> completion. Under high load, requests may spend a noticeable part
>> of their life queued before dispatch. The service time then differs
>> from the latency observed by userspace.
>>
>> Add a new region feature argument, hist_total_latency, which makes
>> the histogram account the total time the bio spent in the block
>> layer instead. The elapsed time is derived from the bio issue
>> timestamp maintained by the block cgroup infrastructure
>> (bio->bi_issue), exposed by the previously added
>> bio_issue_elapsed_ns().
>>
>> Only the histogram is affected: all other counters keep reporting
>> the service time. The option requires precise_timestamps (the
>> total latency is measured in nanoseconds) and a histogram to be
>> specified.
>>
>> https://virtuozzo.atlassian.net/browse/VSTOR-103846
>> Signed-off-by: Andrey Zhadchenko <andrey.zhadchenko at virtuozzo.com>
>> ---
>> .../admin-guide/device-mapper/statistics.rst | 19 ++++++++++++++---
>> drivers/md/dm-rq.c | 4 ++++
>> drivers/md/dm-stats.c | 21 ++++++++++++++++---
>> drivers/md/dm-stats.h | 3 +++
>> drivers/md/dm.c | 4 ++++
>> 5 files changed, 45 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/admin-guide/device-mapper/statistics.rst b/Documentation/admin-guide/device-mapper/statistics.rst
>> index 41ded0bc59335..d5b1b1013c087 100644
>> --- a/Documentation/admin-guide/device-mapper/statistics.rst
>> +++ b/Documentation/admin-guide/device-mapper/statistics.rst
>> @@ -70,6 +70,18 @@ Messages
>> used, the resulting times are in nanoseconds instead of
>> milliseconds. Precise timestamps are a little bit slower
>> to obtain than jiffies-based timestamps.
>> + hist_total_latency
>> + make the latency histogram account the total time a
>> + request spent in the block layer, from bio issue to
>> + completion, instead of just the service time observed by
>> + device-mapper. Under high load, when requests spend a
>> + significant time queued, the total latency is what
>> + userspace actually observes. This option requires
>> + precise_timestamps and a histogram, and only affects the
>> + histogram counters; it needs the bio issue timestamp,
>> + which is only maintained when CONFIG_BLK_CGROUP is
>> + enabled (without it, the histogram keeps accounting the
>> + service time).
>> histogram:n1,n2,n3,n4,...
>> collect histogram of latencies. The
>> numbers n1, n2, etc are times that represent the boundaries
>> @@ -124,10 +136,11 @@ Messages
>>
>> Output format:
>> <region_id>: <start_sector>+<length> <step> <program_id> <aux_data>
>> - precise_timestamps histogram:n1,n2,n3,...
>> + precise_timestamps hist_total_latency histogram:n1,n2,n3,...
>>
>> - The strings "precise_timestamps" and "histogram" are printed only
>> - if they were specified when creating the region.
>> + The strings "precise_timestamps", "hist_total_latency" and
>> + "histogram" are printed only if they were specified when creating
>> + the region.
>>
>> @stats_print <region_id> [<starting_line> <number_of_lines>]
>> Print counters for each step-sized area of a region.
>> diff --git a/drivers/md/dm-rq.c b/drivers/md/dm-rq.c
>> index 58ad4c94d6119..28bd496143770 100644
>> --- a/drivers/md/dm-rq.c
>> +++ b/drivers/md/dm-rq.c
>> @@ -129,6 +129,10 @@ static void rq_end_stats(struct mapped_device *md, struct request *orig)
>> if (unlikely(dm_stats_used(&md->stats))) {
>> struct dm_rq_target_io *tio = tio_from_request(orig);
>>
>> + if (md->stats.hist_total_latency && orig->bio)
>> + tio->stats_aux.histogram_duration_ns =
>> + bio_issue_elapsed_ns(orig->bio);
>> +
>> dm_stats_account_io(&md->stats, rq_data_dir(orig),
>> blk_rq_pos(orig), tio->n_sectors, true,
>> tio->duration_jiffies, &tio->stats_aux);
>> diff --git a/drivers/md/dm-stats.c b/drivers/md/dm-stats.c
>> index 1e5d988f44da6..2ede4b51c5a4c 100644
>> --- a/drivers/md/dm-stats.c
>> +++ b/drivers/md/dm-stats.c
>> @@ -60,6 +60,7 @@ struct dm_stat {
>> };
>>
>> #define STAT_PRECISE_TIMESTAMPS 1
>> +#define STAT_HIST_TOTAL_LATENCY 2
>>
>> struct dm_stats_last_position {
>> sector_t last_sector;
>> @@ -246,15 +247,17 @@ static void dm_stats_recalc_precise_timestamps(struct dm_stats *stats)
>> struct list_head *l;
>> struct dm_stat *tmp_s;
>> bool precise_timestamps = false;
>> + bool hist_total_latency = false;
>>
>> list_for_each(l, &stats->list) {
>> tmp_s = container_of(l, struct dm_stat, list_entry);
>> - if (tmp_s->stat_flags & STAT_PRECISE_TIMESTAMPS) {
>> + if (tmp_s->stat_flags & STAT_PRECISE_TIMESTAMPS)
>> precise_timestamps = true;
>> - break;
>> - }
>> + if (tmp_s->stat_flags & STAT_HIST_TOTAL_LATENCY)
>> + hist_total_latency = true;
>> }
>
> 1) Maybe we should not reuse dm_stats_recalc_precise_timestamps here and introduce either
> dm_stats_recalc_hist_total_latency() or even common dm_stats_recalc().
>
> Calculating hlist_total_latency in dm_stats_recalc_precise_timestamps() is obfuscating
> the code.
I will rename to something more generic.
>
> 2) Missing initialization of hlist_total_latency in dm_stats_init() along with
> precise_timestamps's one makes me think that there could be some race where
> our new variable is used uninitialized. Even if no, I'd add initialization there
> for just uniformity.
Yeah, I missed the initialization
>
>> stats->precise_timestamps = precise_timestamps;
>> + stats->hist_total_latency = hist_total_latency;
>> }
>>
>> static int dm_stats_create(struct dm_stats *stats, sector_t start, sector_t end,
>> @@ -306,6 +309,10 @@ static int dm_stats_create(struct dm_stats *stats, sector_t start, sector_t end,
>> if ((n_histogram_entries + 1) * (size_t)n_entries > DM_STAT_MAX_HISTOGRAM_ENTRIES)
>> return -EOVERFLOW;
>>
>> + if ((stat_flags & STAT_HIST_TOTAL_LATENCY) &&
>> + (!n_histogram_entries || !(stat_flags & STAT_PRECISE_TIMESTAMPS)))
>> + return -EINVAL;
>> +
>> if (!check_shared_memory(shared_alloc_size + histogram_alloc_size +
>> num_possible_cpus() * (percpu_alloc_size + histogram_alloc_size)))
>> return -ENOMEM;
>> @@ -509,6 +516,8 @@ static int dm_stats_list(struct dm_stats *stats, const char *program,
>> s->aux_data);
>> if (s->stat_flags & STAT_PRECISE_TIMESTAMPS)
>> DMEMIT(" precise_timestamps");
>> + if (s->stat_flags & STAT_HIST_TOTAL_LATENCY)
>> + DMEMIT(" hist_total_latency");
>> if (s->n_histogram_entries) {
>> unsigned int i;
>>
>> @@ -612,6 +621,10 @@ static void dm_stat_for_entry(struct dm_stat *s, size_t entry,
>> if (s->n_histogram_entries) {
>> unsigned int lo = 0, hi = s->n_histogram_entries + 1;
>>
>> + if ((s->stat_flags & STAT_HIST_TOTAL_LATENCY) &&
>> + stats_aux->histogram_duration_ns)
>> + duration = stats_aux->histogram_duration_ns;
>
> This hunk logically belongs to the code above:
I don't think so.
First of all, the mentioned hunk handles duration_ns which is used both
for histogram and total read/write times. I would even say that duration
is not used anywhere when the histogram is not requested, so assigning
it there is a premature optimization.
histogram_duration_ns, as a histogram-related counter, makes no sense in
block calculating total read/write ticks.
Also adding one more if/else with two checks (histogram_duration_ns can
be 0 if CONFIG_BLKCG is not set) inside another if/else is even harder
to read and comprehend.
>
> if (!(s->stat_flags & STAT_PRECISE_TIMESTAMPS)) {
> p->ticks[idx] += duration_jiffies;
> duration = jiffies_to_msecs(duration_jiffies);
> } else {
> p->ticks[idx] += stats_aux->duration_ns;
> if (s->stat_flags & STAT_HIST_TOTAL_LATENCY)
> duration = stats_aux->histogram_duration_ns;
> else
> duration = stats_aux->duration_ns;
> }
>
> No? Mixing duration setting code with other code, probably, only obfuscates things.
>
>> +
>> while (lo + 1 < hi) {
>> unsigned int mid = (lo + hi) / 2;
>>
>> @@ -1063,6 +1076,8 @@ static int message_stats_create(struct mapped_device *md,
>> goto ret_einval;
>> if (!strcasecmp(a, "precise_timestamps"))
>> stat_flags |= STAT_PRECISE_TIMESTAMPS;
>> + else if (!strcasecmp(a, "hist_total_latency"))
>> + stat_flags |= STAT_HIST_TOTAL_LATENCY;
>> else if (!strncasecmp(a, "histogram:", 10)) {
>> if (n_histogram_entries)
>> goto ret_einval;
>> diff --git a/drivers/md/dm-stats.h b/drivers/md/dm-stats.h
>> index c6728c8b41594..cce68414b97ee 100644
>> --- a/drivers/md/dm-stats.h
>> +++ b/drivers/md/dm-stats.h
>> @@ -14,11 +14,13 @@ struct dm_stats {
>> struct list_head list; /* list of struct dm_stat */
>> struct dm_stats_last_position __percpu *last;
>> bool precise_timestamps;
>> + bool hist_total_latency;
>> };
>>
>> struct dm_stats_aux {
>> bool merged;
>> unsigned long long duration_ns;
>> + unsigned long long histogram_duration_ns;
>> };
>>
>> int dm_stats_init(struct dm_stats *st);
>> @@ -41,6 +43,7 @@ static inline bool dm_stats_used(struct dm_stats *st)
>>
>> static inline void dm_stats_record_start(struct dm_stats *stats, struct dm_stats_aux *aux)
>> {
>> + aux->histogram_duration_ns = 0;
>> if (unlikely(stats->precise_timestamps))
>> aux->duration_ns = ktime_to_ns(ktime_get());
>> }
>> diff --git a/drivers/md/dm.c b/drivers/md/dm.c
>> index 130eec93fa10e..7029201480358 100644
>> --- a/drivers/md/dm.c
>> +++ b/drivers/md/dm.c
>> @@ -569,6 +569,10 @@ static void dm_io_acct(struct dm_io *io, bool end)
>> unlikely(dm_stats_used(&io->md->stats))) {
>> sector_t sector;
>>
>> + if (end && io->md->stats.hist_total_latency)
>> + io->stats_aux.histogram_duration_ns =
>> + bio_issue_elapsed_ns(bio);
>> +
>> if (unlikely(dm_io_flagged(io, DM_IO_WAS_SPLIT)))
>> sector = bio_end_sector(bio) - io->sector_offset;
>> else
>
More information about the Devel
mailing list