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

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

在 15/6/15 下午8:47, Mikulas Patocka 写道:
>
> On Sun, 14 Jun 2015, Coly Li wrote:
>
>> ? 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.
> It is searched using binary search - so the complexity is logarithmic.
It seems I didn't understand dm stats code very well, I need more time
to read and understand your patches :-)

I will test the patches in our storage product environment for the
performance number, hope it is good.
>
>> 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.
> There is no maximum - and there doesn't need to be any maximum because the 
> array is allocated dynamically.
Maybe I am too paranoid, IMHO dynamically allocated object should have a
maximum limitation, in case of it occupies too much system resource (e.g
memory here).

>
>> 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.
> I don't know why should I remove the code for arguments checking. We check 
> arguments in the kernel, even if the function may be only called by root - 
> to prevent bugs in userspace tools from causing kernel crash.
>
I didn't express myself well. I meant to minimize the number of
parameters, so most of the arguments checking code could be removed.
I didn't mean not checking the arguments, I meant that it could be
better if much less parameters sent from user space.

Coly


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