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

[Nikolay Borisov <nborisov@suse.com>]

On 10.03.19 г. 11:56 ч., Qu Wenruo wrote:
> 
> 
> On 2019/3/10 下午5:40, Nikolay Borisov wrote:
>>
>>
>> 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?
> 
> Since this patchset is mostly an RFC, I skipped a lot things like mount
> option to skip all such events.
> 
> But the bfp part is really a good direction for me to investigate.
> If we can do the same work using more established facility, then I'm
> completely fine to discard the patchset.

The only thing which migth have to be patched in btrfs is to add the
tree owner being part of struct extent_buffer { since there is no easy
way to query it outside of btrfs. This can be gated behind an ifdef
BTRFS_DEBUG .

There is currently a very similar script in iovisor toolset:
https://github.com/iovisor/bcc/blob/master/tools/funclatency.py

it equates to what function_graph currently does but can easily be
extended to mimic the logic in your patchset. I.e modify a map based on
the owner of an extent buffer.

> 
> Thanks,
> Qu
> 
>> 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
>>>
>>>
>