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 <[email protected]>
> ---
> .../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.
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.
> 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:
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.
_______________________________________________
Devel mailing list
[email protected]
https://lists.openvz.org/mailman/listinfo/devel
