[RFC] btrfs: Remove 'objectid' member from struct btrfs_root
diff mbox series

Message ID 432b7e99-dd88-4724-083f-a4b7ab5efe31@jp.fujitsu.com
State New
Headers show
Series
  • [RFC] btrfs: Remove 'objectid' member from struct btrfs_root
Related show

Commit Message

Misono Tomohiro Aug. 6, 2018, 5:25 a.m. UTC
There are two members in struct btrfs_root which indicate root's
objectid: ->objectid and ->root_key.objectid.

They are both set to the same value in __setup_root():
  static void __setup_root(struct btrfs_root *root,
                           struct btrfs_fs_info *fs_info,
                           u64 objectid)
  {
    ...
    root->objectid = objectid;
    ...
    root->root_key.objectid = objecitd;
    ...
  }
and not changed to other value after initialization.

grep in btrfs directory shows both are used in many places:
  $ grep -rI "root->root_key.objectid" | wc -l
  133
  $ grep -rI "root->objectid" | wc -l
  55
 (4.17, inc. some noise)

It is confusing to have two similar variable names and it seems
that there is no rule about which should be used in a certain case.

Since ->root_key itself is needed for tree reloc tree, let's remove
'objecitd' member and unify code to use ->root_key.objectid in all places.

Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
---
Although being fundamentally independent, this is based on the
patch: https://patchwork.kernel.org/patch/10556485/
since it also touches root->objectid.

 fs/btrfs/backref.c           |  5 +++--
 fs/btrfs/btrfs_inode.h       |  8 ++++----
 fs/btrfs/ctree.c             |  2 +-
 fs/btrfs/ctree.h             |  1 -
 fs/btrfs/delayed-inode.c     |  5 +++--
 fs/btrfs/disk-io.c           |  5 ++---
 fs/btrfs/export.c            |  4 ++--
 fs/btrfs/inode.c             |  2 +-
 fs/btrfs/ioctl.c             |  2 +-
 fs/btrfs/qgroup.c            | 23 ++++++++++++-----------
 fs/btrfs/ref-verify.c        |  8 ++++----
 fs/btrfs/relocation.c        |  3 ++-
 fs/btrfs/send.c              | 16 ++++++++--------
 fs/btrfs/super.c             |  6 ++++--
 fs/btrfs/transaction.c       |  4 ++--
 include/trace/events/btrfs.h | 15 ++++++++-------
 16 files changed, 57 insertions(+), 52 deletions(-)

Comments

Qu Wenruo Aug. 6, 2018, 6:17 a.m. UTC | #1
On 2018年08月06日 13:25, Misono Tomohiro wrote:
> There are two members in struct btrfs_root which indicate root's
> objectid: ->objectid and ->root_key.objectid.
> 
> They are both set to the same value in __setup_root():
>   static void __setup_root(struct btrfs_root *root,
>                            struct btrfs_fs_info *fs_info,
>                            u64 objectid)
>   {
>     ...
>     root->objectid = objectid;
>     ...
>     root->root_key.objectid = objecitd;
>     ...
>   }
> and not changed to other value after initialization.
> 
> grep in btrfs directory shows both are used in many places:
>   $ grep -rI "root->root_key.objectid" | wc -l
>   133
>   $ grep -rI "root->objectid" | wc -l
>   55
>  (4.17, inc. some noise)
> 
> It is confusing to have two similar variable names and it seems
> that there is no rule about which should be used in a certain case.
> 
> Since ->root_key itself is needed for tree reloc tree, let's remove
> 'objecitd' member and unify code to use ->root_key.objectid in all places.

It's a pretty nice move, just a small nitpick about __setup_root()
inlined later.
(And a personal crazy idea no need to address)

> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>

Feel free to add my tag:
Reviewed-by: Qu Wenruo <wqu@suse.com>

> ---
> Although being fundamentally independent, this is based on the
> patch: https://patchwork.kernel.org/patch/10556485/
> since it also touches root->objectid.
> 
>  fs/btrfs/backref.c           |  5 +++--
>  fs/btrfs/btrfs_inode.h       |  8 ++++----
>  fs/btrfs/ctree.c             |  2 +-
>  fs/btrfs/ctree.h             |  1 -
>  fs/btrfs/delayed-inode.c     |  5 +++--
>  fs/btrfs/disk-io.c           |  5 ++---
>  fs/btrfs/export.c            |  4 ++--
>  fs/btrfs/inode.c             |  2 +-
>  fs/btrfs/ioctl.c             |  2 +-
>  fs/btrfs/qgroup.c            | 23 ++++++++++++-----------
>  fs/btrfs/ref-verify.c        |  8 ++++----
>  fs/btrfs/relocation.c        |  3 ++-
>  fs/btrfs/send.c              | 16 ++++++++--------
>  fs/btrfs/super.c             |  6 ++++--
>  fs/btrfs/transaction.c       |  4 ++--
>  include/trace/events/btrfs.h | 15 ++++++++-------
>  16 files changed, 57 insertions(+), 52 deletions(-)

> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 318be7864072..5be7c2bc45c0 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1202,7 +1202,6 @@ struct btrfs_root {
>  	int last_log_commit;
>  	pid_t log_start_pid;
>  
> -	u64 objectid;

Off topic crazy idea here.

I think it is a little crazy, but it should save a lot of objectid
related modification:

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 118346aceea9..e6d70f2309a3 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1166,7 +1166,10 @@ struct btrfs_root {

        unsigned long state;
        struct btrfs_root_item root_item;
-       struct btrfs_key root_key;
+       union {
+               struct btrfs_key root_key;
+               u64 objectid;
+       };
        struct btrfs_fs_info *fs_info;
        struct extent_io_tree dirty_log_pages;

@@ -1198,7 +1201,6 @@ struct btrfs_root {
        int last_log_commit;
        pid_t log_start_pid;

-       u64 objectid;
        u64 last_trans;

        u32 type;

I'm not sure if this is a really crazy idea or a dirty hack to reduce
some modification.
Anyway, I'm completely fine with current patch.

>  	u64 last_trans;
>  
>  	u32 type;
> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
> index f51b509f2d9b..07187e4ab600 100644
> --- a/fs/btrfs/delayed-inode.c
> +++ b/fs/btrfs/delayed-inode.c
> @@ -1462,7 +1462,7 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>  	if (unlikely(ret)) {
>  		btrfs_err(trans->fs_info,
>  			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
> -			  name_len, name, delayed_node->root->objectid,
> +			  name_len, name, delayed_node->root->root_key.objectid,
>  			  delayed_node->inode_id, ret);
>  		BUG();
>  	}
> @@ -1533,7 +1533,8 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
>  	if (unlikely(ret)) {
>  		btrfs_err(trans->fs_info,
>  			  "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
> -			  index, node->root->objectid, node->inode_id, ret);
> +			  index, node->root->root_key.objectid,
> +			  node->inode_id, ret);
>  		BUG();
>  	}
>  	mutex_unlock(&node->mutex);
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 468365dd3b59..3fe87f67869b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -125,8 +125,8 @@ struct async_submit_bio {
>   * Different roots are used for different purposes and may nest inside each
>   * other and they require separate keysets.  As lockdep keys should be
>   * static, assign keysets according to the purpose of the root as indicated
> - * by btrfs_root->objectid.  This ensures that all special purpose roots
> - * have separate keysets.
> + * by btrfs_root->root_key.objectid.  This ensures that all special purpose
> + * roots have separate keysets.
>   *
>   * Lock-nesting across peer nodes is always done with the immediate parent
>   * node locked thus preventing deadlock.  As lockdep doesn't know this, use
> @@ -1148,7 +1148,6 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,

Since objectid is not really used and root->root_key can be set later by
btrfs_create_tree() or alloc_log_tree().

What about either removing @objectid parameter or replace it with @root_key?

Thanks,
Qu

>  	root->state = 0;
>  	root->orphan_cleanup_state = 0;
>  
> -	root->objectid = objectid;
>  	root->last_trans = 0;
>  	root->highest_objectid = 0;
>  	root->nr_delalloc_inodes = 0;
> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
> index 1f3755b3a37a..ddf28ecf17f9 100644
> --- a/fs/btrfs/export.c
> +++ b/fs/btrfs/export.c
> @@ -33,7 +33,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>  	type = FILEID_BTRFS_WITHOUT_PARENT;
>  
>  	fid->objectid = btrfs_ino(BTRFS_I(inode));
> -	fid->root_objectid = BTRFS_I(inode)->root->objectid;
> +	fid->root_objectid = BTRFS_I(inode)->root->root_key.objectid;
>  	fid->gen = inode->i_generation;
>  
>  	if (parent) {
> @@ -41,7 +41,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>  
>  		fid->parent_objectid = BTRFS_I(parent)->location.objectid;
>  		fid->parent_gen = parent->i_generation;
> -		parent_root_id = BTRFS_I(parent)->root->objectid;
> +		parent_root_id = BTRFS_I(parent)->root->root_key.objectid;
>  
>  		if (parent_root_id != fid->root_objectid) {
>  			fid->parent_root_objectid = parent_root_id;
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 3f51ddc18f98..78111d972af8 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6613,7 +6613,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>  	int drop_inode = 0;
>  
>  	/* do not allow sys_link's with other subvols of the same device */
> -	if (root->objectid != BTRFS_I(inode)->root->objectid)
> +	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
>  		return -EXDEV;
>  
>  	if (inode->i_nlink >= BTRFS_LINK_MAX)
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 6eaadddaca9f..8ab30c62df36 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -4363,7 +4363,7 @@ static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
>  		ret = PTR_ERR(new_root);
>  		goto out;
>  	}
> -	if (!is_fstree(new_root->objectid)) {
> +	if (!is_fstree(new_root->root_key.objectid)) {
>  		ret = -ENOENT;
>  		goto out;
>  	}
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 2ba29f0609d9..16a9771b13fe 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3004,7 +3004,7 @@ int btrfs_qgroup_reserve_data(struct inode *inode,
>  	int ret;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &root->fs_info->flags) ||
> -	    !is_fstree(root->objectid) || len == 0)
> +	    !is_fstree(root->root_key.objectid) || len == 0)
>  		return 0;
>  
>  	/* @reserved parameter is mandatory for qgroup */
> @@ -3090,7 +3090,7 @@ static int qgroup_free_reserved_data(struct inode *inode,
>  			goto out;
>  		freed += changeset.bytes_changed;
>  	}
> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
> +	btrfs_qgroup_free_refroot(root->fs_info, root->root_key.objectid, freed,
>  				  BTRFS_QGROUP_RSV_DATA);
>  	ret = freed;
>  out:
> @@ -3122,7 +3122,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  					changeset.bytes_changed, trace_op);
>  	if (free)
>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
> -				BTRFS_I(inode)->root->objectid,
> +				BTRFS_I(inode)->root->root_key.objectid,
>  				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>  	ret = changeset.bytes_changed;
>  out:
> @@ -3215,7 +3215,7 @@ int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
>  	int ret;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
> -	    !is_fstree(root->objectid) || num_bytes == 0)
> +	    !is_fstree(root->root_key.objectid) || num_bytes == 0)
>  		return 0;
>  
>  	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
> @@ -3240,13 +3240,13 @@ void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
> -	    !is_fstree(root->objectid))
> +	    !is_fstree(root->root_key.objectid))
>  		return;
>  
>  	/* TODO: Update trace point to handle such free */
>  	trace_qgroup_meta_free_all_pertrans(root);
>  	/* Special value -1 means to free all reserved space */
> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, (u64)-1,
> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid, (u64)-1,
>  				  BTRFS_QGROUP_RSV_META_PERTRANS);
>  }
>  
> @@ -3256,7 +3256,7 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
> -	    !is_fstree(root->objectid))
> +	    !is_fstree(root->root_key.objectid))
>  		return;
>  
>  	/*
> @@ -3267,7 +3267,8 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>  	num_bytes = sub_root_meta_rsv(root, num_bytes, type);
>  	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
>  	trace_qgroup_meta_reserve(root, type, -(s64)num_bytes);
> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes, type);
> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid,
> +				  num_bytes, type);
>  }
>  
>  static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
> @@ -3321,13 +3322,13 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>  	struct btrfs_fs_info *fs_info = root->fs_info;
>  
>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
> -	    !is_fstree(root->objectid))
> +	    !is_fstree(root->root_key.objectid))
>  		return;
>  	/* Same as btrfs_qgroup_free_meta_prealloc() */
>  	num_bytes = sub_root_meta_rsv(root, num_bytes,
>  				      BTRFS_QGROUP_RSV_META_PREALLOC);
>  	trace_qgroup_meta_convert(root, num_bytes);
> -	qgroup_convert_meta(fs_info, root->objectid, num_bytes);
> +	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
>  }
>  
>  /*
> @@ -3354,7 +3355,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>  				inode->i_ino, unode->val, unode->aux);
>  		}
>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
> -				BTRFS_I(inode)->root->objectid,
> +				BTRFS_I(inode)->root->root_key.objectid,
>  				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>  
>  	}
> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
> index e5b9e596bb92..d69fbfb30aa9 100644
> --- a/fs/btrfs/ref-verify.c
> +++ b/fs/btrfs/ref-verify.c
> @@ -732,7 +732,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>  
>  	INIT_LIST_HEAD(&ra->list);
>  	ra->action = action;
> -	ra->root = root->objectid;
> +	ra->root = root->root_key.objectid;
>  
>  	/*
>  	 * This is an allocation, preallocate the block_entry in case we haven't
> @@ -787,8 +787,8 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>  			 * one we want to lookup below when we modify the
>  			 * re->num_refs.
>  			 */
> -			ref_root = root->objectid;
> -			re->root_objectid = root->objectid;
> +			ref_root = root->root_key.objectid;
> +			re->root_objectid = root->root_key.objectid;
>  			re->num_refs = 0;
>  		}
>  
> @@ -862,7 +862,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>  			 * didn't thik of some other corner case.
>  			 */
>  			btrfs_err(fs_info, "failed to find root %llu for %llu",
> -				  root->objectid, be->bytenr);
> +				  root->root_key.objectid, be->bytenr);
>  			dump_block_entry(fs_info, be);
>  			dump_ref_action(fs_info, ra);
>  			kfree(ra);
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 8783a1776540..d626362687d7 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -884,7 +884,8 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>  		    cur->bytenr) {
>  			btrfs_err(root->fs_info,
>  	"couldn't find block (%llu) (level %d) in tree (%llu) with key (%llu %u %llu)",
> -				  cur->bytenr, level - 1, root->objectid,
> +				  cur->bytenr, level - 1,
> +				  root->root_key.objectid,
>  				  node_key->objectid, node_key->type,
>  				  node_key->offset);
>  			err = -ENOENT;
> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
> index 551294a6c9e2..f003ec949726 100644
> --- a/fs/btrfs/send.c
> +++ b/fs/btrfs/send.c
> @@ -1186,9 +1186,9 @@ static int __clone_root_cmp_bsearch(const void *key, const void *elt)
>  	u64 root = (u64)(uintptr_t)key;
>  	struct clone_root *cr = (struct clone_root *)elt;
>  
> -	if (root < cr->root->objectid)
> +	if (root < cr->root->root_key.objectid)
>  		return -1;
> -	if (root > cr->root->objectid)
> +	if (root > cr->root->root_key.objectid)
>  		return 1;
>  	return 0;
>  }
> @@ -1198,9 +1198,9 @@ static int __clone_root_cmp_sort(const void *e1, const void *e2)
>  	struct clone_root *cr1 = (struct clone_root *)e1;
>  	struct clone_root *cr2 = (struct clone_root *)e2;
>  
> -	if (cr1->root->objectid < cr2->root->objectid)
> +	if (cr1->root->root_key.objectid < cr2->root->root_key.objectid)
>  		return -1;
> -	if (cr1->root->objectid > cr2->root->objectid)
> +	if (cr1->root->root_key.objectid > cr2->root->root_key.objectid)
>  		return 1;
>  	return 0;
>  }
> @@ -2346,7 +2346,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>  		return -ENOMEM;
>  	}
>  
> -	key.objectid = send_root->objectid;
> +	key.objectid = send_root->root_key.objectid;
>  	key.type = BTRFS_ROOT_BACKREF_KEY;
>  	key.offset = 0;
>  
> @@ -2362,7 +2362,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>  	leaf = path->nodes[0];
>  	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
>  	if (key.type != BTRFS_ROOT_BACKREF_KEY ||
> -	    key.objectid != send_root->objectid) {
> +	    key.objectid != send_root->root_key.objectid) {
>  		ret = -ENOENT;
>  		goto out;
>  	}
> @@ -4907,8 +4907,8 @@ static int send_clone(struct send_ctx *sctx,
>  
>  	btrfs_debug(sctx->send_root->fs_info,
>  		    "send_clone offset=%llu, len=%d, clone_root=%llu, clone_inode=%llu, clone_offset=%llu",
> -		    offset, len, clone_root->root->objectid, clone_root->ino,
> -		    clone_root->offset);
> +		    offset, len, clone_root->root->root_key.objectid,
> +		    clone_root->ino, clone_root->offset);
>  
>  	p = fs_path_alloc();
>  	if (!p)
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 67de3c0fc85b..f114e848f4c9 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -2177,8 +2177,10 @@ static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>  	buf->f_fsid.val[0] = be32_to_cpu(fsid[0]) ^ be32_to_cpu(fsid[2]);
>  	buf->f_fsid.val[1] = be32_to_cpu(fsid[1]) ^ be32_to_cpu(fsid[3]);
>  	/* Mask in the root object ID too, to disambiguate subvols */
> -	buf->f_fsid.val[0] ^= BTRFS_I(d_inode(dentry))->root->objectid >> 32;
> -	buf->f_fsid.val[1] ^= BTRFS_I(d_inode(dentry))->root->objectid;
> +	buf->f_fsid.val[0] ^=
> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid >> 32;
> +	buf->f_fsid.val[1] ^=
> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid;
>  
>  	return 0;
>  }
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 3b84f5015029..c429bdda6348 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -118,7 +118,7 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans)
>  		list_del_init(&root->dirty_list);
>  		free_extent_buffer(root->commit_root);
>  		root->commit_root = btrfs_root_node(root);
> -		if (is_fstree(root->objectid))
> +		if (is_fstree(root->root_key.objectid))
>  			btrfs_unpin_free_ino(root);
>  		clear_btree_io_tree(&root->dirty_log_pages);
>  	}
> @@ -2330,7 +2330,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
>  	list_del_init(&root->root_list);
>  	spin_unlock(&fs_info->trans_lock);
>  
> -	btrfs_debug(fs_info, "cleaner removing %llu", root->objectid);
> +	btrfs_debug(fs_info, "cleaner removing %llu", root->root_key.objectid);
>  
>  	btrfs_kill_all_delayed_nodes(root);
>  
> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
> index b401c4e36394..abe3ff774f58 100644
> --- a/include/trace/events/btrfs.h
> +++ b/include/trace/events/btrfs.h
> @@ -316,7 +316,7 @@ DECLARE_EVENT_CLASS(btrfs__file_extent_item_regular,
>  	),
>  
>  	TP_fast_assign_btrfs(bi->root->fs_info,
> -		__entry->root_obj	= bi->root->objectid;
> +		__entry->root_obj	= bi->root->root_key.objectid;
>  		__entry->ino		= btrfs_ino(bi);
>  		__entry->isize		= bi->vfs_inode.i_size;
>  		__entry->disk_isize	= bi->disk_i_size;
> @@ -367,7 +367,7 @@ DECLARE_EVENT_CLASS(
>  
>  	TP_fast_assign_btrfs(
>  		bi->root->fs_info,
> -		__entry->root_obj	= bi->root->objectid;
> +		__entry->root_obj	= bi->root->root_key.objectid;
>  		__entry->ino		= btrfs_ino(bi);
>  		__entry->isize		= bi->vfs_inode.i_size;
>  		__entry->disk_isize	= bi->disk_i_size;
> @@ -1477,7 +1477,8 @@ DECLARE_EVENT_CLASS(btrfs__qgroup_rsv_data,
>  	),
>  
>  	TP_fast_assign_btrfs(btrfs_sb(inode->i_sb),
> -		__entry->rootid		= BTRFS_I(inode)->root->objectid;
> +		__entry->rootid		=
> +			BTRFS_I(inode)->root->root_key.objectid;
>  		__entry->ino		= btrfs_ino(BTRFS_I(inode));
>  		__entry->start		= start;
>  		__entry->len		= len;
> @@ -1675,7 +1676,7 @@ TRACE_EVENT(qgroup_meta_reserve,
>  	),
>  
>  	TP_fast_assign_btrfs(root->fs_info,
> -		__entry->refroot	= root->objectid;
> +		__entry->refroot	= root->root_key.objectid;
>  		__entry->diff		= diff;
>  	),
>  
> @@ -1697,7 +1698,7 @@ TRACE_EVENT(qgroup_meta_convert,
>  	),
>  
>  	TP_fast_assign_btrfs(root->fs_info,
> -		__entry->refroot	= root->objectid;
> +		__entry->refroot	= root->root_key.objectid;
>  		__entry->diff		= diff;
>  	),
>  
> @@ -1721,7 +1722,7 @@ TRACE_EVENT(qgroup_meta_free_all_pertrans,
>  	),
>  
>  	TP_fast_assign_btrfs(root->fs_info,
> -		__entry->refroot	= root->objectid;
> +		__entry->refroot	= root->root_key.objectid;
>  		spin_lock(&root->qgroup_meta_rsv_lock);
>  		__entry->diff		= -(s64)root->qgroup_meta_rsv_pertrans;
>  		spin_unlock(&root->qgroup_meta_rsv_lock);
> @@ -1802,7 +1803,7 @@ TRACE_EVENT(btrfs_inode_mod_outstanding_extents,
>  	),
>  
>  	TP_fast_assign_btrfs(root->fs_info,
> -		__entry->root_objectid	= root->objectid;
> +		__entry->root_objectid	= root->root_key.objectid;
>  		__entry->ino		= ino;
>  		__entry->mod		= mod;
>  	),
>
Su Yue Aug. 6, 2018, 6:37 a.m. UTC | #2
On 08/06/2018 02:17 PM, Qu Wenruo wrote:
> 
> 
> On 2018年08月06日 13:25, Misono Tomohiro wrote:
>> There are two members in struct btrfs_root which indicate root's
>> objectid: ->objectid and ->root_key.objectid.
>>
>> They are both set to the same value in __setup_root():
>>    static void __setup_root(struct btrfs_root *root,
>>                             struct btrfs_fs_info *fs_info,
>>                             u64 objectid)
>>    {
>>      ...
>>      root->objectid = objectid;
>>      ...
>>      root->root_key.objectid = objecitd;
>>      ...
>>    }
>> and not changed to other value after initialization.
>>
>> grep in btrfs directory shows both are used in many places:
>>    $ grep -rI "root->root_key.objectid" | wc -l
>>    133
>>    $ grep -rI "root->objectid" | wc -l
>>    55
>>   (4.17, inc. some noise)
>>
>> It is confusing to have two similar variable names and it seems
>> that there is no rule about which should be used in a certain case.
>>
>> Since ->root_key itself is needed for tree reloc tree, let's remove
>> 'objecitd' member and unify code to use ->root_key.objectid in all places.
> 
> It's a pretty nice move, just a small nitpick about __setup_root()
> inlined later.
> (And a personal crazy idea no need to address)
> 
>>
>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> 
> Feel free to add my tag:
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
>> ---
>> Although being fundamentally independent, this is based on the
>> patch: https://patchwork.kernel.org/patch/10556485/
>> since it also touches root->objectid.
>>
>>   fs/btrfs/backref.c           |  5 +++--
>>   fs/btrfs/btrfs_inode.h       |  8 ++++----
>>   fs/btrfs/ctree.c             |  2 +-
>>   fs/btrfs/ctree.h             |  1 -
>>   fs/btrfs/delayed-inode.c     |  5 +++--
>>   fs/btrfs/disk-io.c           |  5 ++---
>>   fs/btrfs/export.c            |  4 ++--
>>   fs/btrfs/inode.c             |  2 +-
>>   fs/btrfs/ioctl.c             |  2 +-
>>   fs/btrfs/qgroup.c            | 23 ++++++++++++-----------
>>   fs/btrfs/ref-verify.c        |  8 ++++----
>>   fs/btrfs/relocation.c        |  3 ++-
>>   fs/btrfs/send.c              | 16 ++++++++--------
>>   fs/btrfs/super.c             |  6 ++++--
>>   fs/btrfs/transaction.c       |  4 ++--
>>   include/trace/events/btrfs.h | 15 ++++++++-------
>>   16 files changed, 57 insertions(+), 52 deletions(-)
> 
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 318be7864072..5be7c2bc45c0 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1202,7 +1202,6 @@ struct btrfs_root {
>>   	int last_log_commit;
>>   	pid_t log_start_pid;
>>   
>> -	u64 objectid;
> 
> Off topic crazy idea here.
> 
> I think it is a little crazy, but it should save a lot of objectid
> related modification:
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 118346aceea9..e6d70f2309a3 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1166,7 +1166,10 @@ struct btrfs_root {
> 
>          unsigned long state;
>          struct btrfs_root_item root_item;
> -       struct btrfs_key root_key;
> +       union {
> +               struct btrfs_key root_key;
> +               u64 objectid;
> +       };
>          struct btrfs_fs_info *fs_info;
>          struct extent_io_tree dirty_log_pages;
> 
> @@ -1198,7 +1201,6 @@ struct btrfs_root {
>          int last_log_commit;
>          pid_t log_start_pid;
> 
> -       u64 objectid;
>          u64 last_trans;
> 
>          u32 type;
> 
> I'm not sure if this is a really crazy idea or a dirty hack to reduce
> some modification.
Wow, a tricky thought. Of course, Misono's patch is indeed good.

If the union trick is doable(I'm not sure about it), some lazy guys
like me will save little time of tpying 9 characters "root_key."

Thanks,
Su

> Anyway, I'm completely fine with current patch.
> 
>>   	u64 last_trans;
>>   
>>   	u32 type;
>> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
>> index f51b509f2d9b..07187e4ab600 100644
>> --- a/fs/btrfs/delayed-inode.c
>> +++ b/fs/btrfs/delayed-inode.c
>> @@ -1462,7 +1462,7 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>>   	if (unlikely(ret)) {
>>   		btrfs_err(trans->fs_info,
>>   			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
>> -			  name_len, name, delayed_node->root->objectid,
>> +			  name_len, name, delayed_node->root->root_key.objectid,
>>   			  delayed_node->inode_id, ret);
>>   		BUG();
>>   	}
>> @@ -1533,7 +1533,8 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
>>   	if (unlikely(ret)) {
>>   		btrfs_err(trans->fs_info,
>>   			  "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
>> -			  index, node->root->objectid, node->inode_id, ret);
>> +			  index, node->root->root_key.objectid,
>> +			  node->inode_id, ret);
>>   		BUG();
>>   	}
>>   	mutex_unlock(&node->mutex);
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 468365dd3b59..3fe87f67869b 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -125,8 +125,8 @@ struct async_submit_bio {
>>    * Different roots are used for different purposes and may nest inside each
>>    * other and they require separate keysets.  As lockdep keys should be
>>    * static, assign keysets according to the purpose of the root as indicated
>> - * by btrfs_root->objectid.  This ensures that all special purpose roots
>> - * have separate keysets.
>> + * by btrfs_root->root_key.objectid.  This ensures that all special purpose
>> + * roots have separate keysets.
>>    *
>>    * Lock-nesting across peer nodes is always done with the immediate parent
>>    * node locked thus preventing deadlock.  As lockdep doesn't know this, use
>> @@ -1148,7 +1148,6 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
> 
> Since objectid is not really used and root->root_key can be set later by
> btrfs_create_tree() or alloc_log_tree().
> 
> What about either removing @objectid parameter or replace it with @root_key?
> 
> Thanks,
> Qu
> 
>>   	root->state = 0;
>>   	root->orphan_cleanup_state = 0;
>>   
>> -	root->objectid = objectid;
>>   	root->last_trans = 0;
>>   	root->highest_objectid = 0;
>>   	root->nr_delalloc_inodes = 0;
>> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
>> index 1f3755b3a37a..ddf28ecf17f9 100644
>> --- a/fs/btrfs/export.c
>> +++ b/fs/btrfs/export.c
>> @@ -33,7 +33,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>   	type = FILEID_BTRFS_WITHOUT_PARENT;
>>   
>>   	fid->objectid = btrfs_ino(BTRFS_I(inode));
>> -	fid->root_objectid = BTRFS_I(inode)->root->objectid;
>> +	fid->root_objectid = BTRFS_I(inode)->root->root_key.objectid;
>>   	fid->gen = inode->i_generation;
>>   
>>   	if (parent) {
>> @@ -41,7 +41,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>   
>>   		fid->parent_objectid = BTRFS_I(parent)->location.objectid;
>>   		fid->parent_gen = parent->i_generation;
>> -		parent_root_id = BTRFS_I(parent)->root->objectid;
>> +		parent_root_id = BTRFS_I(parent)->root->root_key.objectid;
>>   
>>   		if (parent_root_id != fid->root_objectid) {
>>   			fid->parent_root_objectid = parent_root_id;
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 3f51ddc18f98..78111d972af8 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6613,7 +6613,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>>   	int drop_inode = 0;
>>   
>>   	/* do not allow sys_link's with other subvols of the same device */
>> -	if (root->objectid != BTRFS_I(inode)->root->objectid)
>> +	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
>>   		return -EXDEV;
>>   
>>   	if (inode->i_nlink >= BTRFS_LINK_MAX)
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 6eaadddaca9f..8ab30c62df36 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4363,7 +4363,7 @@ static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
>>   		ret = PTR_ERR(new_root);
>>   		goto out;
>>   	}
>> -	if (!is_fstree(new_root->objectid)) {
>> +	if (!is_fstree(new_root->root_key.objectid)) {
>>   		ret = -ENOENT;
>>   		goto out;
>>   	}
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 2ba29f0609d9..16a9771b13fe 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -3004,7 +3004,7 @@ int btrfs_qgroup_reserve_data(struct inode *inode,
>>   	int ret;
>>   
>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &root->fs_info->flags) ||
>> -	    !is_fstree(root->objectid) || len == 0)
>> +	    !is_fstree(root->root_key.objectid) || len == 0)
>>   		return 0;
>>   
>>   	/* @reserved parameter is mandatory for qgroup */
>> @@ -3090,7 +3090,7 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>   			goto out;
>>   		freed += changeset.bytes_changed;
>>   	}
>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>> +	btrfs_qgroup_free_refroot(root->fs_info, root->root_key.objectid, freed,
>>   				  BTRFS_QGROUP_RSV_DATA);
>>   	ret = freed;
>>   out:
>> @@ -3122,7 +3122,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>   					changeset.bytes_changed, trace_op);
>>   	if (free)
>>   		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>> -				BTRFS_I(inode)->root->objectid,
>> +				BTRFS_I(inode)->root->root_key.objectid,
>>   				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>   	ret = changeset.bytes_changed;
>>   out:
>> @@ -3215,7 +3215,7 @@ int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
>>   	int ret;
>>   
>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid) || num_bytes == 0)
>> +	    !is_fstree(root->root_key.objectid) || num_bytes == 0)
>>   		return 0;
>>   
>>   	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
>> @@ -3240,13 +3240,13 @@ void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>>   
>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>   		return;
>>   
>>   	/* TODO: Update trace point to handle such free */
>>   	trace_qgroup_meta_free_all_pertrans(root);
>>   	/* Special value -1 means to free all reserved space */
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, (u64)-1,
>> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid, (u64)-1,
>>   				  BTRFS_QGROUP_RSV_META_PERTRANS);
>>   }
>>   
>> @@ -3256,7 +3256,7 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>>   
>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>   		return;
>>   
>>   	/*
>> @@ -3267,7 +3267,8 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>>   	num_bytes = sub_root_meta_rsv(root, num_bytes, type);
>>   	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
>>   	trace_qgroup_meta_reserve(root, type, -(s64)num_bytes);
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes, type);
>> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid,
>> +				  num_bytes, type);
>>   }
>>   
>>   static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
>> @@ -3321,13 +3322,13 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>>   	struct btrfs_fs_info *fs_info = root->fs_info;
>>   
>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>   		return;
>>   	/* Same as btrfs_qgroup_free_meta_prealloc() */
>>   	num_bytes = sub_root_meta_rsv(root, num_bytes,
>>   				      BTRFS_QGROUP_RSV_META_PREALLOC);
>>   	trace_qgroup_meta_convert(root, num_bytes);
>> -	qgroup_convert_meta(fs_info, root->objectid, num_bytes);
>> +	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
>>   }
>>   
>>   /*
>> @@ -3354,7 +3355,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>   				inode->i_ino, unode->val, unode->aux);
>>   		}
>>   		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>> -				BTRFS_I(inode)->root->objectid,
>> +				BTRFS_I(inode)->root->root_key.objectid,
>>   				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>   
>>   	}
>> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
>> index e5b9e596bb92..d69fbfb30aa9 100644
>> --- a/fs/btrfs/ref-verify.c
>> +++ b/fs/btrfs/ref-verify.c
>> @@ -732,7 +732,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>   
>>   	INIT_LIST_HEAD(&ra->list);
>>   	ra->action = action;
>> -	ra->root = root->objectid;
>> +	ra->root = root->root_key.objectid;
>>   
>>   	/*
>>   	 * This is an allocation, preallocate the block_entry in case we haven't
>> @@ -787,8 +787,8 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>   			 * one we want to lookup below when we modify the
>>   			 * re->num_refs.
>>   			 */
>> -			ref_root = root->objectid;
>> -			re->root_objectid = root->objectid;
>> +			ref_root = root->root_key.objectid;
>> +			re->root_objectid = root->root_key.objectid;
>>   			re->num_refs = 0;
>>   		}
>>   
>> @@ -862,7 +862,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>   			 * didn't thik of some other corner case.
>>   			 */
>>   			btrfs_err(fs_info, "failed to find root %llu for %llu",
>> -				  root->objectid, be->bytenr);
>> +				  root->root_key.objectid, be->bytenr);
>>   			dump_block_entry(fs_info, be);
>>   			dump_ref_action(fs_info, ra);
>>   			kfree(ra);
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 8783a1776540..d626362687d7 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -884,7 +884,8 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>>   		    cur->bytenr) {
>>   			btrfs_err(root->fs_info,
>>   	"couldn't find block (%llu) (level %d) in tree (%llu) with key (%llu %u %llu)",
>> -				  cur->bytenr, level - 1, root->objectid,
>> +				  cur->bytenr, level - 1,
>> +				  root->root_key.objectid,
>>   				  node_key->objectid, node_key->type,
>>   				  node_key->offset);
>>   			err = -ENOENT;
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index 551294a6c9e2..f003ec949726 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -1186,9 +1186,9 @@ static int __clone_root_cmp_bsearch(const void *key, const void *elt)
>>   	u64 root = (u64)(uintptr_t)key;
>>   	struct clone_root *cr = (struct clone_root *)elt;
>>   
>> -	if (root < cr->root->objectid)
>> +	if (root < cr->root->root_key.objectid)
>>   		return -1;
>> -	if (root > cr->root->objectid)
>> +	if (root > cr->root->root_key.objectid)
>>   		return 1;
>>   	return 0;
>>   }
>> @@ -1198,9 +1198,9 @@ static int __clone_root_cmp_sort(const void *e1, const void *e2)
>>   	struct clone_root *cr1 = (struct clone_root *)e1;
>>   	struct clone_root *cr2 = (struct clone_root *)e2;
>>   
>> -	if (cr1->root->objectid < cr2->root->objectid)
>> +	if (cr1->root->root_key.objectid < cr2->root->root_key.objectid)
>>   		return -1;
>> -	if (cr1->root->objectid > cr2->root->objectid)
>> +	if (cr1->root->root_key.objectid > cr2->root->root_key.objectid)
>>   		return 1;
>>   	return 0;
>>   }
>> @@ -2346,7 +2346,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>>   		return -ENOMEM;
>>   	}
>>   
>> -	key.objectid = send_root->objectid;
>> +	key.objectid = send_root->root_key.objectid;
>>   	key.type = BTRFS_ROOT_BACKREF_KEY;
>>   	key.offset = 0;
>>   
>> @@ -2362,7 +2362,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>>   	leaf = path->nodes[0];
>>   	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
>>   	if (key.type != BTRFS_ROOT_BACKREF_KEY ||
>> -	    key.objectid != send_root->objectid) {
>> +	    key.objectid != send_root->root_key.objectid) {
>>   		ret = -ENOENT;
>>   		goto out;
>>   	}
>> @@ -4907,8 +4907,8 @@ static int send_clone(struct send_ctx *sctx,
>>   
>>   	btrfs_debug(sctx->send_root->fs_info,
>>   		    "send_clone offset=%llu, len=%d, clone_root=%llu, clone_inode=%llu, clone_offset=%llu",
>> -		    offset, len, clone_root->root->objectid, clone_root->ino,
>> -		    clone_root->offset);
>> +		    offset, len, clone_root->root->root_key.objectid,
>> +		    clone_root->ino, clone_root->offset);
>>   
>>   	p = fs_path_alloc();
>>   	if (!p)
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 67de3c0fc85b..f114e848f4c9 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2177,8 +2177,10 @@ static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>   	buf->f_fsid.val[0] = be32_to_cpu(fsid[0]) ^ be32_to_cpu(fsid[2]);
>>   	buf->f_fsid.val[1] = be32_to_cpu(fsid[1]) ^ be32_to_cpu(fsid[3]);
>>   	/* Mask in the root object ID too, to disambiguate subvols */
>> -	buf->f_fsid.val[0] ^= BTRFS_I(d_inode(dentry))->root->objectid >> 32;
>> -	buf->f_fsid.val[1] ^= BTRFS_I(d_inode(dentry))->root->objectid;
>> +	buf->f_fsid.val[0] ^=
>> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid >> 32;
>> +	buf->f_fsid.val[1] ^=
>> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid;
>>   
>>   	return 0;
>>   }
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 3b84f5015029..c429bdda6348 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -118,7 +118,7 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans)
>>   		list_del_init(&root->dirty_list);
>>   		free_extent_buffer(root->commit_root);
>>   		root->commit_root = btrfs_root_node(root);
>> -		if (is_fstree(root->objectid))
>> +		if (is_fstree(root->root_key.objectid))
>>   			btrfs_unpin_free_ino(root);
>>   		clear_btree_io_tree(&root->dirty_log_pages);
>>   	}
>> @@ -2330,7 +2330,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
>>   	list_del_init(&root->root_list);
>>   	spin_unlock(&fs_info->trans_lock);
>>   
>> -	btrfs_debug(fs_info, "cleaner removing %llu", root->objectid);
>> +	btrfs_debug(fs_info, "cleaner removing %llu", root->root_key.objectid);
>>   
>>   	btrfs_kill_all_delayed_nodes(root);
>>   
>> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
>> index b401c4e36394..abe3ff774f58 100644
>> --- a/include/trace/events/btrfs.h
>> +++ b/include/trace/events/btrfs.h
>> @@ -316,7 +316,7 @@ DECLARE_EVENT_CLASS(btrfs__file_extent_item_regular,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(bi->root->fs_info,
>> -		__entry->root_obj	= bi->root->objectid;
>> +		__entry->root_obj	= bi->root->root_key.objectid;
>>   		__entry->ino		= btrfs_ino(bi);
>>   		__entry->isize		= bi->vfs_inode.i_size;
>>   		__entry->disk_isize	= bi->disk_i_size;
>> @@ -367,7 +367,7 @@ DECLARE_EVENT_CLASS(
>>   
>>   	TP_fast_assign_btrfs(
>>   		bi->root->fs_info,
>> -		__entry->root_obj	= bi->root->objectid;
>> +		__entry->root_obj	= bi->root->root_key.objectid;
>>   		__entry->ino		= btrfs_ino(bi);
>>   		__entry->isize		= bi->vfs_inode.i_size;
>>   		__entry->disk_isize	= bi->disk_i_size;
>> @@ -1477,7 +1477,8 @@ DECLARE_EVENT_CLASS(btrfs__qgroup_rsv_data,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(btrfs_sb(inode->i_sb),
>> -		__entry->rootid		= BTRFS_I(inode)->root->objectid;
>> +		__entry->rootid		=
>> +			BTRFS_I(inode)->root->root_key.objectid;
>>   		__entry->ino		= btrfs_ino(BTRFS_I(inode));
>>   		__entry->start		= start;
>>   		__entry->len		= len;
>> @@ -1675,7 +1676,7 @@ TRACE_EVENT(qgroup_meta_reserve,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>   		__entry->diff		= diff;
>>   	),
>>   
>> @@ -1697,7 +1698,7 @@ TRACE_EVENT(qgroup_meta_convert,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>   		__entry->diff		= diff;
>>   	),
>>   
>> @@ -1721,7 +1722,7 @@ TRACE_EVENT(qgroup_meta_free_all_pertrans,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>   		spin_lock(&root->qgroup_meta_rsv_lock);
>>   		__entry->diff		= -(s64)root->qgroup_meta_rsv_pertrans;
>>   		spin_unlock(&root->qgroup_meta_rsv_lock);
>> @@ -1802,7 +1803,7 @@ TRACE_EVENT(btrfs_inode_mod_outstanding_extents,
>>   	),
>>   
>>   	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->root_objectid	= root->objectid;
>> +		__entry->root_objectid	= root->root_key.objectid;
>>   		__entry->ino		= ino;
>>   		__entry->mod		= mod;
>>   	),
>>
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Misono Tomohiro Aug. 6, 2018, 6:41 a.m. UTC | #3
On 2018/08/06 15:17, Qu Wenruo wrote:
> 
> 
> On 2018年08月06日 13:25, Misono Tomohiro wrote:
>> There are two members in struct btrfs_root which indicate root's
>> objectid: ->objectid and ->root_key.objectid.
>>
>> They are both set to the same value in __setup_root():
>>   static void __setup_root(struct btrfs_root *root,
>>                            struct btrfs_fs_info *fs_info,
>>                            u64 objectid)
>>   {
>>     ...
>>     root->objectid = objectid;
>>     ...
>>     root->root_key.objectid = objecitd;
>>     ...
>>   }
>> and not changed to other value after initialization.
>>
>> grep in btrfs directory shows both are used in many places:
>>   $ grep -rI "root->root_key.objectid" | wc -l
>>   133
>>   $ grep -rI "root->objectid" | wc -l
>>   55
>>  (4.17, inc. some noise)
>>
>> It is confusing to have two similar variable names and it seems
>> that there is no rule about which should be used in a certain case.
>>
>> Since ->root_key itself is needed for tree reloc tree, let's remove
>> 'objecitd' member and unify code to use ->root_key.objectid in all places.
> 
> It's a pretty nice move, just a small nitpick about __setup_root()
> inlined later.
> (And a personal crazy idea no need to address)
> 
>>
>> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>
> 
> Feel free to add my tag:
> Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks for the review.

> 
>> ---
>> Although being fundamentally independent, this is based on the
>> patch: https://patchwork.kernel.org/patch/10556485/
>> since it also touches root->objectid.
>>
>>  fs/btrfs/backref.c           |  5 +++--
>>  fs/btrfs/btrfs_inode.h       |  8 ++++----
>>  fs/btrfs/ctree.c             |  2 +-
>>  fs/btrfs/ctree.h             |  1 -
>>  fs/btrfs/delayed-inode.c     |  5 +++--
>>  fs/btrfs/disk-io.c           |  5 ++---
>>  fs/btrfs/export.c            |  4 ++--
>>  fs/btrfs/inode.c             |  2 +-
>>  fs/btrfs/ioctl.c             |  2 +-
>>  fs/btrfs/qgroup.c            | 23 ++++++++++++-----------
>>  fs/btrfs/ref-verify.c        |  8 ++++----
>>  fs/btrfs/relocation.c        |  3 ++-
>>  fs/btrfs/send.c              | 16 ++++++++--------
>>  fs/btrfs/super.c             |  6 ++++--
>>  fs/btrfs/transaction.c       |  4 ++--
>>  include/trace/events/btrfs.h | 15 ++++++++-------
>>  16 files changed, 57 insertions(+), 52 deletions(-)
> 
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 318be7864072..5be7c2bc45c0 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1202,7 +1202,6 @@ struct btrfs_root {
>>  	int last_log_commit;
>>  	pid_t log_start_pid;
>>  
>> -	u64 objectid;
> 
> Off topic crazy idea here.
> 
> I think it is a little crazy, but it should save a lot of objectid
> related modification:
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 118346aceea9..e6d70f2309a3 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1166,7 +1166,10 @@ struct btrfs_root {
> 
>         unsigned long state;
>         struct btrfs_root_item root_item;
> -       struct btrfs_key root_key;
> +       union {
> +               struct btrfs_key root_key;
> +               u64 objectid;
> +       };
>         struct btrfs_fs_info *fs_info;
>         struct extent_io_tree dirty_log_pages;
> 
> @@ -1198,7 +1201,6 @@ struct btrfs_root {
>         int last_log_commit;
>         pid_t log_start_pid;
> 
> -       u64 objectid;
>         u64 last_trans;
> 
>         u32 type;
> 
> I'm not sure if this is a really crazy idea or a dirty hack to reduce
> some modification.
> Anyway, I'm completely fine with current patch.
> 
>>  	u64 last_trans;
>>  
>>  	u32 type;
>> diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
>> index f51b509f2d9b..07187e4ab600 100644
>> --- a/fs/btrfs/delayed-inode.c
>> +++ b/fs/btrfs/delayed-inode.c
>> @@ -1462,7 +1462,7 @@ int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
>>  	if (unlikely(ret)) {
>>  		btrfs_err(trans->fs_info,
>>  			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
>> -			  name_len, name, delayed_node->root->objectid,
>> +			  name_len, name, delayed_node->root->root_key.objectid,
>>  			  delayed_node->inode_id, ret);
>>  		BUG();
>>  	}
>> @@ -1533,7 +1533,8 @@ int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
>>  	if (unlikely(ret)) {
>>  		btrfs_err(trans->fs_info,
>>  			  "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
>> -			  index, node->root->objectid, node->inode_id, ret);
>> +			  index, node->root->root_key.objectid,
>> +			  node->inode_id, ret);
>>  		BUG();
>>  	}
>>  	mutex_unlock(&node->mutex);
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 468365dd3b59..3fe87f67869b 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -125,8 +125,8 @@ struct async_submit_bio {
>>   * Different roots are used for different purposes and may nest inside each
>>   * other and they require separate keysets.  As lockdep keys should be
>>   * static, assign keysets according to the purpose of the root as indicated
>> - * by btrfs_root->objectid.  This ensures that all special purpose roots
>> - * have separate keysets.
>> + * by btrfs_root->root_key.objectid.  This ensures that all special purpose
>> + * roots have separate keysets.
>>   *
>>   * Lock-nesting across peer nodes is always done with the immediate parent
>>   * node locked thus preventing deadlock.  As lockdep doesn't know this, use
>> @@ -1148,7 +1148,6 @@ static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
> 
> Since objectid is not really used and root->root_key can be set later by
> btrfs_create_tree() or alloc_log_tree().
> 
> What about either removing @objectid parameter or replace it with @root_key?

It seems that setting all @root_key member in __setup_root() is more natural for
btrfs_create_tree()/alloc_log_tree(), so I prefer to replace @objectid with @root_key.
will do in next version.

Thanks,
Misono

> 
> Thanks,
> Qu
> 
>>  	root->state = 0;
>>  	root->orphan_cleanup_state = 0;
>>  
>> -	root->objectid = objectid;
>>  	root->last_trans = 0;
>>  	root->highest_objectid = 0;
>>  	root->nr_delalloc_inodes = 0;
>> diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
>> index 1f3755b3a37a..ddf28ecf17f9 100644
>> --- a/fs/btrfs/export.c
>> +++ b/fs/btrfs/export.c
>> @@ -33,7 +33,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>  	type = FILEID_BTRFS_WITHOUT_PARENT;
>>  
>>  	fid->objectid = btrfs_ino(BTRFS_I(inode));
>> -	fid->root_objectid = BTRFS_I(inode)->root->objectid;
>> +	fid->root_objectid = BTRFS_I(inode)->root->root_key.objectid;
>>  	fid->gen = inode->i_generation;
>>  
>>  	if (parent) {
>> @@ -41,7 +41,7 @@ static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
>>  
>>  		fid->parent_objectid = BTRFS_I(parent)->location.objectid;
>>  		fid->parent_gen = parent->i_generation;
>> -		parent_root_id = BTRFS_I(parent)->root->objectid;
>> +		parent_root_id = BTRFS_I(parent)->root->root_key.objectid;
>>  
>>  		if (parent_root_id != fid->root_objectid) {
>>  			fid->parent_root_objectid = parent_root_id;
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index 3f51ddc18f98..78111d972af8 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -6613,7 +6613,7 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
>>  	int drop_inode = 0;
>>  
>>  	/* do not allow sys_link's with other subvols of the same device */
>> -	if (root->objectid != BTRFS_I(inode)->root->objectid)
>> +	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
>>  		return -EXDEV;
>>  
>>  	if (inode->i_nlink >= BTRFS_LINK_MAX)
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 6eaadddaca9f..8ab30c62df36 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4363,7 +4363,7 @@ static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
>>  		ret = PTR_ERR(new_root);
>>  		goto out;
>>  	}
>> -	if (!is_fstree(new_root->objectid)) {
>> +	if (!is_fstree(new_root->root_key.objectid)) {
>>  		ret = -ENOENT;
>>  		goto out;
>>  	}
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 2ba29f0609d9..16a9771b13fe 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -3004,7 +3004,7 @@ int btrfs_qgroup_reserve_data(struct inode *inode,
>>  	int ret;
>>  
>>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &root->fs_info->flags) ||
>> -	    !is_fstree(root->objectid) || len == 0)
>> +	    !is_fstree(root->root_key.objectid) || len == 0)
>>  		return 0;
>>  
>>  	/* @reserved parameter is mandatory for qgroup */
>> @@ -3090,7 +3090,7 @@ static int qgroup_free_reserved_data(struct inode *inode,
>>  			goto out;
>>  		freed += changeset.bytes_changed;
>>  	}
>> -	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
>> +	btrfs_qgroup_free_refroot(root->fs_info, root->root_key.objectid, freed,
>>  				  BTRFS_QGROUP_RSV_DATA);
>>  	ret = freed;
>>  out:
>> @@ -3122,7 +3122,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>>  					changeset.bytes_changed, trace_op);
>>  	if (free)
>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>> -				BTRFS_I(inode)->root->objectid,
>> +				BTRFS_I(inode)->root->root_key.objectid,
>>  				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>  	ret = changeset.bytes_changed;
>>  out:
>> @@ -3215,7 +3215,7 @@ int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
>>  	int ret;
>>  
>>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid) || num_bytes == 0)
>> +	    !is_fstree(root->root_key.objectid) || num_bytes == 0)
>>  		return 0;
>>  
>>  	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
>> @@ -3240,13 +3240,13 @@ void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>>  
>>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>  		return;
>>  
>>  	/* TODO: Update trace point to handle such free */
>>  	trace_qgroup_meta_free_all_pertrans(root);
>>  	/* Special value -1 means to free all reserved space */
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, (u64)-1,
>> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid, (u64)-1,
>>  				  BTRFS_QGROUP_RSV_META_PERTRANS);
>>  }
>>  
>> @@ -3256,7 +3256,7 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>>  
>>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>  		return;
>>  
>>  	/*
>> @@ -3267,7 +3267,8 @@ void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
>>  	num_bytes = sub_root_meta_rsv(root, num_bytes, type);
>>  	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
>>  	trace_qgroup_meta_reserve(root, type, -(s64)num_bytes);
>> -	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes, type);
>> +	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid,
>> +				  num_bytes, type);
>>  }
>>  
>>  static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
>> @@ -3321,13 +3322,13 @@ void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>>  
>>  	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
>> -	    !is_fstree(root->objectid))
>> +	    !is_fstree(root->root_key.objectid))
>>  		return;
>>  	/* Same as btrfs_qgroup_free_meta_prealloc() */
>>  	num_bytes = sub_root_meta_rsv(root, num_bytes,
>>  				      BTRFS_QGROUP_RSV_META_PREALLOC);
>>  	trace_qgroup_meta_convert(root, num_bytes);
>> -	qgroup_convert_meta(fs_info, root->objectid, num_bytes);
>> +	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
>>  }
>>  
>>  /*
>> @@ -3354,7 +3355,7 @@ void btrfs_qgroup_check_reserved_leak(struct inode *inode)
>>  				inode->i_ino, unode->val, unode->aux);
>>  		}
>>  		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
>> -				BTRFS_I(inode)->root->objectid,
>> +				BTRFS_I(inode)->root->root_key.objectid,
>>  				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
>>  
>>  	}
>> diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
>> index e5b9e596bb92..d69fbfb30aa9 100644
>> --- a/fs/btrfs/ref-verify.c
>> +++ b/fs/btrfs/ref-verify.c
>> @@ -732,7 +732,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>  
>>  	INIT_LIST_HEAD(&ra->list);
>>  	ra->action = action;
>> -	ra->root = root->objectid;
>> +	ra->root = root->root_key.objectid;
>>  
>>  	/*
>>  	 * This is an allocation, preallocate the block_entry in case we haven't
>> @@ -787,8 +787,8 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>  			 * one we want to lookup below when we modify the
>>  			 * re->num_refs.
>>  			 */
>> -			ref_root = root->objectid;
>> -			re->root_objectid = root->objectid;
>> +			ref_root = root->root_key.objectid;
>> +			re->root_objectid = root->root_key.objectid;
>>  			re->num_refs = 0;
>>  		}
>>  
>> @@ -862,7 +862,7 @@ int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
>>  			 * didn't thik of some other corner case.
>>  			 */
>>  			btrfs_err(fs_info, "failed to find root %llu for %llu",
>> -				  root->objectid, be->bytenr);
>> +				  root->root_key.objectid, be->bytenr);
>>  			dump_block_entry(fs_info, be);
>>  			dump_ref_action(fs_info, ra);
>>  			kfree(ra);
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 8783a1776540..d626362687d7 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -884,7 +884,8 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>>  		    cur->bytenr) {
>>  			btrfs_err(root->fs_info,
>>  	"couldn't find block (%llu) (level %d) in tree (%llu) with key (%llu %u %llu)",
>> -				  cur->bytenr, level - 1, root->objectid,
>> +				  cur->bytenr, level - 1,
>> +				  root->root_key.objectid,
>>  				  node_key->objectid, node_key->type,
>>  				  node_key->offset);
>>  			err = -ENOENT;
>> diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
>> index 551294a6c9e2..f003ec949726 100644
>> --- a/fs/btrfs/send.c
>> +++ b/fs/btrfs/send.c
>> @@ -1186,9 +1186,9 @@ static int __clone_root_cmp_bsearch(const void *key, const void *elt)
>>  	u64 root = (u64)(uintptr_t)key;
>>  	struct clone_root *cr = (struct clone_root *)elt;
>>  
>> -	if (root < cr->root->objectid)
>> +	if (root < cr->root->root_key.objectid)
>>  		return -1;
>> -	if (root > cr->root->objectid)
>> +	if (root > cr->root->root_key.objectid)
>>  		return 1;
>>  	return 0;
>>  }
>> @@ -1198,9 +1198,9 @@ static int __clone_root_cmp_sort(const void *e1, const void *e2)
>>  	struct clone_root *cr1 = (struct clone_root *)e1;
>>  	struct clone_root *cr2 = (struct clone_root *)e2;
>>  
>> -	if (cr1->root->objectid < cr2->root->objectid)
>> +	if (cr1->root->root_key.objectid < cr2->root->root_key.objectid)
>>  		return -1;
>> -	if (cr1->root->objectid > cr2->root->objectid)
>> +	if (cr1->root->root_key.objectid > cr2->root->root_key.objectid)
>>  		return 1;
>>  	return 0;
>>  }
>> @@ -2346,7 +2346,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>>  		return -ENOMEM;
>>  	}
>>  
>> -	key.objectid = send_root->objectid;
>> +	key.objectid = send_root->root_key.objectid;
>>  	key.type = BTRFS_ROOT_BACKREF_KEY;
>>  	key.offset = 0;
>>  
>> @@ -2362,7 +2362,7 @@ static int send_subvol_begin(struct send_ctx *sctx)
>>  	leaf = path->nodes[0];
>>  	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
>>  	if (key.type != BTRFS_ROOT_BACKREF_KEY ||
>> -	    key.objectid != send_root->objectid) {
>> +	    key.objectid != send_root->root_key.objectid) {
>>  		ret = -ENOENT;
>>  		goto out;
>>  	}
>> @@ -4907,8 +4907,8 @@ static int send_clone(struct send_ctx *sctx,
>>  
>>  	btrfs_debug(sctx->send_root->fs_info,
>>  		    "send_clone offset=%llu, len=%d, clone_root=%llu, clone_inode=%llu, clone_offset=%llu",
>> -		    offset, len, clone_root->root->objectid, clone_root->ino,
>> -		    clone_root->offset);
>> +		    offset, len, clone_root->root->root_key.objectid,
>> +		    clone_root->ino, clone_root->offset);
>>  
>>  	p = fs_path_alloc();
>>  	if (!p)
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 67de3c0fc85b..f114e848f4c9 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2177,8 +2177,10 @@ static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
>>  	buf->f_fsid.val[0] = be32_to_cpu(fsid[0]) ^ be32_to_cpu(fsid[2]);
>>  	buf->f_fsid.val[1] = be32_to_cpu(fsid[1]) ^ be32_to_cpu(fsid[3]);
>>  	/* Mask in the root object ID too, to disambiguate subvols */
>> -	buf->f_fsid.val[0] ^= BTRFS_I(d_inode(dentry))->root->objectid >> 32;
>> -	buf->f_fsid.val[1] ^= BTRFS_I(d_inode(dentry))->root->objectid;
>> +	buf->f_fsid.val[0] ^=
>> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid >> 32;
>> +	buf->f_fsid.val[1] ^=
>> +		BTRFS_I(d_inode(dentry))->root->root_key.objectid;
>>  
>>  	return 0;
>>  }
>> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
>> index 3b84f5015029..c429bdda6348 100644
>> --- a/fs/btrfs/transaction.c
>> +++ b/fs/btrfs/transaction.c
>> @@ -118,7 +118,7 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans)
>>  		list_del_init(&root->dirty_list);
>>  		free_extent_buffer(root->commit_root);
>>  		root->commit_root = btrfs_root_node(root);
>> -		if (is_fstree(root->objectid))
>> +		if (is_fstree(root->root_key.objectid))
>>  			btrfs_unpin_free_ino(root);
>>  		clear_btree_io_tree(&root->dirty_log_pages);
>>  	}
>> @@ -2330,7 +2330,7 @@ int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
>>  	list_del_init(&root->root_list);
>>  	spin_unlock(&fs_info->trans_lock);
>>  
>> -	btrfs_debug(fs_info, "cleaner removing %llu", root->objectid);
>> +	btrfs_debug(fs_info, "cleaner removing %llu", root->root_key.objectid);
>>  
>>  	btrfs_kill_all_delayed_nodes(root);
>>  
>> diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
>> index b401c4e36394..abe3ff774f58 100644
>> --- a/include/trace/events/btrfs.h
>> +++ b/include/trace/events/btrfs.h
>> @@ -316,7 +316,7 @@ DECLARE_EVENT_CLASS(btrfs__file_extent_item_regular,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(bi->root->fs_info,
>> -		__entry->root_obj	= bi->root->objectid;
>> +		__entry->root_obj	= bi->root->root_key.objectid;
>>  		__entry->ino		= btrfs_ino(bi);
>>  		__entry->isize		= bi->vfs_inode.i_size;
>>  		__entry->disk_isize	= bi->disk_i_size;
>> @@ -367,7 +367,7 @@ DECLARE_EVENT_CLASS(
>>  
>>  	TP_fast_assign_btrfs(
>>  		bi->root->fs_info,
>> -		__entry->root_obj	= bi->root->objectid;
>> +		__entry->root_obj	= bi->root->root_key.objectid;
>>  		__entry->ino		= btrfs_ino(bi);
>>  		__entry->isize		= bi->vfs_inode.i_size;
>>  		__entry->disk_isize	= bi->disk_i_size;
>> @@ -1477,7 +1477,8 @@ DECLARE_EVENT_CLASS(btrfs__qgroup_rsv_data,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(btrfs_sb(inode->i_sb),
>> -		__entry->rootid		= BTRFS_I(inode)->root->objectid;
>> +		__entry->rootid		=
>> +			BTRFS_I(inode)->root->root_key.objectid;
>>  		__entry->ino		= btrfs_ino(BTRFS_I(inode));
>>  		__entry->start		= start;
>>  		__entry->len		= len;
>> @@ -1675,7 +1676,7 @@ TRACE_EVENT(qgroup_meta_reserve,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>  		__entry->diff		= diff;
>>  	),
>>  
>> @@ -1697,7 +1698,7 @@ TRACE_EVENT(qgroup_meta_convert,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>  		__entry->diff		= diff;
>>  	),
>>  
>> @@ -1721,7 +1722,7 @@ TRACE_EVENT(qgroup_meta_free_all_pertrans,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->refroot	= root->objectid;
>> +		__entry->refroot	= root->root_key.objectid;
>>  		spin_lock(&root->qgroup_meta_rsv_lock);
>>  		__entry->diff		= -(s64)root->qgroup_meta_rsv_pertrans;
>>  		spin_unlock(&root->qgroup_meta_rsv_lock);
>> @@ -1802,7 +1803,7 @@ TRACE_EVENT(btrfs_inode_mod_outstanding_extents,
>>  	),
>>  
>>  	TP_fast_assign_btrfs(root->fs_info,
>> -		__entry->root_objectid	= root->objectid;
>> +		__entry->root_objectid	= root->root_key.objectid;
>>  		__entry->ino		= ino;
>>  		__entry->mod		= mod;
>>  	),
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Sterba Aug. 13, 2018, 5:37 p.m. UTC | #4
On Mon, Aug 06, 2018 at 02:17:54PM +0800, Qu Wenruo wrote:
> > -	u64 objectid;
> 
> Off topic crazy idea here.
> 
> I think it is a little crazy, but it should save a lot of objectid
> related modification:
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 118346aceea9..e6d70f2309a3 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -1166,7 +1166,10 @@ struct btrfs_root {
> 
>         unsigned long state;
>         struct btrfs_root_item root_item;
> -       struct btrfs_key root_key;
> +       union {
> +               struct btrfs_key root_key;
> +               u64 objectid;
> +       };
>         struct btrfs_fs_info *fs_info;
>         struct extent_io_tree dirty_log_pages;
> 
> @@ -1198,7 +1201,6 @@ struct btrfs_root {
>         int last_log_commit;
>         pid_t log_start_pid;
> 
> -       u64 objectid;
>         u64 last_trans;
> 
>         u32 type;
> 
> I'm not sure if this is a really crazy idea or a dirty hack to reduce
> some modification.

Be wary of such tricks. This might make you feel good for a moment how
good your C knowlege is, and also might save a few keystrokes. And a few
years later this costs somebody a week of debugging a mysterious memory
corrupption under circumstances not imagined right now.

Try to write understandable code. If there is a reason to use tricks,
like the struct page has to do for performance reasons, then it must be
documented and justified.
David Sterba Aug. 13, 2018, 5:38 p.m. UTC | #5
On Mon, Aug 06, 2018 at 02:25:24PM +0900, Misono Tomohiro wrote:
> There are two members in struct btrfs_root which indicate root's
> objectid: ->objectid and ->root_key.objectid.
> 
> They are both set to the same value in __setup_root():
>   static void __setup_root(struct btrfs_root *root,
>                            struct btrfs_fs_info *fs_info,
>                            u64 objectid)
>   {
>     ...
>     root->objectid = objectid;
>     ...
>     root->root_key.objectid = objecitd;
>     ...
>   }
> and not changed to other value after initialization.
> 
> grep in btrfs directory shows both are used in many places:
>   $ grep -rI "root->root_key.objectid" | wc -l
>   133
>   $ grep -rI "root->objectid" | wc -l
>   55
>  (4.17, inc. some noise)
> 
> It is confusing to have two similar variable names and it seems
> that there is no rule about which should be used in a certain case.
> 
> Since ->root_key itself is needed for tree reloc tree, let's remove
> 'objecitd' member and unify code to use ->root_key.objectid in all places.
> 
> Signed-off-by: Misono Tomohiro <misono.tomohiro@jp.fujitsu.com>

Reviewed-by: David Sterba <dsterba@suse.com>

If you have further updates, please base them on top of this patch as it
looks good to me in its current form.
Qu Wenruo Aug. 14, 2018, 1:22 p.m. UTC | #6
On 2018/8/14 上午1:37, David Sterba wrote:
> On Mon, Aug 06, 2018 at 02:17:54PM +0800, Qu Wenruo wrote:
>>> -	u64 objectid;
>>
>> Off topic crazy idea here.
>>
>> I think it is a little crazy, but it should save a lot of objectid
>> related modification:
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index 118346aceea9..e6d70f2309a3 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -1166,7 +1166,10 @@ struct btrfs_root {
>>
>>         unsigned long state;
>>         struct btrfs_root_item root_item;
>> -       struct btrfs_key root_key;
>> +       union {
>> +               struct btrfs_key root_key;
>> +               u64 objectid;
>> +       };
>>         struct btrfs_fs_info *fs_info;
>>         struct extent_io_tree dirty_log_pages;
>>
>> @@ -1198,7 +1201,6 @@ struct btrfs_root {
>>         int last_log_commit;
>>         pid_t log_start_pid;
>>
>> -       u64 objectid;
>>         u64 last_trans;
>>
>>         u32 type;
>>
>> I'm not sure if this is a really crazy idea or a dirty hack to reduce
>> some modification.
> 
> Be wary of such tricks. This might make you feel good for a moment how
> good your C knowlege is, and also might save a few keystrokes. And a few
> years later this costs somebody a week of debugging a mysterious memory
> corrupption under circumstances not imagined right now.

Yep, that's why I'm calling this a "crazy idea" for the same reason.

Just wondering if there is any better way to do it without using a
trick, like anonymous structure inside btrfs_root?
(Purely to improve my C skill if possible)

Anyway, I'm completely OK with current patch.

Thanks,
Qu

> 
> Try to write understandable code. If there is a reason to use tricks,
> like the struct page has to do for performance reasons, then it must be
> documented and justified.
>

Patch
diff mbox series

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index ae750b1574a2..84006e3dd105 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -1468,7 +1468,7 @@  int btrfs_check_shared(struct btrfs_root *root, u64 inum, u64 bytenr)
 	struct seq_list elem = SEQ_LIST_INIT(elem);
 	int ret = 0;
 	struct share_check shared = {
-		.root_objectid = root->objectid,
+		.root_objectid = root->root_key.objectid,
 		.inum = inum,
 		.share_count = 0,
 	};
@@ -2031,7 +2031,8 @@  static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 			/* path must be released before calling iterate()! */
 			btrfs_debug(fs_root->fs_info,
 				"following ref at offset %u for inode %llu in tree %llu",
-				cur, found_key.objectid, fs_root->objectid);
+				cur, found_key.objectid,
+				fs_root->root_key.objectid);
 			ret = iterate(parent, name_len,
 				      (unsigned long)(iref + 1), eb, ctx);
 			if (ret)
diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 1343ac57b438..97d91e55b70a 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -206,7 +206,7 @@  static inline struct btrfs_inode *BTRFS_I(const struct inode *inode)
 static inline unsigned long btrfs_inode_hash(u64 objectid,
 					     const struct btrfs_root *root)
 {
-	u64 h = objectid ^ (root->objectid * GOLDEN_RATIO_PRIME);
+	u64 h = objectid ^ (root->root_key.objectid * GOLDEN_RATIO_PRIME);
 
 #if BITS_PER_LONG == 32
 	h = (h >> 32) ^ (h & 0xffffffff);
@@ -339,15 +339,15 @@  static inline void btrfs_print_data_csum_error(struct btrfs_inode *inode,
 	struct btrfs_root *root = inode->root;
 
 	/* Output minus objectid, which is more meaningful */
-	if (root->objectid >= BTRFS_LAST_FREE_OBJECTID)
+	if (root->root_key.objectid >= BTRFS_LAST_FREE_OBJECTID)
 		btrfs_warn_rl(root->fs_info,
 	"csum failed root %lld ino %lld off %llu csum 0x%08x expected csum 0x%08x mirror %d",
-			root->objectid, btrfs_ino(inode),
+			root->root_key.objectid, btrfs_ino(inode),
 			logical_start, csum, csum_expected, mirror_num);
 	else
 		btrfs_warn_rl(root->fs_info,
 	"csum failed root %llu ino %llu off %llu csum 0x%08x expected csum 0x%08x mirror %d",
-			root->objectid, btrfs_ino(inode),
+			root->root_key.objectid, btrfs_ino(inode),
 			logical_start, csum, csum_expected, mirror_num);
 }
 
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index d436fb4c002e..1f71695cb0a8 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -207,7 +207,7 @@  static void add_root_to_dirty_list(struct btrfs_root *root)
 	spin_lock(&fs_info->trans_lock);
 	if (!test_and_set_bit(BTRFS_ROOT_DIRTY, &root->state)) {
 		/* Want the extent tree to be the last on the list */
-		if (root->objectid == BTRFS_EXTENT_TREE_OBJECTID)
+		if (root->root_key.objectid == BTRFS_EXTENT_TREE_OBJECTID)
 			list_move_tail(&root->dirty_list,
 				       &fs_info->dirty_cowonly_roots);
 		else
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 318be7864072..5be7c2bc45c0 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1202,7 +1202,6 @@  struct btrfs_root {
 	int last_log_commit;
 	pid_t log_start_pid;
 
-	u64 objectid;
 	u64 last_trans;
 
 	u32 type;
diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c
index f51b509f2d9b..07187e4ab600 100644
--- a/fs/btrfs/delayed-inode.c
+++ b/fs/btrfs/delayed-inode.c
@@ -1462,7 +1462,7 @@  int btrfs_insert_delayed_dir_index(struct btrfs_trans_handle *trans,
 	if (unlikely(ret)) {
 		btrfs_err(trans->fs_info,
 			  "err add delayed dir index item(name: %.*s) into the insertion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
-			  name_len, name, delayed_node->root->objectid,
+			  name_len, name, delayed_node->root->root_key.objectid,
 			  delayed_node->inode_id, ret);
 		BUG();
 	}
@@ -1533,7 +1533,8 @@  int btrfs_delete_delayed_dir_index(struct btrfs_trans_handle *trans,
 	if (unlikely(ret)) {
 		btrfs_err(trans->fs_info,
 			  "err add delayed dir index item(index: %llu) into the deletion tree of the delayed node(root id: %llu, inode id: %llu, errno: %d)",
-			  index, node->root->objectid, node->inode_id, ret);
+			  index, node->root->root_key.objectid,
+			  node->inode_id, ret);
 		BUG();
 	}
 	mutex_unlock(&node->mutex);
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 468365dd3b59..3fe87f67869b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -125,8 +125,8 @@  struct async_submit_bio {
  * Different roots are used for different purposes and may nest inside each
  * other and they require separate keysets.  As lockdep keys should be
  * static, assign keysets according to the purpose of the root as indicated
- * by btrfs_root->objectid.  This ensures that all special purpose roots
- * have separate keysets.
+ * by btrfs_root->root_key.objectid.  This ensures that all special purpose
+ * roots have separate keysets.
  *
  * Lock-nesting across peer nodes is always done with the immediate parent
  * node locked thus preventing deadlock.  As lockdep doesn't know this, use
@@ -1148,7 +1148,6 @@  static void __setup_root(struct btrfs_root *root, struct btrfs_fs_info *fs_info,
 	root->state = 0;
 	root->orphan_cleanup_state = 0;
 
-	root->objectid = objectid;
 	root->last_trans = 0;
 	root->highest_objectid = 0;
 	root->nr_delalloc_inodes = 0;
diff --git a/fs/btrfs/export.c b/fs/btrfs/export.c
index 1f3755b3a37a..ddf28ecf17f9 100644
--- a/fs/btrfs/export.c
+++ b/fs/btrfs/export.c
@@ -33,7 +33,7 @@  static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
 	type = FILEID_BTRFS_WITHOUT_PARENT;
 
 	fid->objectid = btrfs_ino(BTRFS_I(inode));
-	fid->root_objectid = BTRFS_I(inode)->root->objectid;
+	fid->root_objectid = BTRFS_I(inode)->root->root_key.objectid;
 	fid->gen = inode->i_generation;
 
 	if (parent) {
@@ -41,7 +41,7 @@  static int btrfs_encode_fh(struct inode *inode, u32 *fh, int *max_len,
 
 		fid->parent_objectid = BTRFS_I(parent)->location.objectid;
 		fid->parent_gen = parent->i_generation;
-		parent_root_id = BTRFS_I(parent)->root->objectid;
+		parent_root_id = BTRFS_I(parent)->root->root_key.objectid;
 
 		if (parent_root_id != fid->root_objectid) {
 			fid->parent_root_objectid = parent_root_id;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 3f51ddc18f98..78111d972af8 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -6613,7 +6613,7 @@  static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
 	int drop_inode = 0;
 
 	/* do not allow sys_link's with other subvols of the same device */
-	if (root->objectid != BTRFS_I(inode)->root->objectid)
+	if (root->root_key.objectid != BTRFS_I(inode)->root->root_key.objectid)
 		return -EXDEV;
 
 	if (inode->i_nlink >= BTRFS_LINK_MAX)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6eaadddaca9f..8ab30c62df36 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -4363,7 +4363,7 @@  static long btrfs_ioctl_default_subvol(struct file *file, void __user *argp)
 		ret = PTR_ERR(new_root);
 		goto out;
 	}
-	if (!is_fstree(new_root->objectid)) {
+	if (!is_fstree(new_root->root_key.objectid)) {
 		ret = -ENOENT;
 		goto out;
 	}
diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 2ba29f0609d9..16a9771b13fe 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -3004,7 +3004,7 @@  int btrfs_qgroup_reserve_data(struct inode *inode,
 	int ret;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &root->fs_info->flags) ||
-	    !is_fstree(root->objectid) || len == 0)
+	    !is_fstree(root->root_key.objectid) || len == 0)
 		return 0;
 
 	/* @reserved parameter is mandatory for qgroup */
@@ -3090,7 +3090,7 @@  static int qgroup_free_reserved_data(struct inode *inode,
 			goto out;
 		freed += changeset.bytes_changed;
 	}
-	btrfs_qgroup_free_refroot(root->fs_info, root->objectid, freed,
+	btrfs_qgroup_free_refroot(root->fs_info, root->root_key.objectid, freed,
 				  BTRFS_QGROUP_RSV_DATA);
 	ret = freed;
 out:
@@ -3122,7 +3122,7 @@  static int __btrfs_qgroup_release_data(struct inode *inode,
 					changeset.bytes_changed, trace_op);
 	if (free)
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
-				BTRFS_I(inode)->root->objectid,
+				BTRFS_I(inode)->root->root_key.objectid,
 				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 	ret = changeset.bytes_changed;
 out:
@@ -3215,7 +3215,7 @@  int __btrfs_qgroup_reserve_meta(struct btrfs_root *root, int num_bytes,
 	int ret;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
-	    !is_fstree(root->objectid) || num_bytes == 0)
+	    !is_fstree(root->root_key.objectid) || num_bytes == 0)
 		return 0;
 
 	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
@@ -3240,13 +3240,13 @@  void btrfs_qgroup_free_meta_all_pertrans(struct btrfs_root *root)
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
-	    !is_fstree(root->objectid))
+	    !is_fstree(root->root_key.objectid))
 		return;
 
 	/* TODO: Update trace point to handle such free */
 	trace_qgroup_meta_free_all_pertrans(root);
 	/* Special value -1 means to free all reserved space */
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, (u64)-1,
+	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid, (u64)-1,
 				  BTRFS_QGROUP_RSV_META_PERTRANS);
 }
 
@@ -3256,7 +3256,7 @@  void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
-	    !is_fstree(root->objectid))
+	    !is_fstree(root->root_key.objectid))
 		return;
 
 	/*
@@ -3267,7 +3267,8 @@  void __btrfs_qgroup_free_meta(struct btrfs_root *root, int num_bytes,
 	num_bytes = sub_root_meta_rsv(root, num_bytes, type);
 	BUG_ON(num_bytes != round_down(num_bytes, fs_info->nodesize));
 	trace_qgroup_meta_reserve(root, type, -(s64)num_bytes);
-	btrfs_qgroup_free_refroot(fs_info, root->objectid, num_bytes, type);
+	btrfs_qgroup_free_refroot(fs_info, root->root_key.objectid,
+				  num_bytes, type);
 }
 
 static void qgroup_convert_meta(struct btrfs_fs_info *fs_info, u64 ref_root,
@@ -3321,13 +3322,13 @@  void btrfs_qgroup_convert_reserved_meta(struct btrfs_root *root, int num_bytes)
 	struct btrfs_fs_info *fs_info = root->fs_info;
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags) ||
-	    !is_fstree(root->objectid))
+	    !is_fstree(root->root_key.objectid))
 		return;
 	/* Same as btrfs_qgroup_free_meta_prealloc() */
 	num_bytes = sub_root_meta_rsv(root, num_bytes,
 				      BTRFS_QGROUP_RSV_META_PREALLOC);
 	trace_qgroup_meta_convert(root, num_bytes);
-	qgroup_convert_meta(fs_info, root->objectid, num_bytes);
+	qgroup_convert_meta(fs_info, root->root_key.objectid, num_bytes);
 }
 
 /*
@@ -3354,7 +3355,7 @@  void btrfs_qgroup_check_reserved_leak(struct inode *inode)
 				inode->i_ino, unode->val, unode->aux);
 		}
 		btrfs_qgroup_free_refroot(BTRFS_I(inode)->root->fs_info,
-				BTRFS_I(inode)->root->objectid,
+				BTRFS_I(inode)->root->root_key.objectid,
 				changeset.bytes_changed, BTRFS_QGROUP_RSV_DATA);
 
 	}
diff --git a/fs/btrfs/ref-verify.c b/fs/btrfs/ref-verify.c
index e5b9e596bb92..d69fbfb30aa9 100644
--- a/fs/btrfs/ref-verify.c
+++ b/fs/btrfs/ref-verify.c
@@ -732,7 +732,7 @@  int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 
 	INIT_LIST_HEAD(&ra->list);
 	ra->action = action;
-	ra->root = root->objectid;
+	ra->root = root->root_key.objectid;
 
 	/*
 	 * This is an allocation, preallocate the block_entry in case we haven't
@@ -787,8 +787,8 @@  int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 			 * one we want to lookup below when we modify the
 			 * re->num_refs.
 			 */
-			ref_root = root->objectid;
-			re->root_objectid = root->objectid;
+			ref_root = root->root_key.objectid;
+			re->root_objectid = root->root_key.objectid;
 			re->num_refs = 0;
 		}
 
@@ -862,7 +862,7 @@  int btrfs_ref_tree_mod(struct btrfs_root *root, u64 bytenr, u64 num_bytes,
 			 * didn't thik of some other corner case.
 			 */
 			btrfs_err(fs_info, "failed to find root %llu for %llu",
-				  root->objectid, be->bytenr);
+				  root->root_key.objectid, be->bytenr);
 			dump_block_entry(fs_info, be);
 			dump_ref_action(fs_info, ra);
 			kfree(ra);
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 8783a1776540..d626362687d7 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -884,7 +884,8 @@  struct backref_node *build_backref_tree(struct reloc_control *rc,
 		    cur->bytenr) {
 			btrfs_err(root->fs_info,
 	"couldn't find block (%llu) (level %d) in tree (%llu) with key (%llu %u %llu)",
-				  cur->bytenr, level - 1, root->objectid,
+				  cur->bytenr, level - 1,
+				  root->root_key.objectid,
 				  node_key->objectid, node_key->type,
 				  node_key->offset);
 			err = -ENOENT;
diff --git a/fs/btrfs/send.c b/fs/btrfs/send.c
index 551294a6c9e2..f003ec949726 100644
--- a/fs/btrfs/send.c
+++ b/fs/btrfs/send.c
@@ -1186,9 +1186,9 @@  static int __clone_root_cmp_bsearch(const void *key, const void *elt)
 	u64 root = (u64)(uintptr_t)key;
 	struct clone_root *cr = (struct clone_root *)elt;
 
-	if (root < cr->root->objectid)
+	if (root < cr->root->root_key.objectid)
 		return -1;
-	if (root > cr->root->objectid)
+	if (root > cr->root->root_key.objectid)
 		return 1;
 	return 0;
 }
@@ -1198,9 +1198,9 @@  static int __clone_root_cmp_sort(const void *e1, const void *e2)
 	struct clone_root *cr1 = (struct clone_root *)e1;
 	struct clone_root *cr2 = (struct clone_root *)e2;
 
-	if (cr1->root->objectid < cr2->root->objectid)
+	if (cr1->root->root_key.objectid < cr2->root->root_key.objectid)
 		return -1;
-	if (cr1->root->objectid > cr2->root->objectid)
+	if (cr1->root->root_key.objectid > cr2->root->root_key.objectid)
 		return 1;
 	return 0;
 }
@@ -2346,7 +2346,7 @@  static int send_subvol_begin(struct send_ctx *sctx)
 		return -ENOMEM;
 	}
 
-	key.objectid = send_root->objectid;
+	key.objectid = send_root->root_key.objectid;
 	key.type = BTRFS_ROOT_BACKREF_KEY;
 	key.offset = 0;
 
@@ -2362,7 +2362,7 @@  static int send_subvol_begin(struct send_ctx *sctx)
 	leaf = path->nodes[0];
 	btrfs_item_key_to_cpu(leaf, &key, path->slots[0]);
 	if (key.type != BTRFS_ROOT_BACKREF_KEY ||
-	    key.objectid != send_root->objectid) {
+	    key.objectid != send_root->root_key.objectid) {
 		ret = -ENOENT;
 		goto out;
 	}
@@ -4907,8 +4907,8 @@  static int send_clone(struct send_ctx *sctx,
 
 	btrfs_debug(sctx->send_root->fs_info,
 		    "send_clone offset=%llu, len=%d, clone_root=%llu, clone_inode=%llu, clone_offset=%llu",
-		    offset, len, clone_root->root->objectid, clone_root->ino,
-		    clone_root->offset);
+		    offset, len, clone_root->root->root_key.objectid,
+		    clone_root->ino, clone_root->offset);
 
 	p = fs_path_alloc();
 	if (!p)
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 67de3c0fc85b..f114e848f4c9 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -2177,8 +2177,10 @@  static int btrfs_statfs(struct dentry *dentry, struct kstatfs *buf)
 	buf->f_fsid.val[0] = be32_to_cpu(fsid[0]) ^ be32_to_cpu(fsid[2]);
 	buf->f_fsid.val[1] = be32_to_cpu(fsid[1]) ^ be32_to_cpu(fsid[3]);
 	/* Mask in the root object ID too, to disambiguate subvols */
-	buf->f_fsid.val[0] ^= BTRFS_I(d_inode(dentry))->root->objectid >> 32;
-	buf->f_fsid.val[1] ^= BTRFS_I(d_inode(dentry))->root->objectid;
+	buf->f_fsid.val[0] ^=
+		BTRFS_I(d_inode(dentry))->root->root_key.objectid >> 32;
+	buf->f_fsid.val[1] ^=
+		BTRFS_I(d_inode(dentry))->root->root_key.objectid;
 
 	return 0;
 }
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index 3b84f5015029..c429bdda6348 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -118,7 +118,7 @@  static noinline void switch_commit_roots(struct btrfs_transaction *trans)
 		list_del_init(&root->dirty_list);
 		free_extent_buffer(root->commit_root);
 		root->commit_root = btrfs_root_node(root);
-		if (is_fstree(root->objectid))
+		if (is_fstree(root->root_key.objectid))
 			btrfs_unpin_free_ino(root);
 		clear_btree_io_tree(&root->dirty_log_pages);
 	}
@@ -2330,7 +2330,7 @@  int btrfs_clean_one_deleted_snapshot(struct btrfs_root *root)
 	list_del_init(&root->root_list);
 	spin_unlock(&fs_info->trans_lock);
 
-	btrfs_debug(fs_info, "cleaner removing %llu", root->objectid);
+	btrfs_debug(fs_info, "cleaner removing %llu", root->root_key.objectid);
 
 	btrfs_kill_all_delayed_nodes(root);
 
diff --git a/include/trace/events/btrfs.h b/include/trace/events/btrfs.h
index b401c4e36394..abe3ff774f58 100644
--- a/include/trace/events/btrfs.h
+++ b/include/trace/events/btrfs.h
@@ -316,7 +316,7 @@  DECLARE_EVENT_CLASS(btrfs__file_extent_item_regular,
 	),
 
 	TP_fast_assign_btrfs(bi->root->fs_info,
-		__entry->root_obj	= bi->root->objectid;
+		__entry->root_obj	= bi->root->root_key.objectid;
 		__entry->ino		= btrfs_ino(bi);
 		__entry->isize		= bi->vfs_inode.i_size;
 		__entry->disk_isize	= bi->disk_i_size;
@@ -367,7 +367,7 @@  DECLARE_EVENT_CLASS(
 
 	TP_fast_assign_btrfs(
 		bi->root->fs_info,
-		__entry->root_obj	= bi->root->objectid;
+		__entry->root_obj	= bi->root->root_key.objectid;
 		__entry->ino		= btrfs_ino(bi);
 		__entry->isize		= bi->vfs_inode.i_size;
 		__entry->disk_isize	= bi->disk_i_size;
@@ -1477,7 +1477,8 @@  DECLARE_EVENT_CLASS(btrfs__qgroup_rsv_data,
 	),
 
 	TP_fast_assign_btrfs(btrfs_sb(inode->i_sb),
-		__entry->rootid		= BTRFS_I(inode)->root->objectid;
+		__entry->rootid		=
+			BTRFS_I(inode)->root->root_key.objectid;
 		__entry->ino		= btrfs_ino(BTRFS_I(inode));
 		__entry->start		= start;
 		__entry->len		= len;
@@ -1675,7 +1676,7 @@  TRACE_EVENT(qgroup_meta_reserve,
 	),
 
 	TP_fast_assign_btrfs(root->fs_info,
-		__entry->refroot	= root->objectid;
+		__entry->refroot	= root->root_key.objectid;
 		__entry->diff		= diff;
 	),
 
@@ -1697,7 +1698,7 @@  TRACE_EVENT(qgroup_meta_convert,
 	),
 
 	TP_fast_assign_btrfs(root->fs_info,
-		__entry->refroot	= root->objectid;
+		__entry->refroot	= root->root_key.objectid;
 		__entry->diff		= diff;
 	),
 
@@ -1721,7 +1722,7 @@  TRACE_EVENT(qgroup_meta_free_all_pertrans,
 	),
 
 	TP_fast_assign_btrfs(root->fs_info,
-		__entry->refroot	= root->objectid;
+		__entry->refroot	= root->root_key.objectid;
 		spin_lock(&root->qgroup_meta_rsv_lock);
 		__entry->diff		= -(s64)root->qgroup_meta_rsv_pertrans;
 		spin_unlock(&root->qgroup_meta_rsv_lock);
@@ -1802,7 +1803,7 @@  TRACE_EVENT(btrfs_inode_mod_outstanding_extents,
 	),
 
 	TP_fast_assign_btrfs(root->fs_info,
-		__entry->root_objectid	= root->objectid;
+		__entry->root_objectid	= root->root_key.objectid;
 		__entry->ino		= ino;
 		__entry->mod		= mod;
 	),