Re: [PATCH RFC 0/3] btrfs: Performance profiler support

[Nikolay Borisov <nborisov@suse.com>]

On 10.03.19 г. 11:34 ч., Qu Wenruo wrote:
> 
> 
> On 2019/3/10 下午5:29, Nikolay Borisov wrote:
>>
>>
>> On 10.03.19 г. 5:08 ч., Anand Jain wrote:
>>>
>>>  I agree we need btrfs specific performance measurements and its
>>>  my list too.
>>>
>>>  However my idea was to add it as a btrfs-progs subcommand such as
>>>
>>>    btrfs inspect perf ...
>>>
>>>  And implement by using the systemtap/perf/bpf/dtrace, as these
>>>  can tap the kernel functions from the useland using which we
>>>  can measure the time taken and no kernel changes will be required.
>>>  But yes we need to update the btrfs-progs if we rename the kernel
>>>  function, which I think is ok.
>>>
>>>  I was too early trying this with bpf before, probably there are
>>>  more tools now to do that same thing.
>>
>> This is way too developer oriented to be included in the generic btrfs
>> tools. Frankly bpf makes sense but only as a separate script being
>> developed and possibly shared on github or whatnot so that other
>> interested people can use it. However, integrating with btrfs-progs
>> definitely seems the wrong thing to do.
>>
>> On the same note - I'm highly against this patchset landing in the kernel.
> 
> Are you against the interface part or the btrfs specific perf part?
> 
> For the first half, indeed the new sysfs is not good and I'm OK to move
> to any more established interface.
> 
> For the later half, if bfp/ftrace can do the same work, then I'm fine.
> Otherwise the need is still here, both developer or experienced sys
> admin will need this feature.

I'm against this functionality being part of btrfs. What would be more
useful is a set of bpf scripts which might very well be developed
alongside the main project. They will be a lot more flexible also they
will add overhead only when you really need them, whereas the current
approach AFAIK (I might be wrong, correct me if so) always accumulates
certain stats irrespective of whether the sys admin needs them or not?
At the very least you will sprinkle code such as :

'if (perf measurement enabled) ..... '

I'd really we didn't add this clutter.

> 
> Thanks,
> Qu
> 
>