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

[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.

> 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.

> 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.

> 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

Mikulas