Re: [PATCH 0/4] Integrate dm-latency functionality to dm-statistics

From: Coly Li <colyli@gmail.com>
To: Mikulas Patocka <mpatocka@redhat.com>,
	"Alasdair G. Kergon" <agk@redhat.com>,
	Mike Snitzer <msnitzer@redhat.com>
Cc: Tao Ma <boyu.mt@taobao.com>,
	dm-devel@redhat.com, Laurence Oberman <loberman@redhat.com>,
	Robin Dong <sanbai@alibaba-inc.com>
Subject: Re: [PATCH 0/4] Integrate dm-latency functionality to dm-statistics
Date: Sun, 14 Jun 2015 01:02:12 +0800	[thread overview]
Message-ID: <557C6214.4010506@gmail.com> (raw)
In-Reply-To: <alpine.LRH.2.02.1506091705160.1545@file01.intranet.prod.int.rdu2.redhat.com>

在 15/6/10 上午5:20, Mikulas Patocka 写道:
> Hi
>
> Here I'm sending patches that add funcionality provided by dm-latency into 
> dm-statistics framework. Dm-latency was introduced in this post 
> https://www.redhat.com/archives/dm-devel/2015-February/msg00158.html , 
> however, there is already similar framework dm-statistics in the kernel. 
> In order to have cleaner and easier to maintain code, it is better to 
> integrate dm-latency functionality into dm-statistics, rather than having 
> two frameworks that do similar things.
>
> This patch series makes these changes:
> * it is possible to use precise time measurement with nanosecond 
>   granularity (previously, time was measured in jiffies)
> * it is possible to collect histogram of latencies. The histogram 
>   boundaries may be selected by the user and there is unlimited number of 
>   possible boundaries (in dm-latency, the histogram boundaries were fixed)
> * it is possible to use dm-statistics on multipath
>
> The documentation of these extensions is in 
> Documentation/device-mapper/statistics.txt
>
> I'd like to ask people who designed dm-latency to check this patchset, try 
> to use it and tell us if it provides sufficient functionality to them (the 
> indended functinality of dm-latency was to predict hard disk failures from 
> i/o request latency histogram).
>

Hi Mikulas,

I was in travel last week, sorry for late response. Here are some
comments from me, just for your information,

1, If I understand correctly, latency histogram boundaries are organized by list in your implementation, and the list is searched in linear order. We had a similar but a little bit simple implementation, with very high I/O request work load on multiple hard disks, we observed 1% CPU performance cost. This is why dm-latency uses a very simple static array of counters to record I/O latency. I suggest you should do some benchmarks to compare the performance cost.

2, There should be a maximum latency histogram boundaries limitation. Maybe you do it in the patch and I missed it, that's should be my fault.

3, in message_stats_create() I see more code  to handle arguments checking. hmm, I don't check this cold block very carefully, but I suggest that string checking in kernel space should be very careful, if you may simplify current designment and remove the code for arguments checking, it might be better.

4, there are some minor suggestions, I will add my comments in other emails.

For rest of the patch set, they are good to me. Thanks for your effort to make a good start :-)

Coly

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel