| Message ID | 20180105195117.5131-10-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 05, 2018 at 12:51:16PM -0700, Liu Bo wrote: > 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> Reviewed-by: Josef Bacik <jbacik@fb.com> Thanks, Josef -- 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, Jan 05, 2018 at 12:51:16PM -0700, Liu Bo wrote: > 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> Nikolay has some concernas about adding the tracepoint, so I'll leave this patch out of the series for now as we should decide how to proceed. Thacepoints are considered an ABI by some and not ABI by others. I think it's a good addition to the debugging aids that also may turn out to be useful for evaluating performance later. At minimum we could add some prefix/suffix to the debugging tracepoint name, so we can let developers add what they need right away. -- 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 19.01.2018 20:15, David Sterba wrote: > On Fri, Jan 05, 2018 at 12:51:16PM -0700, Liu Bo wrote: >> 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> > > Nikolay has some concernas about adding the tracepoint, so I'll leave > this patch out of the series for now as we should decide how to proceed. > > Thacepoints are considered an ABI by some and not ABI by others. I think > it's a good addition to the debugging aids that also may turn out to be > useful for evaluating performance later. > > At minimum we could add some prefix/suffix to the debugging tracepoint > name, so we can let developers add what they need right away. > My concern specifically has to do with the fact that if tracepoints are considere ABI then whatever decision we make now will be cast in stone and if we juggle the code around we will still have to retain the format of the tracepoint. If, OTOH we are able to remove and change the tracepoint as we see fit - then it's okay. Otherwise we risk polluting the code -- 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, Jan 19, 2018 at 08:22:36PM +0200, Nikolay Borisov wrote: > > > On 19.01.2018 20:15, David Sterba wrote: > > On Fri, Jan 05, 2018 at 12:51:16PM -0700, Liu Bo wrote: > >> 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> > > > > Nikolay has some concernas about adding the tracepoint, so I'll leave > > this patch out of the series for now as we should decide how to proceed. > > > > Thacepoints are considered an ABI by some and not ABI by others. I think > > it's a good addition to the debugging aids that also may turn out to be > > useful for evaluating performance later. > > > > At minimum we could add some prefix/suffix to the debugging tracepoint > > name, so we can let developers add what they need right away. > My concern specifically has to do with the fact that if tracepoints are > considere ABI then whatever decision we make now will be cast in stone Right, same as with the ioctls, we take a great care there and still do mistakes that get discovered months after. > and if we juggle the code around we will still have to retain the format > of the tracepoint. If, OTOH we are able to remove and change the > tracepoint as we see fit - then it's okay. Otherwise we risk polluting > the code Understood and agreed. Jeff raised a question if there are namespaces in the tracepoints. That's an interesting point and would be more systematic than inventing our own naming scheme. -- 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 b5d0add..a5a1d17 100644 --- a/fs/btrfs/extent_map.c +++ b/fs/btrfs/extent_map.c @@ -552,6 +552,9 @@ 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 * extent causing the -EEXIST. 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 | 3 +++ include/trace/events/btrfs.h | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+)