在 15/6/10 上午5:21, Mikulas Patocka 写道:
> This patch makes it possible to use precise timestamps with nanosecond
> granularity in dm statistics.
>
> Signed-off-by: Mikulas Patocka <mpatocka@xxxxxxxxxx>
>
> ---
> Documentation/device-mapper/statistics.txt | 25 ++++-
> drivers/md/dm-stats.c | 139 +++++++++++++++++++++--------
> drivers/md/dm-stats.h | 4
> 3 files changed, 125 insertions(+), 43 deletions(-)
>
> Index: linux-4.1-rc7/drivers/md/dm-stats.c
> ===================================================================
> --- linux-4.1-rc7.orig/drivers/md/dm-stats.c 2015-06-08 16:02:27.000000000 +0200
> +++ linux-4.1-rc7/drivers/md/dm-stats.c 2015-06-08 16:38:43.000000000 +0200
> @@ -33,13 +33,14 @@ struct dm_stat_percpu {
>
[snip]
> @@ -560,8 +578,17 @@ void dm_stats_account_io(struct dm_stats
>
> rcu_read_lock();
>
> - list_for_each_entry_rcu(s, &stats->list, list_entry)
> - __dm_stat_bio(s, bi_rw, bi_sector, end_sector, end, duration, stats_aux);
> + got_precise_time = false;
> + list_for_each_entry_rcu(s, &stats->list, list_entry) {
> + if (s->stat_flags & STAT_PRECISE_TIMESTAMPS && !got_precise_time) {
> + if (!end)
> + stats_aux->duration_ns = ktime_to_ns(ktime_get());
> + else
> + stats_aux->duration_ns = ktime_to_ns(ktime_get()) - stats_aux->duration_ns;
> + got_precise_time = true;
> + }
There are many conditions user space processes may query the global
timer resource frequently. ktime_get() will call spin_lock when
accessing global timer resource, calling ktime_get() for each list entry
might introduce locking contention when stats->list is quite large. A
satisfied solution might be something like this,
+ now = ktime_to_ns(ktime_get());
+ list_for_each_entry_rcu(s, &stats->list, list_entry) {
+ if (s->stat_flags & STAT_PRECISE_TIMESTAMPS && !got_precise_time) {
+ if (!end)
+ stats_aux->duration_ns = now;
+ else
+ stats_aux->duration_ns = now - stats_aux->duration_ns;
+ got_precise_time = true;
+ }
In this case the global timer could be access only once, locking
contention should be much less. Yes, it looks to be some inaccurate, but
for hard disk I/O latency measurement, the difference almost could be
ignored.
> + __dm_stat_bio(s, bi_rw, bi_sector, end_sector, end, duration_jiffies, stats_aux);
> + }
>
> rcu_read_unlock();
> }
[snip]
> @@ -772,21 +803,31 @@ static int message_stats_create(struct m
> unsigned long long start, end, len, step;
> unsigned divisor;
> const char *program_id, *aux_data;
> + unsigned stat_flags = 0;
> +
> + struct dm_arg_set as, as_backup;
> + const char *a;
> + unsigned feature_args;
>
> /*
> * Input format:
> - * <range> <step> [<program_id> [<aux_data>]]
> + * <range> <step> [<extra_parameters> <parameters>] [<program_id> [<aux_data>]]
> */
>
> - if (argc < 3 || argc > 5)
> + if (argc < 3)
> return -EINVAL;
>
> - if (!strcmp(argv[1], "-")) {
> + as.argc = argc;
> + as.argv = argv;
> + dm_consume_args(&as, 1);
> +
> + a = dm_shift_arg(&as);
> + if (!strcmp(a, "-")) {
> start = 0;
> len = dm_get_size(md);
> if (!len)
> len = 1;
> - } else if (sscanf(argv[1], "%llu+%llu%c", &start, &len, &dummy) != 2 ||
> + } else if (sscanf(a, "%llu+%llu%c", &start, &len, &dummy) != 2 ||
> start != (sector_t)start || len != (sector_t)len)
> return -EINVAL;
>
> @@ -794,7 +835,8 @@ static int message_stats_create(struct m
> if (start >= end)
> return -EINVAL;
>
> - if (sscanf(argv[2], "/%u%c", &divisor, &dummy) == 1) {
> + a = dm_shift_arg(&as);
> + if (sscanf(a, "/%u%c", &divisor, &dummy) == 1) {
> if (!divisor)
> return -EINVAL;
> step = end - start;
> @@ -802,18 +844,39 @@ static int message_stats_create(struct m
> step++;
> if (!step)
> step = 1;
> - } else if (sscanf(argv[2], "%llu%c", &step, &dummy) != 1 ||
> + } else if (sscanf(a, "%llu%c", &step, &dummy) != 1 ||
> step != (sector_t)step || !step)
> return -EINVAL;
>
> + as_backup = as;
> + a = dm_shift_arg(&as);
> + if (a && sscanf(a, "%u%c", &feature_args, &dummy) == 1) {
> + while (feature_args--) {
> + a = dm_shift_arg(&as);
> + if (!a)
> + return -EINVAL;
> + if (!strcasecmp(a, "precise_timestamps"))
> + stat_flags |= STAT_PRECISE_TIMESTAMPS;
> + else
> + return -EINVAL;
> + }
> + } else {
> + as = as_backup;
> + }
> +
> program_id = "-";
> aux_data = "-";
>
> - if (argc > 3)
> - program_id = argv[3];
> + a = dm_shift_arg(&as);
> + if (a)
> + program_id = a;
> +
> + a = dm_shift_arg(&as);
> + if (a)
> + aux_data = a;
>
> - if (argc > 4)
> - aux_data = argv[4];
> + if (as.argc)
> + return -EINVAL;
>
If I could, I would simplify the design to remove the above changes in
message_stats_create(). Just my suggestion ...
[snip]
Coly
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
