Re: [PATCH v2 01/39] btrfs: backref: Introduce the skeleton of btrfs_backref_iter

[David Sterba <dsterba@suse.cz>]

On Thu, Mar 26, 2020 at 04:32:38PM +0800, Qu Wenruo wrote:
> --- a/fs/btrfs/backref.h
> +++ b/fs/btrfs/backref.h
> @@ -78,4 +78,43 @@ struct prelim_ref {
>  	u64 wanted_disk_byte;
>  };
>  
> +/*
> + * Helper structure to help iterate backrefs of one extent.
> + *
> + * Now it only supports iteration for tree block in commit root.
> + */
> +struct btrfs_backref_iter {
> +	u64 bytenr;
> +	struct btrfs_path *path;
> +	struct btrfs_fs_info *fs_info;
> +	struct btrfs_key cur_key;
> +	u32 item_ptr;
> +	u32 cur_ptr;
> +	u32 end_ptr;
> +};
> +
> +struct btrfs_backref_iter *btrfs_backref_iter_alloc(
> +		struct btrfs_fs_info *fs_info, gfp_t gfp_flag);
> +
> +static inline void btrfs_backref_iter_free(struct btrfs_backref_iter *iter)
> +{
> +	if (!iter)
> +		return;
> +	btrfs_free_path(iter->path);
> +	kfree(iter);
> +}

Why do you make so many functions static inline? It makes sense for some
of them but in the following patches there are functions that are either
too big (so when they're inlined it bloats the asm) or called
infrequently so the inlining does not bring much. Code in header files
should be kept to minimum.

There are also functions not used anywhere else than in backref.c so
they don't need to be exported for now. For example
btrfs_backref_iter_is_inline_ref.

> +
> +int btrfs_backref_iter_start(struct btrfs_backref_iter *iter, u64 bytenr);
> +
> +static inline void
> +btrfs_backref_iter_release(struct btrfs_backref_iter *iter)

Please keep the function type and name on the same line, arguments can
go to the next line.

> +{
> +	iter->bytenr = 0;
> +	iter->item_ptr = 0;
> +	iter->cur_ptr = 0;
> +	iter->end_ptr = 0;
> +	btrfs_release_path(iter->path);
> +	memset(&iter->cur_key, 0, sizeof(iter->cur_key));
> +}
> +
>  #endif
> -- 
> 2.26.0