Re: [PATCH 01/13] btrfs: update btrfs_insert_inode_defrag to to use rb helper

[David Sterba <dsterba@suse.cz>]

Please post the series with a cover letter so comments that apply to the
whole series can be posted there.

The series looks good, tests are running OK so far, I have mostly coding
style comments.

- rephrase the subject line to

"btrfs: use rb_find_add() in btrfs_insert_inode_defrag(I)

here mentioning the rb helper also suggests what the patch does and is
obvious so it does not need a long description.

On Tue, Apr 22, 2025 at 02:14:52AM -0600, Yangtao Li wrote:
> Update btrfs_insert_inode_defrag() to use rb_find_add().

The following text can be used in most patches (adjusted accordingly)

"Use the rb-tree helper so we don't open code the search and insert
code."
> 
> Suggested-by: David Sterba <dsterba@suse.com>

Please drop this tag.

> Signed-off-by: Yangtao Li <frank.li@vivo.com>
> ---
>  fs/btrfs/defrag.c | 52 +++++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/btrfs/defrag.c b/fs/btrfs/defrag.c
> index d4310d93f532..d908bce0b8a1 100644
> --- a/fs/btrfs/defrag.c
> +++ b/fs/btrfs/defrag.c
> @@ -60,6 +60,16 @@ static int compare_inode_defrag(const struct inode_defrag *defrag1,
>  		return 0;
>  }
>  
> +static int inode_defrag_cmp(struct rb_node *new, const struct rb_node *exist)

This is a bit confusing name because there's also compare_inode_defrag()
but I don't have a better suggestion.

> +{
> +	const struct inode_defrag *new_defrag =
> +		rb_entry(new, struct inode_defrag, rb_node);
> +	const struct inode_defrag *exist_defrag =
> +		rb_entry(exist, struct inode_defrag, rb_node);
> +
> +	return compare_inode_defrag(new_defrag, exist_defrag);
> +}
> +
>  /*
>   * Insert a record for an inode into the defrag tree.  The lock must be held
>   * already.
> @@ -71,37 +81,25 @@ static int btrfs_insert_inode_defrag(struct btrfs_inode *inode,
>  				     struct inode_defrag *defrag)
>  {
>  	struct btrfs_fs_info *fs_info = inode->root->fs_info;
> -	struct inode_defrag *entry;
> -	struct rb_node **p;
> -	struct rb_node *parent = NULL;
> -	int ret;
> +	struct rb_node *exist;

Please use 'node' for the rb_nodes.

>  
> -	p = &fs_info->defrag_inodes.rb_node;
> -	while (*p) {
> -		parent = *p;
> -		entry = rb_entry(parent, struct inode_defrag, rb_node);
> +	exist = rb_find_add(&defrag->rb_node, &fs_info->defrag_inodes, inode_defrag_cmp);
> +	if (exist) {
> +		struct inode_defrag *entry;
>  
> -		ret = compare_inode_defrag(defrag, entry);
> -		if (ret < 0)
> -			p = &parent->rb_left;
> -		else if (ret > 0)
> -			p = &parent->rb_right;
> -		else {
> -			/*
> -			 * If we're reinserting an entry for an old defrag run,
> -			 * make sure to lower the transid of our existing
> -			 * record.
> -			 */
> -			if (defrag->transid < entry->transid)
> -				entry->transid = defrag->transid;
> -			entry->extent_thresh = min(defrag->extent_thresh,
> -						   entry->extent_thresh);
> -			return -EEXIST;
> -		}
> +		entry = rb_entry(exist, struct inode_defrag, rb_node);
> +		/*
> +		 * If we're reinserting an entry for an old defrag run,
> +		 * make sure to lower the transid of our existing
> +		 * record.

Please reformat the comment to 80 columns.

> +		 */
> +		if (defrag->transid < entry->transid)
> +			entry->transid = defrag->transid;
> +		entry->extent_thresh = min(defrag->extent_thresh,
> +					   entry->extent_thresh);
> +		return -EEXIST;
>  	}
>  	set_bit(BTRFS_INODE_IN_DEFRAG, &inode->runtime_flags);
> -	rb_link_node(&defrag->rb_node, parent, p);
> -	rb_insert_color(&defrag->rb_node, &fs_info->defrag_inodes);
>  	return 0;
>  }
>  
> -- 
> 2.39.0
>