Liu Bo
2012-Oct-19 09:50 UTC
[PATCH 1/4] Btrfs: MOD_LOG_KEY_REMOVE_WHILE_MOVING never change node''s nritems
Key MOD_LOG_KEY_REMOVE_WHILE_MOVING means that we''re doing memmove inside an extent buffer node, and the node''s number of items remains unchanged (unless we are inserting a single pointer, but we have MOD_LOG_KEY_ADD for that). So we don''t need to increase node''s number of items during rewinding, otherwise we may get an node larger than leafsize and cause general protection errors later. Here is the details, - If we do memory move for inserting a single pointer, we need to add node''s nritems by one, and we honor MOD_LOG_KEY_ADD for adding. - If we do memory move for deleting a single pointer, we need to decrease node''s nritems by one, and we honor MOD_LOG_KEY_REMOVE for deleting. - If we do memory move for balance left/right, we need to decrease node''s nritems, and we honor MOD_LOG_KEY_REMOVE for balaning. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ctree.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index b334362..2858c2f 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1142,13 +1142,13 @@ __tree_mod_log_rewind(struct extent_buffer *eb, u64 time_seq, switch (tm->op) { case MOD_LOG_KEY_REMOVE_WHILE_FREEING: BUG_ON(tm->slot < n); - case MOD_LOG_KEY_REMOVE_WHILE_MOVING: case MOD_LOG_KEY_REMOVE: + n++; + case MOD_LOG_KEY_REMOVE_WHILE_MOVING: btrfs_set_node_key(eb, &tm->key, tm->slot); btrfs_set_node_blockptr(eb, tm->slot, tm->blockptr); btrfs_set_node_ptr_generation(eb, tm->slot, tm->generation); - n++; break; case MOD_LOG_KEY_REPLACE: BUG_ON(tm->slot >= n); -- 1.7.7.6 -- 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
Liu Bo
2012-Oct-19 09:50 UTC
[PATCH 2/4] Btrfs: reorder tree mod log operations in deleting a pointer
Since we don''t use MOD_LOG_KEY_REMOVE_WHILE_MOVING to add nritems during rewinding, we should insert a MOD_LOG_KEY_REMOVE operation first. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ctree.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 2858c2f..bbea4fb 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -4569,6 +4569,12 @@ static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root, u32 nritems; int ret; + if (tree_mod_log && level) { + ret = tree_mod_log_insert_key(root->fs_info, parent, slot, + MOD_LOG_KEY_REMOVE); + BUG_ON(ret < 0); + } + nritems = btrfs_header_nritems(parent); if (slot != nritems - 1) { if (tree_mod_log && level) @@ -4579,10 +4585,6 @@ static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root, btrfs_node_key_ptr_offset(slot + 1), sizeof(struct btrfs_key_ptr) * (nritems - slot - 1)); - } else if (tree_mod_log && level) { - ret = tree_mod_log_insert_key(root->fs_info, parent, slot, - MOD_LOG_KEY_REMOVE); - BUG_ON(ret < 0); } nritems--; -- 1.7.7.6 -- 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
The argument ''tree_mod_log'' is not necessary since all of callers enable it. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ctree.c | 16 +++++++--------- 1 files changed, 7 insertions(+), 9 deletions(-) diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index bbea4fb..235831f 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -38,8 +38,7 @@ static int balance_node_right(struct btrfs_trans_handle *trans, struct extent_buffer *dst_buf, struct extent_buffer *src_buf); static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct btrfs_path *path, int level, int slot, - int tree_mod_log); + struct btrfs_path *path, int level, int slot); static void tree_mod_log_free_eb(struct btrfs_fs_info *fs_info, struct extent_buffer *eb); struct extent_buffer *read_old_tree_block(struct btrfs_root *root, u64 bytenr, @@ -1789,7 +1788,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, if (btrfs_header_nritems(right) == 0) { clean_tree_block(trans, root, right); btrfs_tree_unlock(right); - del_ptr(trans, root, path, level + 1, pslot + 1, 1); + del_ptr(trans, root, path, level + 1, pslot + 1); root_sub_used(root, right->len); btrfs_free_tree_block(trans, root, right, 0, 1); free_extent_buffer_stale(right); @@ -1833,7 +1832,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, if (btrfs_header_nritems(mid) == 0) { clean_tree_block(trans, root, mid); btrfs_tree_unlock(mid); - del_ptr(trans, root, path, level + 1, pslot, 1); + del_ptr(trans, root, path, level + 1, pslot); root_sub_used(root, mid->len); btrfs_free_tree_block(trans, root, mid, 0, 1); free_extent_buffer_stale(mid); @@ -4562,14 +4561,13 @@ int btrfs_insert_item(struct btrfs_trans_handle *trans, struct btrfs_root * empty a node. */ static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root, - struct btrfs_path *path, int level, int slot, - int tree_mod_log) + struct btrfs_path *path, int level, int slot) { struct extent_buffer *parent = path->nodes[level]; u32 nritems; int ret; - if (tree_mod_log && level) { + if (level) { ret = tree_mod_log_insert_key(root->fs_info, parent, slot, MOD_LOG_KEY_REMOVE); BUG_ON(ret < 0); @@ -4577,7 +4575,7 @@ static void del_ptr(struct btrfs_trans_handle *trans, struct btrfs_root *root, nritems = btrfs_header_nritems(parent); if (slot != nritems - 1) { - if (tree_mod_log && level) + if (level) tree_mod_log_eb_move(root->fs_info, parent, slot, slot + 1, nritems - slot - 1); memmove_extent_buffer(parent, @@ -4618,7 +4616,7 @@ static noinline void btrfs_del_leaf(struct btrfs_trans_handle *trans, struct extent_buffer *leaf) { WARN_ON(btrfs_header_generation(leaf) != trans->transid); - del_ptr(trans, root, path, 1, path->slots[1], 1); + del_ptr(trans, root, path, 1, path->slots[1]); /* * btrfs_free_extent is expensive, we want to make sure we -- 1.7.7.6 -- 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
''disk_key'' is not used at all. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/ctree.c | 3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c index 235831f..7f649d2 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -772,8 +772,7 @@ tree_mod_log_eb_move(struct btrfs_fs_info *fs_info, struct extent_buffer *dst, static noinline void tree_mod_log_set_node_key(struct btrfs_fs_info *fs_info, - struct extent_buffer *eb, - struct btrfs_disk_key *disk_key, int slot, int atomic) + struct extent_buffer *eb, int slot, int atomic) { int ret; -- 1.7.7.6 -- 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
Hi liubo, Patches 3 and 4 are looking good. I''m still trying to sort out on the other two, as I cannot reproduce your bug with your script, unfortunately. After applying all 4 patches, the result doesn''t compile, here: fs/btrfs/ctree.c: In function ''balance_level'': fs/btrfs/ctree.c:1799: warning: passing argument 3 of ''tree_mod_log_set_node_key'' makes integer from pointer without a cast fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct btrfs_disk_key *'' fs/btrfs/ctree.c:1799: error: too many arguments to function ''tree_mod_log_set_node_key'' fs/btrfs/ctree.c:1844: warning: passing argument 3 of ''tree_mod_log_set_node_key'' makes integer from pointer without a cast fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct btrfs_disk_key *'' fs/btrfs/ctree.c:1844: error: too many arguments to function ''tree_mod_log_set_node_key'' fs/btrfs/ctree.c: In function ''push_nodes_for_insert'': fs/btrfs/ctree.c:1943: warning: passing argument 3 of ''tree_mod_log_set_node_key'' makes integer from pointer without a cast fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct btrfs_disk_key *'' fs/btrfs/ctree.c:1943: error: too many arguments to function ''tree_mod_log_set_node_key'' fs/btrfs/ctree.c:1996: warning: passing argument 3 of ''tree_mod_log_set_node_key'' makes integer from pointer without a cast fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct btrfs_disk_key *'' fs/btrfs/ctree.c:1996: error: too many arguments to function ''tree_mod_log_set_node_key'' fs/btrfs/ctree.c: In function ''fixup_low_keys'': fs/btrfs/ctree.c:2880: warning: passing argument 3 of ''tree_mod_log_set_node_key'' makes integer from pointer without a cast fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct btrfs_disk_key *'' fs/btrfs/ctree.c:2880: error: too many arguments to function ''tree_mod_log_set_node_key'' CC [M] fs/btrfs/sysfs.o make[2]: *** [fs/btrfs/ctree.o] Error 1 make[2]: *** Waiting for unfinished jobs.... make[1]: *** [fs/btrfs] Error 2 make[1]: *** Waiting for unfinished jobs.... make: *** [fs] Error 2 -Jan -- 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
On 10/19/2012 08:14 PM, Jan Schmidt wrote:> Hi liubo, > > Patches 3 and 4 are looking good. I''m still trying to sort out on the other two, > as I cannot reproduce your bug with your script, unfortunately. > > After applying all 4 patches, the result doesn''t compile, here: > > fs/btrfs/ctree.c: In function ''balance_level'': > fs/btrfs/ctree.c:1799: warning: passing argument 3 of > ''tree_mod_log_set_node_key'' makes integer from pointer without a cast > fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct > btrfs_disk_key *'' > fs/btrfs/ctree.c:1799: error: too many arguments to function > ''tree_mod_log_set_node_key'' > fs/btrfs/ctree.c:1844: warning: passing argument 3 of > ''tree_mod_log_set_node_key'' makes integer from pointer without a cast > fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct > btrfs_disk_key *'' > fs/btrfs/ctree.c:1844: error: too many arguments to function > ''tree_mod_log_set_node_key'' > fs/btrfs/ctree.c: In function ''push_nodes_for_insert'': > fs/btrfs/ctree.c:1943: warning: passing argument 3 of > ''tree_mod_log_set_node_key'' makes integer from pointer without a cast > fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct > btrfs_disk_key *'' > fs/btrfs/ctree.c:1943: error: too many arguments to function > ''tree_mod_log_set_node_key'' > fs/btrfs/ctree.c:1996: warning: passing argument 3 of > ''tree_mod_log_set_node_key'' makes integer from pointer without a cast > fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct > btrfs_disk_key *'' > fs/btrfs/ctree.c:1996: error: too many arguments to function > ''tree_mod_log_set_node_key'' > fs/btrfs/ctree.c: In function ''fixup_low_keys'': > fs/btrfs/ctree.c:2880: warning: passing argument 3 of > ''tree_mod_log_set_node_key'' makes integer from pointer without a cast > fs/btrfs/ctree.c:774: note: expected ''int'' but argument is of type ''struct > btrfs_disk_key *'' > fs/btrfs/ctree.c:2880: error: too many arguments to function > ''tree_mod_log_set_node_key'' > CC [M] fs/btrfs/sysfs.o > make[2]: *** [fs/btrfs/ctree.o] Error 1 > make[2]: *** Waiting for unfinished jobs.... > make[1]: *** [fs/btrfs] Error 2 > make[1]: *** Waiting for unfinished jobs.... > make: *** [fs] Error 2 >oh, I''m so sorry, I forgot to format new patches and sent the wrong version. Sorry for the trouble. thanks, liubo> -Jan > -- > 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 >-- 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