btrfs-progs: initial reference count of extent buffer is correct?

btrfs-progs: initial reference count of extent buffer is correct?

[Naohiro Aota]

Hi, list

I'm having trouble with my btrfs FS recently and running btrfs check to
try to fix the FS. Unfortunately, it aborted with:

btrfsck: root-tree.c:81: btrfs_update_root: Assertion `!(ret != 0)' failed.

It means that "extent tree root" is not found in "tree root tree"! Then
I added btrfs_print_leaf() there to see what is happening there. There
were "(... METADATA_ITEM 0)" keys listed. Well, I found "tree root
tree"'s root extent buffer is somewhat replaced by a extent buffer from
the extent tree.

Reading the code, it seems that free_some_buffers() reclaim extent
buffers allocated to root trees because they are not
extent_buffer_get()ed (i.e. @refs == 1). 

To reproduce this problem, try running this code. This program first
print root tree node's bytenr, and scan some trees. If your FS is large
enough to run free_some_buffers(), tree root node's bytenr after the
scan would be different.

#include <stdio.h>
#include "ctree.h"
#include "disk-io.h"

void scan_tree(struct btrfs_root *root, struct extent_buffer *eb)
{
  u32 i;
  u32 nr;
  nr = btrfs_header_nritems(eb);
  if (btrfs_is_leaf(eb)) return;
  u32 size = btrfs_level_size(root, btrfs_header_level(eb) - 1);
  for (i = 0; i < nr; i++) {
    if (btrfs_is_leaf(eb)) return;
    u64 bytenr = btrfs_node_blockptr(eb, i);
    struct extent_buffer *next = read_tree_block(root, bytenr, size,
						 btrfs_node_ptr_generation(eb, i));
    if (!next) continue;
    scan_tree(root, next);
  }
}

int main(int ac, char **av)
{
  struct btrfs_fs_info *info;
  struct btrfs_root *root;
  info = open_ctree_fs_info(av[1], 0, 0, OPEN_CTREE_PARTIAL);
  root = info->fs_root;
  printf("tree root %lld\n", info->tree_root->node->start);
  scan_tree(info->fs_root, info->extent_root->node);
  scan_tree(info->fs_root, info->csum_root->node);
  scan_tree(info->fs_root, info->fs_root->node);
  printf("tree root %lld\n", info->tree_root->node->start);
  return close_ctree(root);
}

On my environment, the above code print the following result. "Tree root
tree variable" is eventually pointing to another extent!

$ ./btrfs-reproduce /dev/sda3
tree root 91393835008
tree root 49102848

I found commit 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 changed the
initial @refs to 1, stating that "we don't give enough
free_extent_buffer() to reduce the eb's references to zero so that the
eb can finally be freed", but I don't think this is correct. Even if
initial @refs == 2, one free_extent_buffer() would make the @refs to 1
and so let it reclaimed by free_some_buffer(), so it does not seems to
be a problem for me...

I think there are some collides how to use extent buffer: should
__alloc_extent_buffer set @refs = 2 for the caller or should the code
call extent_buffer_get() by themselves everywhere you allocate eb before
any other eb allocation not to let the first eb reclaimed? How to fix
this problem? revert 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 is the
collect way? or add "missing" extent_buffer_get() everywhere allocating
is done?

Naohiro

Re: btrfs-progs: initial reference count of extent buffer is correct?

[Liu Bo]

On Mon, Aug 25, 2014 at 02:26:49PM +0900, Naohiro Aota wrote:
> Hi, list
> 
> I'm having trouble with my btrfs FS recently and running btrfs check to
> try to fix the FS. Unfortunately, it aborted with:
> 
> btrfsck: root-tree.c:81: btrfs_update_root: Assertion `!(ret != 0)' failed.
> 
> It means that "extent tree root" is not found in "tree root tree"! Then
> I added btrfs_print_leaf() there to see what is happening there. There
> were "(... METADATA_ITEM 0)" keys listed. Well, I found "tree root
> tree"'s root extent buffer is somewhat replaced by a extent buffer from
> the extent tree.
> 
> Reading the code, it seems that free_some_buffers() reclaim extent
> buffers allocated to root trees because they are not
> extent_buffer_get()ed (i.e. @refs == 1). 
> 
> To reproduce this problem, try running this code. This program first
> print root tree node's bytenr, and scan some trees. If your FS is large
> enough to run free_some_buffers(), tree root node's bytenr after the
> scan would be different.
> 
> #include <stdio.h>
> #include "ctree.h"
> #include "disk-io.h"
> 
> void scan_tree(struct btrfs_root *root, struct extent_buffer *eb)
> {
>   u32 i;
>   u32 nr;
>   nr = btrfs_header_nritems(eb);
>   if (btrfs_is_leaf(eb)) return;
>   u32 size = btrfs_level_size(root, btrfs_header_level(eb) - 1);
>   for (i = 0; i < nr; i++) {
>     if (btrfs_is_leaf(eb)) return;
>     u64 bytenr = btrfs_node_blockptr(eb, i);
>     struct extent_buffer *next = read_tree_block(root, bytenr, size,
> 						 btrfs_node_ptr_generation(eb, i));
>     if (!next) continue;
>     scan_tree(root, next);
>   }
> }
> 
> int main(int ac, char **av)
> {
>   struct btrfs_fs_info *info;
>   struct btrfs_root *root;
>   info = open_ctree_fs_info(av[1], 0, 0, OPEN_CTREE_PARTIAL);
>   root = info->fs_root;
>   printf("tree root %lld\n", info->tree_root->node->start);
>   scan_tree(info->fs_root, info->extent_root->node);
>   scan_tree(info->fs_root, info->csum_root->node);
>   scan_tree(info->fs_root, info->fs_root->node);
>   printf("tree root %lld\n", info->tree_root->node->start);
>   return close_ctree(root);
> }
> 
> On my environment, the above code print the following result. "Tree root
> tree variable" is eventually pointing to another extent!
> 
> $ ./btrfs-reproduce /dev/sda3
> tree root 91393835008
> tree root 49102848
> 
> I found commit 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 changed the
> initial @refs to 1, stating that "we don't give enough
> free_extent_buffer() to reduce the eb's references to zero so that the
> eb can finally be freed", but I don't think this is correct. Even if
> initial @refs == 2, one free_extent_buffer() would make the @refs to 1
> and so let it reclaimed by free_some_buffer(), so it does not seems to
> be a problem for me...
> 
> I think there are some collides how to use extent buffer: should
> __alloc_extent_buffer set @refs = 2 for the caller or should the code
> call extent_buffer_get() by themselves everywhere you allocate eb before
> any other eb allocation not to let the first eb reclaimed? How to fix
> this problem? revert 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 is the
> collect way? or add "missing" extent_buffer_get() everywhere allocating
> is done?

You may think of it twice, commit 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 
is to fix a bug of assigning a free block to two different extent buffers, ie.
two different extent buffers' share the same eb->start, so it's not just bumping
a reference cnt.

Right now we want to be consistent with the kernel side, decreasing eb->refs=0
means it'd be freed, so droping free_some_buffer() can be a good choice.

And for caching extent buffer, we've increased eb->refs by 1 to keep it in the
cache rbtree.

thanks,
-liubo

Re: btrfs-progs: initial reference count of extent buffer is correct?

[naota]

Hi, Liu

Thank you for your explanation, and I'm sorry for this long silence.

Liu Bo <bo.li.liu@oracle.com> writes:

> You may think of it twice, commit 53ee1bccf99cd5b474fe1aa857b7dd176e3a1407 
> is to fix a bug of assigning a free block to two different extent buffers, ie.
> two different extent buffers' share the same eb->start, so it's not just bumping
> a reference cnt.
>
> Right now we want to be consistent with the kernel side, decreasing eb->refs=0
> means it'd be freed, so droping free_some_buffer() can be a good choice.

Now I understand the reason why @refs = 1 initially.

I'll post a patch to drop free_some_buffer() after this.

> And for caching extent buffer, we've increased eb->refs by 1 to keep it in the
> cache rbtree.
>
> thanks,
> -liubo

Regards,
Naohiro