[PATCH] Btrfs: remove assertion when searching for a key in a node/leaf

[PATCH] Btrfs: remove assertion when searching for a key in a node/leaf

[fdmanana]

From: Filipe Manana <fdmanana@suse.com>

At ctree.c:key_search(), the assertion that verifies the first key on a
child extent buffer corresponds to the key at a specific slot in the
parent has a disadvantage: we effectively hit a BUG_ON() which requires
rebooting the machine later. It also does not tell any information about
which extent buffer is affected, from which root, the expected and found
keys, etc.

However as of commit 581c1760415c48 ("btrfs: Validate child tree block's
level and first key"), that assertion is not needed since at the time we
read an extent buffer from disk we validate that its first key matches the
key, at the respective slot, in the parent extent buffer. Therefore just
remove the assertion at key_search().

Signed-off-by: Filipe Manana <fdmanana@suse.com>
---
 fs/btrfs/ctree.c | 21 ---------------------
 1 file changed, 21 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 5b9f602fb9e2..e754bd019618 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2529,26 +2529,6 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-static void key_search_validate(struct extent_buffer *b,
-				const struct btrfs_key *key,
-				int level)
-{
-#ifdef CONFIG_BTRFS_ASSERT
-	struct btrfs_disk_key disk_key;
-
-	btrfs_cpu_key_to_disk(&disk_key, key);
-
-	if (level == 0)
-		ASSERT(!memcmp_extent_buffer(b, &disk_key,
-		    offsetof(struct btrfs_leaf, items[0].key),
-		    sizeof(disk_key)));
-	else
-		ASSERT(!memcmp_extent_buffer(b, &disk_key,
-		    offsetof(struct btrfs_node, ptrs[0].key),
-		    sizeof(disk_key)));
-#endif
-}
-
 static int key_search(struct extent_buffer *b, const struct btrfs_key *key,
 		      int level, int *prev_cmp, int *slot)
 {
@@ -2557,7 +2537,6 @@ static int key_search(struct extent_buffer *b, const struct btrfs_key *key,
 		return *prev_cmp;
 	}
 
-	key_search_validate(b, key, level);
 	*slot = 0;
 
 	return 0;
-- 
2.11.0

Re: [PATCH] Btrfs: remove assertion when searching for a key in a node/leaf

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 2226 bytes --]



On 2019/2/20 下午7:11, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At ctree.c:key_search(), the assertion that verifies the first key on a
> child extent buffer corresponds to the key at a specific slot in the
> parent has a disadvantage: we effectively hit a BUG_ON() which requires
> rebooting the machine later. It also does not tell any information about
> which extent buffer is affected, from which root, the expected and found
> keys, etc.
> 
> However as of commit 581c1760415c48 ("btrfs: Validate child tree block's
> level and first key"), that assertion is not needed since at the time we
> read an extent buffer from disk we validate that its first key matches the
> key, at the respective slot, in the parent extent buffer. Therefore just
> remove the assertion at key_search().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/ctree.c | 21 ---------------------
>  1 file changed, 21 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 5b9f602fb9e2..e754bd019618 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2529,26 +2529,6 @@ setup_nodes_for_search(struct btrfs_trans_handle *trans,
>  	return ret;
>  }
>  
> -static void key_search_validate(struct extent_buffer *b,
> -				const struct btrfs_key *key,
> -				int level)
> -{
> -#ifdef CONFIG_BTRFS_ASSERT
> -	struct btrfs_disk_key disk_key;
> -
> -	btrfs_cpu_key_to_disk(&disk_key, key);
> -
> -	if (level == 0)
> -		ASSERT(!memcmp_extent_buffer(b, &disk_key,
> -		    offsetof(struct btrfs_leaf, items[0].key),
> -		    sizeof(disk_key)));
> -	else
> -		ASSERT(!memcmp_extent_buffer(b, &disk_key,
> -		    offsetof(struct btrfs_node, ptrs[0].key),
> -		    sizeof(disk_key)));
> -#endif
> -}
> -
>  static int key_search(struct extent_buffer *b, const struct btrfs_key *key,
>  		      int level, int *prev_cmp, int *slot)
>  {
> @@ -2557,7 +2537,6 @@ static int key_search(struct extent_buffer *b, const struct btrfs_key *key,
>  		return *prev_cmp;
>  	}
>  
> -	key_search_validate(b, key, level);
>  	*slot = 0;
>  
>  	return 0;
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] Btrfs: remove assertion when searching for a key in a node/leaf

[David Sterba]

On Wed, Feb 20, 2019 at 11:11:43AM +0000, fdmanana@kernel.org wrote:
> From: Filipe Manana <fdmanana@suse.com>
> 
> At ctree.c:key_search(), the assertion that verifies the first key on a
> child extent buffer corresponds to the key at a specific slot in the
> parent has a disadvantage: we effectively hit a BUG_ON() which requires
> rebooting the machine later. It also does not tell any information about
> which extent buffer is affected, from which root, the expected and found
> keys, etc.
> 
> However as of commit 581c1760415c48 ("btrfs: Validate child tree block's
> level and first key"), that assertion is not needed since at the time we
> read an extent buffer from disk we validate that its first key matches the
> key, at the respective slot, in the parent extent buffer. Therefore just
> remove the assertion at key_search().
> 
> Signed-off-by: Filipe Manana <fdmanana@suse.com>

Added to misc-next, thanks.