Re: [PATCH 7/8] btrfs: extent-tree: Use btrfs_ref to refactor btrfs_inc_extent_ref()

[Nikolay Borisov <nborisov@suse.com>]

On 6.12.18 г. 8:59 ч., Qu Wenruo wrote:
> Now we don't need to play the dirty game of reusing @owner for tree block
> level.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/ctree.h       |  6 ++---
>  fs/btrfs/extent-tree.c | 58 ++++++++++++++++++++++--------------------
>  fs/btrfs/file.c        | 20 ++++++++++-----
>  fs/btrfs/inode.c       | 10 +++++---
>  fs/btrfs/ioctl.c       | 17 ++++++++-----
>  fs/btrfs/relocation.c  | 44 ++++++++++++++++++++------------
>  fs/btrfs/tree-log.c    | 12 ++++++---
>  7 files changed, 100 insertions(+), 67 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 6f4b1e605736..db3df5ce6087 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -40,6 +40,7 @@ extern struct kmem_cache *btrfs_bit_radix_cachep;
>  extern struct kmem_cache *btrfs_path_cachep;
>  extern struct kmem_cache *btrfs_free_space_cachep;
>  struct btrfs_ordered_sum;
> +struct btrfs_ref;
>  
>  #define BTRFS_MAGIC 0x4D5F53665248425FULL /* ascii _BHRfS_M, no null */
>  
> @@ -2682,10 +2683,7 @@ int btrfs_free_and_pin_reserved_extent(struct btrfs_fs_info *fs_info,
>  void btrfs_prepare_extent_commit(struct btrfs_fs_info *fs_info);
>  int btrfs_finish_extent_commit(struct btrfs_trans_handle *trans);
>  int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
> -			 struct btrfs_root *root,
> -			 u64 bytenr, u64 num_bytes, u64 parent,
> -			 u64 root_objectid, u64 owner, u64 offset,
> -			 bool for_reloc);
> +			 struct btrfs_ref *generic_ref);
>  
>  int btrfs_start_dirty_block_groups(struct btrfs_trans_handle *trans);
>  int btrfs_write_dirty_block_groups(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 70c05ca30d9a..ff60091aef6b 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -2026,36 +2026,28 @@ int btrfs_discard_extent(struct btrfs_fs_info *fs_info, u64 bytenr,
>  
>  /* Can return -ENOMEM */
>  int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
> -			 struct btrfs_root *root,
> -			 u64 bytenr, u64 num_bytes, u64 parent,
> -			 u64 root_objectid, u64 owner, u64 offset,
> -			 bool for_reloc)
> +			 struct btrfs_ref *generic_ref)
>  {
> -	struct btrfs_fs_info *fs_info = root->fs_info;
> -	struct btrfs_ref generic_ref = { 0 };
> +	struct btrfs_fs_info *fs_info = trans->fs_info;
>  	int old_ref_mod, new_ref_mod;
>  	int ret;
>  
> -	BUG_ON(owner < BTRFS_FIRST_FREE_OBJECTID &&
> -	       root_objectid == BTRFS_TREE_LOG_OBJECTID);
> +	BUG_ON(generic_ref->type == BTRFS_REF_NOT_SET ||
> +	       !generic_ref->action);

Ouch, we should be removing BUG_ONs and not introducing new ones. Make
it an assert

> +	BUG_ON(generic_ref->type == BTRFS_REF_METADATA &&
> +	       generic_ref->tree_ref.root == BTRFS_TREE_LOG_OBJECTID);

Ideally this should also be converted to an assert. I guess it could
only happen if the filesystem is corrupted so perhaps is redundant now?

>  
> -	btrfs_init_generic_ref(&generic_ref, BTRFS_ADD_DELAYED_REF, bytenr,
> -			       num_bytes, root->root_key.objectid, parent);
> -	generic_ref.skip_qgroup = for_reloc;
> -	if (owner < BTRFS_FIRST_FREE_OBJECTID) {
> -		btrfs_init_tree_ref(&generic_ref, (int)owner, root_objectid);
> -		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref,
> +	if (generic_ref->type == BTRFS_REF_METADATA)
> +		ret = btrfs_add_delayed_tree_ref(trans, generic_ref,
>  				NULL, &old_ref_mod, &new_ref_mod);
> -	} else {
> -		btrfs_init_data_ref(&generic_ref, root_objectid, owner, offset);
> -		ret = btrfs_add_delayed_data_ref(trans, &generic_ref, 0,
> +	else
> +		ret = btrfs_add_delayed_data_ref(trans, generic_ref, 0,
>  						 &old_ref_mod, &new_ref_mod);
> -	}
>  
> -	btrfs_ref_tree_mod(fs_info, &generic_ref);
> +	btrfs_ref_tree_mod(fs_info, generic_ref);
>  
>  	if (ret == 0 && old_ref_mod < 0 && new_ref_mod >= 0)
> -		add_pinned_bytes(fs_info, &generic_ref);
> +		add_pinned_bytes(fs_info, generic_ref);
>  
>  	return ret;
>  }

<snip>