[3/3] Btrfs: fix missing inline refs when walking backrefs
diff mbox

Message ID 1389533914-2272-4-git-send-email-wangshilong1991@gmail.com
State Rejected
Headers show

Commit Message

Wang Shilong Jan. 12, 2014, 1:38 p.m. UTC
From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>

If @slot=0, we may have an expected item in the previous leaf,
So we should handle that case, otherwise, we will miss inline refs
,fix it.

Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
---
 fs/btrfs/backref.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

Comments

Filipe Manana Jan. 12, 2014, 3:36 p.m. UTC | #1
On Sun, Jan 12, 2014 at 1:38 PM, Wang Shilong <wangshilong1991@gmail.com> wrote:
> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>
> If @slot=0, we may have an expected item in the previous leaf,
> So we should handle that case, otherwise, we will miss inline refs
> ,fix it.

Hi Shilong.

How can this happen exactly?

So the search key, regardless of having an BTRFS_EXTENT_ITEM_KEY or  a
BTRFS_METADATA_ITEM_KEY type, always has an offset set to (u64)-1.
This means the btrfs_search_slot call will always return 1 (not found,
as expected) or an error.

And btrfs_search_slot, when it doesn't find a key and if that key
should be the first item in a leaf, it makes the path point to the
previous leaf with a "path->slots[0] ==
btrfs_header_nritems(path->nodes[0])" (see [1]), otherwise to some
value > 0. The only cases I can imagine getting "path->slots[0] == 0"
is if we hit the first, i.e. left most, leaf in the btree or if the
btree consists only of 1 leaf (no nodes) - in these cases there's
isn't a previous leaf.

Did I miss something here?

thanks

[1] - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.c?id=refs/tags/v3.13-rc8#n2629

>
> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
> ---
>  fs/btrfs/backref.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 964679c..4ccd726 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -893,14 +893,13 @@ again:
>                 spin_unlock(&delayed_refs->lock);
>         }
>
> -       if (path->slots[0]) {
> -               struct extent_buffer *leaf;
> -               int slot;
> +       ret = btrfs_previous_extent_item(fs_info->extent_root, path,
> +                                        key.objectid);
> +       if (ret < 0)
> +               goto out;
>
> -               path->slots[0]--;
> -               leaf = path->nodes[0];
> -               slot = path->slots[0];
> -               btrfs_item_key_to_cpu(leaf, &key, slot);
> +       if (ret == 0) {
> +               btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>                 if (key.objectid == bytenr &&
>                     (key.type == BTRFS_EXTENT_ITEM_KEY ||
>                      key.type == BTRFS_METADATA_ITEM_KEY)) {
> --
> 1.8.4
>
> --
> 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
Wang Shilong Jan. 13, 2014, 1:27 a.m. UTC | #2
Hi Filipe,

On 01/12/2014 11:36 PM, Filipe David Manana wrote:
> On Sun, Jan 12, 2014 at 1:38 PM, Wang Shilong <wangshilong1991@gmail.com> wrote:
>> From: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>>
>> If @slot=0, we may have an expected item in the previous leaf,
>> So we should handle that case, otherwise, we will miss inline refs
>> ,fix it.
> Hi Shilong.
>
> How can this happen exactly?
>
> So the search key, regardless of having an BTRFS_EXTENT_ITEM_KEY or  a
> BTRFS_METADATA_ITEM_KEY type, always has an offset set to (u64)-1.
> This means the btrfs_search_slot call will always return 1 (not found,
> as expected) or an error.
>
> And btrfs_search_slot, when it doesn't find a key and if that key
> should be the first item in a leaf, it makes the path point to the
> previous leaf with a "path->slots[0] ==
> btrfs_header_nritems(path->nodes[0])" (see [1]), otherwise to some
> value > 0. The only cases I can imagine getting "path->slots[0] == 0"
> is if we hit the first, i.e. left most, leaf in the btree or if the
> btree consists only of 1 leaf (no nodes) - in these cases there's
> isn't a previous leaf.
>
> Did I miss something here?
>
> thanks
>
> [1] - https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/fs/btrfs/ctree.c?id=refs/tags/v3.13-rc8#n2629
You are right, i was missing something when i made this patch.
Really thanks for you reviewing and correcting me!:-)

Thanks,
Wang
>
>> Signed-off-by: Wang Shilong <wangsl.fnst@cn.fujitsu.com>
>> ---
>>   fs/btrfs/backref.c | 13 ++++++-------
>>   1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
>> index 964679c..4ccd726 100644
>> --- a/fs/btrfs/backref.c
>> +++ b/fs/btrfs/backref.c
>> @@ -893,14 +893,13 @@ again:
>>                  spin_unlock(&delayed_refs->lock);
>>          }
>>
>> -       if (path->slots[0]) {
>> -               struct extent_buffer *leaf;
>> -               int slot;
>> +       ret = btrfs_previous_extent_item(fs_info->extent_root, path,
>> +                                        key.objectid);
>> +       if (ret < 0)
>> +               goto out;
>>
>> -               path->slots[0]--;
>> -               leaf = path->nodes[0];
>> -               slot = path->slots[0];
>> -               btrfs_item_key_to_cpu(leaf, &key, slot);
>> +       if (ret == 0) {
>> +               btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>                  if (key.objectid == bytenr &&
>>                      (key.type == BTRFS_EXTENT_ITEM_KEY ||
>>                       key.type == BTRFS_METADATA_ITEM_KEY)) {
>> --
>> 1.8.4
>>
>> --
>> 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

Patch
diff mbox

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 964679c..4ccd726 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -893,14 +893,13 @@  again:
 		spin_unlock(&delayed_refs->lock);
 	}
 
-	if (path->slots[0]) {
-		struct extent_buffer *leaf;
-		int slot;
+	ret = btrfs_previous_extent_item(fs_info->extent_root, path,
+					 key.objectid);
+	if (ret < 0)
+		goto out;
 
-		path->slots[0]--;
-		leaf = path->nodes[0];
-		slot = path->slots[0];
-		btrfs_item_key_to_cpu(leaf, &key, slot);
+	if (ret == 0) {
+		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 		if (key.objectid == bytenr &&
 		    (key.type == BTRFS_EXTENT_ITEM_KEY ||
 		     key.type == BTRFS_METADATA_ITEM_KEY)) {