[Devel] [PATCH VZ10 03/11] drivers/md/dm-stats: add new hist_total_latency option
Pavel Tikhomirov
ptikhomirov at virtuozzo.com
Fri Jul 24 14:05:59 MSK 2026
On 7/23/26 22:38, Andrey Zhadchenko wrote:
>
>
> 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.
The wrongness of placements also shows in Patch 5:
- if (s->n_histogram_entries) {
- unsigned int lo = 0, hi = s->n_histogram_entries + 1;
-
+ if (s->n_histograms) {
if ((s->stat_flags & STAT_HIST_TOTAL_LATENCY) &&
stats_aux->histogram_duration_ns)
duration = stats_aux->histogram_duration_ns;
- while (lo + 1 < hi) {
- unsigned int mid = (lo + hi) / 2;
-
- if (s->histogram_boundaries[mid - 1] > duration)
where you work your changes around it.
Original code clearly first sets durations based on stat_flags and then uses them,
that is only natural to follow this order.
I doubt that saving some time on not adding if in the preparation saves us much.
This also saves us from writing duration twice on histogram path.
>
>>
>> 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
>>
>
--
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.
More information about the Devel
mailing list