| Message ID | 1395946611-6568-1-git-send-email-jbacik@fb.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Josef, 2014-03-28 2:56 GMT+08:00 Josef Bacik <jbacik@fb.com>: > This was done to allow NO_COW to continue to be NO_COW after relocation but it > is not right. When relocating we will convert blocks to FULL_BACKREF that we > relocate. We can leave some of these full backref blocks behind if they are not > cow'ed out during the relocation, like if we fail the relocation with ENOSPC and > then just drop the reloc tree. Then when we go to cow the block again we won't > lookup the extent flags because we won't think there has been a snapshot > recently which means we will do our normal ref drop thing instead of adding back > a tree ref and dropping the shared ref. This will cause btrfs_free_extent to > blow up because it can't find the ref we are trying to free. This was found > with my ref verifying tool. Thanks, Could we pass error into merge_reloc_roots() something like this: merge_reloc_roots(struct reloc_control *rc, int err) and we only skip reset root's last snapshot if @err is not 0. I think it is meaningful to allow NO_COW to continue after relocation. Thanks, Wang > > Signed-off-by: Josef Bacik <jbacik@fb.com> > --- > fs/btrfs/relocation.c | 21 --------------------- > 1 file changed, 21 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index ec00777..f026a82 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -2318,7 +2318,6 @@ void free_reloc_roots(struct list_head *list) > static noinline_for_stack > int merge_reloc_roots(struct reloc_control *rc) > { > - struct btrfs_trans_handle *trans; > struct btrfs_root *root; > struct btrfs_root *reloc_root; > u64 last_snap; > @@ -2376,26 +2375,6 @@ again: > list_add_tail(&reloc_root->root_list, > &reloc_roots); > goto out; > - } else if (!ret) { > - /* > - * recover the last snapshot tranid to avoid > - * the space balance break NOCOW. > - */ > - root = read_fs_root(rc->extent_root->fs_info, > - objectid); > - if (IS_ERR(root)) > - continue; > - > - trans = btrfs_join_transaction(root); > - BUG_ON(IS_ERR(trans)); > - > - /* Check if the fs/file tree was snapshoted or not. */ > - if (btrfs_root_last_snapshot(&root->root_item) == > - otransid - 1) > - btrfs_set_root_last_snapshot(&root->root_item, > - last_snap); > - > - btrfs_end_transaction(trans, root); > } > } > > -- > 1.8.3.1 > > -- > 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
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index ec00777..f026a82 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -2318,7 +2318,6 @@ void free_reloc_roots(struct list_head *list) static noinline_for_stack int merge_reloc_roots(struct reloc_control *rc) { - struct btrfs_trans_handle *trans; struct btrfs_root *root; struct btrfs_root *reloc_root; u64 last_snap; @@ -2376,26 +2375,6 @@ again: list_add_tail(&reloc_root->root_list, &reloc_roots); goto out; - } else if (!ret) { - /* - * recover the last snapshot tranid to avoid - * the space balance break NOCOW. - */ - root = read_fs_root(rc->extent_root->fs_info, - objectid); - if (IS_ERR(root)) - continue; - - trans = btrfs_join_transaction(root); - BUG_ON(IS_ERR(trans)); - - /* Check if the fs/file tree was snapshoted or not. */ - if (btrfs_root_last_snapshot(&root->root_item) == - otransid - 1) - btrfs_set_root_last_snapshot(&root->root_item, - last_snap); - - btrfs_end_transaction(trans, root); } }
This was done to allow NO_COW to continue to be NO_COW after relocation but it is not right. When relocating we will convert blocks to FULL_BACKREF that we relocate. We can leave some of these full backref blocks behind if they are not cow'ed out during the relocation, like if we fail the relocation with ENOSPC and then just drop the reloc tree. Then when we go to cow the block again we won't lookup the extent flags because we won't think there has been a snapshot recently which means we will do our normal ref drop thing instead of adding back a tree ref and dropping the shared ref. This will cause btrfs_free_extent to blow up because it can't find the ref we are trying to free. This was found with my ref verifying tool. Thanks, Signed-off-by: Josef Bacik <jbacik@fb.com> --- fs/btrfs/relocation.c | 21 --------------------- 1 file changed, 21 deletions(-)