| Message ID | 20171221224256.18196-10-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. thanks, -liubo > > > > Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > > --- > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@oracle.com> >>> --- >>> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 23, 2017 at 09:39:00AM +0200, Nikolay Borisov wrote: > > > 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... Correct me if I'm wrong, using 'perf probe' can only return @retval, it seems that we're not able to reference retval's number like retval->var or retval.val. As @exisiting is returned by search_extent_mapping(), I'm fine without this tracepoint if 'perf probe' can do the referencing for retval. Thanks, -liubo > > > >>> > >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com> > >>> --- > >>> 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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,
This is adding a tracepoint 'btrfs_handle_em_exist' to help debug the subtle bugs around merge_extent_mapping. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/extent_map.c | 1 + include/trace/events/btrfs.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+)