On 22.12.2017 21:07, Liu Bo wrote:
> On Fri, Dec 22, 2017 at 10:56:31AM +0200, Nikolay Borisov wrote:
>>
>>
>> On 22.12.2017 00:42, Liu Bo wrote:
>>> This is adding a tracepoint 'btrfs_handle_em_exist' to help debug the
>>> subtle bugs around merge_extent_mapping.
>>
>> In the next patch you are already making the function which takes all
>> these values noinline, meaning you can attach a kprobe so you can
>> interrogate the args via systemtap,perf probe or even bpf. So I'd rather
>> not add this tracepoint since the general sentiment seems to be that
>> tracepoints are ABI and so have to be maintained.
>>
>
> The following patch of noinline function is inside the else {} branch,
> so kprobe would give us something back only when it runs to else{}.
>
> Since subtle bugs may lie in this area, a simple tracepoint like this
> can help us understand them more efficiently.
In that case if you make search_extent_mapping and put a kprobe/ret
kprobe there you should be able to gain the same information, we should
really careful when adding trace events...
>
> thanks,
> -liubo
>
>>>
>>> Signed-off-by: Liu Bo <bo.li.liu@xxxxxxxxxx>
>>> ---
>>> fs/btrfs/extent_map.c | 1 +
>>> include/trace/events/btrfs.h | 35 +++++++++++++++++++++++++++++++++++
>>> 2 files changed, 36 insertions(+)
>>>
>>> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
>>> index a8b7e24..40e4d30 100644
>>> --- a/fs/btrfs/extent_map.c
>>> +++ b/fs/btrfs/extent_map.c
>>> @@ -539,6 +539,7 @@ int btrfs_add_extent_mapping(struct extent_map_tree *em_tree,
>>> ret = 0;
>>>
>>> existing = search_extent_mapping(em_tree, start, len);
>>> + trace_btrfs_handle_em_exist(existing, em, start, len);
>>>
>>> /*
>>> * existing will always be non-NULL, since there must be
>>> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
>>> index 4342a32..b7ffcf7 100644
>>> --- a/include/trace/events/btrfs.h
>>> +++ b/include/trace/events/btrfs.h
>>> @@ -249,6 +249,41 @@ TRACE_EVENT_CONDITION(btrfs_get_extent,
>>> __entry->refs, __entry->compress_type)
>>> );
>>>
>>> +TRACE_EVENT(btrfs_handle_em_exist,
>>> +
>>> + TP_PROTO(const struct extent_map *existing, const struct extent_map *map, u64 start, u64 len),
>>> +
>>> + TP_ARGS(existing, map, start, len),
>>> +
>>> + TP_STRUCT__entry(
>>> + __field( u64, e_start )
>>> + __field( u64, e_len )
>>> + __field( u64, map_start )
>>> + __field( u64, map_len )
>>> + __field( u64, start )
>>> + __field( u64, len )
>>> + ),
>>> +
>>> + TP_fast_assign(
>>> + __entry->e_start = existing->start;
>>> + __entry->e_len = existing->len;
>>> + __entry->map_start = map->start;
>>> + __entry->map_len = map->len;
>>> + __entry->start = start;
>>> + __entry->len = len;
>>> + ),
>>> +
>>> + TP_printk("start=%llu len=%llu "
>>> + "existing(start=%llu len=%llu) "
>>> + "em(start=%llu len=%llu)",
>>> + (unsigned long long)__entry->start,
>>> + (unsigned long long)__entry->len,
>>> + (unsigned long long)__entry->e_start,
>>> + (unsigned long long)__entry->e_len,
>>> + (unsigned long long)__entry->map_start,
>>> + (unsigned long long)__entry->map_len)
>>> +);
>>> +
>>> /* file extent item */
>>> DECLARE_EVENT_CLASS(btrfs__file_extent_item_regular,
>>>
>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
