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

[Qu Wenruo <wqu@suse.com>]

On 2020/4/1 下午11:37, David Sterba wrote:
> 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.

As most of them meet the requirement for either too small, or too
infrequently called.

> 
> 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.

But it's used in later patches, thus I exported them to avoid re-export
them later.

> 
>> +
>> +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.

Forgot this one...

Do I need to resend?

Thanks,
Qu

> 
>> +{
>> +	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