Re: [PATCH 7/7] btrfs: use fs_info for btrfs_handle_em_exist tracepoint

2018-04-10 Thread David Sterba
On Tue, Apr 10, 2018 at 10:45:39AM +0300, Nikolay Borisov wrote:
> On  9.04.2018 14:58, David Sterba wrote:
> > We really want to know to which filesystem the extent map events belong,
> > but as it cannot be reached from the extent_map pointers, we need to
> > pass it down the callchain.
> 
> I really dislike propagating arguments solely for tracepoints purposes,
> but if we don't have any other choice then I guess we should go with it.
> However...
> 
> > @@ -7092,7 +7092,7 @@ struct extent_map *btrfs_get_extent(struct 
> > btrfs_inode *inode,
> >  
> > err = 0;
> > write_lock(_tree->lock);
> > -   err = btrfs_add_extent_mapping(em_tree, , start, len);
> > +   err = btrfs_add_extent_mapping(fs_info, em_tree, , start, len);
> 
> This function is called only here, and we know that em_tree passed
> points to a struct, stored in btrfs_inode. So can't we use container_of
> to get the btrfs_inode in this function, from where we can reference the
> fs_info? I guess it could be a problem for tests.
> 
> 
> Admittedly  this feels somewhat hacky and I guess is not very
> future-proof, but it's still a viable alternative.

Sounds too fragile to me, from all the alternatives passing fs_info
looks like the cleanest way for now.  The filesystem UUID in the
tracepoint is IMO an important part.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 7/7] btrfs: use fs_info for btrfs_handle_em_exist tracepoint

2018-04-10 Thread Nikolay Borisov


On  9.04.2018 14:58, David Sterba wrote:
> We really want to know to which filesystem the extent map events belong,
> but as it cannot be reached from the extent_map pointers, we need to
> pass it down the callchain.

I really dislike propagating arguments solely for tracepoints purposes,
but if we don't have any other choice then I guess we should go with it.
However...

> 
> Signed-off-by: David Sterba 
> ---
>  fs/btrfs/extent_map.c |  6 --
>  fs/btrfs/extent_map.h |  3 ++-
>  fs/btrfs/inode.c  |  2 +-
>  fs/btrfs/tests/extent-map-tests.c |  8 
>  include/trace/events/btrfs.h  | 12 +++-
>  5 files changed, 18 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/btrfs/extent_map.c b/fs/btrfs/extent_map.c
> index 53a0633c6ef7..581b42d23e0d 100644
> --- a/fs/btrfs/extent_map.c
> +++ b/fs/btrfs/extent_map.c
> @@ -517,6 +517,7 @@ static noinline int merge_extent_mapping(struct 
> extent_map_tree *em_tree,
>  
>  /**
>   * btrfs_add_extent_mapping - add extent mapping into em_tree
> + * @fs_info - used for tracepoint
>   * @em_tree - the extent tree into which we want to insert the extent mapping
>   * @em_in   - extent we are inserting
>   * @start   - start of the logical range btrfs_get_extent() is requesting
> @@ -534,7 +535,8 @@ static noinline int merge_extent_mapping(struct 
> extent_map_tree *em_tree,
>   * Return 0 on success, otherwise -EEXIST.
>   *
>   */
> -int btrfs_add_extent_mapping(struct extent_map_tree *em_tree,
> +int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
> +  struct extent_map_tree *em_tree,
>struct extent_map **em_in, u64 start, u64 len)
>  {
>   int ret;
> @@ -552,7 +554,7 @@ int btrfs_add_extent_mapping(struct extent_map_tree 
> *em_tree,
>  
>   existing = search_extent_mapping(em_tree, start, len);
>  
> - trace_btrfs_handle_em_exist(existing, em, start, len);
> + trace_btrfs_handle_em_exist(fs_info, existing, em, start, len);
>  
>   /*
>* existing will always be non-NULL, since there must be
> diff --git a/fs/btrfs/extent_map.h b/fs/btrfs/extent_map.h
> index f6f8ba114977..f55c8b4ef120 100644
> --- a/fs/btrfs/extent_map.h
> +++ b/fs/btrfs/extent_map.h
> @@ -91,6 +91,7 @@ int unpin_extent_cache(struct extent_map_tree *tree, u64 
> start, u64 len, u64 gen
>  void clear_em_logging(struct extent_map_tree *tree, struct extent_map *em);
>  struct extent_map *search_extent_mapping(struct extent_map_tree *tree,
>u64 start, u64 len);
> -int btrfs_add_extent_mapping(struct extent_map_tree *em_tree,
> +int btrfs_add_extent_mapping(struct btrfs_fs_info *fs_info,
> +  struct extent_map_tree *em_tree,
>struct extent_map **em_in, u64 start, u64 len);
>  #endif
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 1f091c2358a4..18c31006865f 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -7092,7 +7092,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode 
> *inode,
>  
>   err = 0;
>   write_lock(_tree->lock);
> - err = btrfs_add_extent_mapping(em_tree, , start, len);
> + err = btrfs_add_extent_mapping(fs_info, em_tree, , start, len);

This function is called only here, and we know that em_tree passed
points to a struct, stored in btrfs_inode. So can't we use container_of
to get the btrfs_inode in this function, from where we can reference the
fs_info? I guess it could be a problem for tests.


Admittedly  this feels somewhat hacky and I guess is not very
future-proof, but it's still a viable alternative.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html