[PATCH] Btrfs: MOD_LOG_KEY_REMOVE_WHILE_MOVING never change node's nritems

[PATCH] Btrfs: MOD_LOG_KEY_REMOVE_WHILE_MOVING never change node's nritems

[Liu Bo]

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,
so we don't need to increment node's number of items during rewinding,
otherwise we may get an node larger than leafsize and cause general protection
errors later.

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

Re: [PATCH] Btrfs: MOD_LOG_KEY_REMOVE_WHILE_MOVING never change node's nritems

[Jan Schmidt]

On Thu, October 18, 2012 at 09:32 (+0200), Liu Bo wrote:
> 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,
> so we don't need to increment node's number of items during rewinding,
> otherwise we may get an node larger than leafsize and cause general protection
> errors later.

This patch triggers the following bug on when running fsmark on a quota enabled
file system:

1153                 case MOD_LOG_KEY_REPLACE:
1154                         BUG_ON(tm->slot >= n);

MOD_LOG_KEY_REMOVE_WHILE_MOVING elements are added by tree_mod_log_insert_move
when dst_slot < src_slot, thus we're reducing the number of elements in the
buffer, thus replaying adds those elements.

-Jan

Re: [PATCH] Btrfs: MOD_LOG_KEY_REMOVE_WHILE_MOVING never change node's nritems

[Liu Bo]

On 10/19/2012 01:13 PM, Jan Schmidt wrote:
> On Thu, October 18, 2012 at 09:32 (+0200), Liu Bo wrote:
>> 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,
>> so we don't need to increment node's number of items during rewinding,
>> otherwise we may get an node larger than leafsize and cause general protection
>> errors later.
> 
> This patch triggers the following bug on when running fsmark on a quota enabled
> file system:
> 
> 1153                 case MOD_LOG_KEY_REPLACE:
> 1154                         BUG_ON(tm->slot >= n);
> 
> MOD_LOG_KEY_REMOVE_WHILE_MOVING elements are added by tree_mod_log_insert_move
> when dst_slot < src_slot, thus we're reducing the number of elements in the
> buffer, thus replaying adds those elements.
> 

Hi Jan,

I doubt it.

tree_mod_log_insert_move() has only one caller tree_mod_log_eb_move(), and
tree_mod_log_eb_move() has 4 callers:
- push_node_left()
- balance_node_right()
- insert_ptr()
- del_ptr()

Among the 4 callers, only del_ptr() modifies nritems without inserting other keys,
like MOD_LOG_KEY_ADD, etc.

So IMO memmove does not modify the nritems:

| - - - 3 4 5 | => | 3 4 5 - - - |     (nritems remain unchanged)

It is other operations (MOD_LOG_KEY_ADD, MOD_LOG_KEY_REMOVE, MOD_LOG_KEY_REMOVE_WHILE_FREEING)
that modify the nritems.

I think the following patch can help us:

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 6d183f6..cdd5995 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -4722,7 +4722,8 @@ 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) {
+	}
+	if (tree_mod_log && level) {
 		ret = tree_mod_log_insert_key(root->fs_info, parent, slot,
 					      MOD_LOG_KEY_REMOVE);
 		BUG_ON(ret < 0);


thanks,
liubo