| Message ID | 1362414341-17306-3-git-send-email-bo.li.liu@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Mar 05, 2013 at 12:25:37AM +0800, Liu Bo wrote: > We first use btrfs_std_error hook to replace with BUG_ON, and we > also need to cleanup what is left, including reloc roots rbtree > and reloc roots list. > Here we use a helper function to cleanup both rbtree and list, and > since this function can also be used in the balance recover path, > we also make the change as well to keep code simple. I've noticed that return value from merge_reloc_roots is never checked in the callers. Did you verify that this is ok? For example when called from btrfs_recover_relocation, is it possible that the reloc-to-be-recovered is still left unprocessed but the filesystem is going to be silently mounted read-write? The mount and remount callpaths check for recover_relocation errors and do not proceed otherwise. In RO mount, it's not called. So, either merge_reloc_roots callers should catch the errors or there are none by design (ie. the whole reloc operation is restartable). david -- 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
On Mon, Mar 18, 2013 at 03:16:12PM +0100, David Sterba wrote: > On Tue, Mar 05, 2013 at 12:25:37AM +0800, Liu Bo wrote: > > We first use btrfs_std_error hook to replace with BUG_ON, and we > > also need to cleanup what is left, including reloc roots rbtree > > and reloc roots list. > > Here we use a helper function to cleanup both rbtree and list, and > > since this function can also be used in the balance recover path, > > we also make the change as well to keep code simple. > > I've noticed that return value from merge_reloc_roots is never checked > in the callers. Did you verify that this is ok? Yeah, it's fine. Actually we set fs to RO once we get error here, as we have recorded a balance item and , balance can start over again the next time. > > For example when called from btrfs_recover_relocation, is it possible > that the reloc-to-be-recovered is still left unprocessed but the > filesystem is going to be silently mounted read-write? hmm, I'm not able to picture such a case... > > The mount and remount callpaths check for recover_relocation errors and > do not proceed otherwise. In RO mount, it's not called. > > So, either merge_reloc_roots callers should catch the errors or there > are none by design (ie. the whole reloc operation is restartable). > it's desigend to be always ready for a crash or a power off. thanks, liubo -- 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
On Tue, Mar 19, 2013 at 08:16:41PM +0800, Liu Bo wrote: > On Mon, Mar 18, 2013 at 03:16:12PM +0100, David Sterba wrote: > > I've noticed that return value from merge_reloc_roots is never checked > > in the callers. Did you verify that this is ok? > > Yeah, it's fine. Then it's ok to change return value to 'void' so it does not look like an unhandled errorcode. > Actually we set fs to RO once we get error here, as we have recorded a balance > item and , balance can start over again the next time. Yeah, if we get there via transaction abort. My background motivation is to implement a (much) more responsive balance cancel. This is different from the poweroff "cancel", because it does not keep any in-memory state. david -- 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 d9be73b..8608afb 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -2240,13 +2240,28 @@ again: } static noinline_for_stack +void free_reloc_roots(struct list_head *list) +{ + struct btrfs_root *reloc_root; + + while (!list_empty(list)) { + reloc_root = list_entry(list->next, struct btrfs_root, + root_list); + __update_reloc_root(reloc_root, 1); + free_extent_buffer(reloc_root->node); + free_extent_buffer(reloc_root->commit_root); + kfree(reloc_root); + } +} + +static noinline_for_stack int merge_reloc_roots(struct reloc_control *rc) { struct btrfs_root *root; struct btrfs_root *reloc_root; LIST_HEAD(reloc_roots); int found = 0; - int ret; + int ret = 0; again: root = rc->extent_root; @@ -2272,20 +2287,33 @@ again: BUG_ON(root->reloc_root != reloc_root); ret = merge_reloc_root(rc, root); - BUG_ON(ret); + if (ret) + goto out; } else { list_del_init(&reloc_root->root_list); } ret = btrfs_drop_snapshot(reloc_root, rc->block_rsv, 0, 1); - BUG_ON(ret < 0); + if (ret < 0) { + if (list_empty(&reloc_root->root_list)) + list_add_tail(&reloc_root->root_list, + &reloc_roots); + goto out; + } } if (found) { found = 0; goto again; } +out: + if (ret) { + btrfs_std_error(root->fs_info, ret); + if (!list_empty(&reloc_roots)) + free_reloc_roots(&reloc_roots); + } + BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root)); - return 0; + return ret; } static void free_block_list(struct rb_root *blocks) @@ -4266,14 +4294,9 @@ int btrfs_recover_relocation(struct btrfs_root *root) out_free: kfree(rc); out: - while (!list_empty(&reloc_roots)) { - reloc_root = list_entry(reloc_roots.next, - struct btrfs_root, root_list); - list_del(&reloc_root->root_list); - free_extent_buffer(reloc_root->node); - free_extent_buffer(reloc_root->commit_root); - kfree(reloc_root); - } + if (!list_empty(&reloc_roots)) + free_reloc_roots(&reloc_roots); + btrfs_free_path(path); if (err == 0) {
We first use btrfs_std_error hook to replace with BUG_ON, and we also need to cleanup what is left, including reloc roots rbtree and reloc roots list. Here we use a helper function to cleanup both rbtree and list, and since this function can also be used in the balance recover path, we also make the change as well to keep code simple. Signed-off-by: Liu Bo <bo.li.liu@oracle.com> --- fs/btrfs/relocation.c | 47 +++++++++++++++++++++++++++++++++++------------ 1 files changed, 35 insertions(+), 12 deletions(-)