* [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
[not found] <cover.1533531264.git.misono.tomohiro@jp.fujitsu.com>
@ 2018-08-06 5:25 ` Misono Tomohiro
2018-08-06 6:17 ` Qu Wenruo
2018-08-13 17:38 ` David Sterba
0 siblings, 2 replies; 7+ messages in thread
From: Misono Tomohiro @ 2018-08-06 5:25 UTC (permalink / raw)
To: linux-btrfs
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(-)
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;
),
--
2.14.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-06 5:25 ` [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root Misono Tomohiro
@ 2018-08-06 6:17 ` Qu Wenruo
2018-08-06 6:37 ` Su Yue
` (2 more replies)
2018-08-13 17:38 ` David Sterba
1 sibling, 3 replies; 7+ messages in thread
From: Qu Wenruo @ 2018-08-06 6:17 UTC (permalink / raw)
To: Misono Tomohiro, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 20394 bytes --]
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;
> ),
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-06 6:17 ` Qu Wenruo
@ 2018-08-06 6:37 ` Su Yue
2018-08-06 6:41 ` Misono Tomohiro
2018-08-13 17:37 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: Su Yue @ 2018-08-06 6:37 UTC (permalink / raw)
To: Qu Wenruo, Misono Tomohiro, linux-btrfs
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;
>> ),
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-06 6:17 ` Qu Wenruo
2018-08-06 6:37 ` Su Yue
@ 2018-08-06 6:41 ` Misono Tomohiro
2018-08-13 17:37 ` David Sterba
2 siblings, 0 replies; 7+ messages in thread
From: Misono Tomohiro @ 2018-08-06 6:41 UTC (permalink / raw)
To: Qu Wenruo, linux-btrfs
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;
>> ),
>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-06 6:17 ` Qu Wenruo
2018-08-06 6:37 ` Su Yue
2018-08-06 6:41 ` Misono Tomohiro
@ 2018-08-13 17:37 ` David Sterba
2018-08-14 13:22 ` Qu Wenruo
2 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2018-08-13 17:37 UTC (permalink / raw)
To: Qu Wenruo; +Cc: Misono Tomohiro, linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-06 5:25 ` [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root Misono Tomohiro
2018-08-06 6:17 ` Qu Wenruo
@ 2018-08-13 17:38 ` David Sterba
1 sibling, 0 replies; 7+ messages in thread
From: David Sterba @ 2018-08-13 17:38 UTC (permalink / raw)
To: Misono Tomohiro; +Cc: linux-btrfs
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root
2018-08-13 17:37 ` David Sterba
@ 2018-08-14 13:22 ` Qu Wenruo
0 siblings, 0 replies; 7+ messages in thread
From: Qu Wenruo @ 2018-08-14 13:22 UTC (permalink / raw)
To: dsterba, Misono Tomohiro, linux-btrfs
[-- Attachment #1.1: Type: text/plain, Size: 1883 bytes --]
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.
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-08-14 16:09 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <cover.1533531264.git.misono.tomohiro@jp.fujitsu.com>
2018-08-06 5:25 ` [RFC PATCH] btrfs: Remove 'objectid' member from struct btrfs_root Misono Tomohiro
2018-08-06 6:17 ` Qu Wenruo
2018-08-06 6:37 ` Su Yue
2018-08-06 6:41 ` Misono Tomohiro
2018-08-13 17:37 ` David Sterba
2018-08-14 13:22 ` Qu Wenruo
2018-08-13 17:38 ` David Sterba
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.