| Message ID | 1443163381-12332-1-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
On Fri, Sep 25, 2015 at 02:43:01PM +0800, Anand Jain wrote: > btrfs_error() and btrfs_std_error() does the same thing > and calls _btrfs_std_error(), so consolidate them together. > And the main motivation is that btrfs_error() is closely > named with btrfs_err(), one handles error action the other > is to log the error, so don't closely name them. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > Suggested-by: David Sterba <dsterba@suse.com> Reviewed-by: David Sterba <dsterba@suse.com> I guess we can live with the extra NULL argument, in some cases it does not make sense to put a string there. > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -4852,7 +4852,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) > /* update qgroup status and info */ > err = btrfs_run_qgroups(trans, root->fs_info); > if (err < 0) > - btrfs_error(root->fs_info, ret, > + btrfs_std_error(root->fs_info, ret, This looks like a bug, ret instead of err. The value of 'ret' is set by add/del qgroup relation which might fail if the relations are there, but we do not care. We're likely interested in the return code of btrfs_run_qgroups, ie. err. Can you please send a new patch on top of this? > "failed to update qgroup status and info\n"); > err = btrfs_end_transaction(trans, root); > if (err && !ret) -- 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 09/25/2015 06:31 PM, David Sterba wrote: > On Fri, Sep 25, 2015 at 02:43:01PM +0800, Anand Jain wrote: >> btrfs_error() and btrfs_std_error() does the same thing >> and calls _btrfs_std_error(), so consolidate them together. >> And the main motivation is that btrfs_error() is closely >> named with btrfs_err(), one handles error action the other >> is to log the error, so don't closely name them. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> Suggested-by: David Sterba <dsterba@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > > I guess we can live with the extra NULL argument, in some cases it does > not make sense to put a string there. > >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -4852,7 +4852,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) >> /* update qgroup status and info */ >> err = btrfs_run_qgroups(trans, root->fs_info); >> if (err < 0) >> - btrfs_error(root->fs_info, ret, >> + btrfs_std_error(root->fs_info, ret, > > This looks like a bug, ret instead of err. The value of 'ret' is set by > add/del qgroup relation which might fail if the relations are there, but > we do not care. We're likely interested in the return code of > btrfs_run_qgroups, ie. err. Can you please send a new patch on top of this? I think the original code intended to log the error (btrfs_err()) instead of handle the error (btrfs_error()->btrfs_std_error()). Qu, Any idea ? Thanks, Anand >> "failed to update qgroup status and info\n"); >> err = btrfs_end_transaction(trans, root); >> if (err && !ret) > -- > 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
? 2015?10?02? 15:41, Anand Jain ??: > > > On 09/25/2015 06:31 PM, David Sterba wrote: >> On Fri, Sep 25, 2015 at 02:43:01PM +0800, Anand Jain wrote: >>> btrfs_error() and btrfs_std_error() does the same thing >>> and calls _btrfs_std_error(), so consolidate them together. >>> And the main motivation is that btrfs_error() is closely >>> named with btrfs_err(), one handles error action the other >>> is to log the error, so don't closely name them. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> Suggested-by: David Sterba <dsterba@suse.com> >> >> Reviewed-by: David Sterba <dsterba@suse.com> >> >> I guess we can live with the extra NULL argument, in some cases it does >> not make sense to put a string there. >> >>> --- a/fs/btrfs/ioctl.c >>> +++ b/fs/btrfs/ioctl.c >>> @@ -4852,7 +4852,7 @@ static long btrfs_ioctl_qgroup_assign(struct >>> file *file, void __user *arg) >>> /* update qgroup status and info */ >>> err = btrfs_run_qgroups(trans, root->fs_info); >>> if (err < 0) >>> - btrfs_error(root->fs_info, ret, >>> + btrfs_std_error(root->fs_info, ret, >> >> This looks like a bug, ret instead of err. The value of 'ret' is set by >> add/del qgroup relation which might fail if the relations are there, but >> we do not care. We're likely interested in the return code of >> btrfs_run_qgroups, ie. err. Can you please send a new patch on top of >> this? > > > I think the original code intended to log the error (btrfs_err()) > instead of handle the error (btrfs_error()->btrfs_std_error()). > > Qu, Any idea ? > > Thanks, Anand > David is right, that's a bug. We lose the return value of add/del_qgroup_relation(). Just as David mentioned, add/del_qgroup_relation() does a lot of validation check, so we'd better log an error before we run qgroups. But we still need to call btrfs_run_qgroups() to mark INCONSISTENT flag for qgroup tree. And for your patch, it may be my personal preference, but the btrfs_std_error() naming is quite confusing for me. Std_error() means more like stderr, for my first glance, I'd think it's just a new printk() warpper, until I checked the code. It does more than printk, but also set FS_STATE_ERROR bit and set fs to readonly. I'd like it to be something like btrfs_handle_err(). So, in the new patch(es) we may need to do the following things: 1) Add new log for btrfs_add/del_relation(). It only needs to log an error, no need to mark FS_ERROR bit. As it may just be an invalid parameter. 2) Handle the err returned from btrfs_run_qgroups() If btrfs_run_qgroups() return error, that's a BIG problem. Which means we failed to update not only qgroup accounting info, but also qgroup status info.(including failed to mark INCONSISTENT) So we need to set FS_STATE_ERROR bit. Thanks, Qu > >>> "failed to update qgroup status and info\n"); >>> err = btrfs_end_transaction(trans, root); >>> if (err && !ret) >> -- >> 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 -- 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
> It only needs to log an error, no need to mark FS_ERROR bit. > As it may just be an invalid parameter. This ans-ed my question. Then there are two bugs. one which David pointed out. two In original code (before this patch), it should have been calling btrfs_err() instead of btrfs_error(). Thanks, Anand -- 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
> btrfs_std_error() naming is quite confusing for me. > > Std_error() means more like stderr, for my first glance, I'd think > it's just a new printk() warpper, until I checked the code. > It does more than printk, but also set FS_STATE_ERROR bit and set fs > to readonly. > I'd like it to be something like btrfs_handle_err(). David, I agree with Qu comment on this patch. I am ok change btrfs_std_error to btrfs_handle_error() with existing btrfs_handle_error() be renamed to __handle_error() If you agree as well, do you want a patch on top of it or a replacement patch will do. ? Thanks, Anand On 09/25/2015 06:31 PM, David Sterba wrote: > On Fri, Sep 25, 2015 at 02:43:01PM +0800, Anand Jain wrote: >> btrfs_error() and btrfs_std_error() does the same thing >> and calls _btrfs_std_error(), so consolidate them together. >> And the main motivation is that btrfs_error() is closely >> named with btrfs_err(), one handles error action the other >> is to log the error, so don't closely name them. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> Suggested-by: David Sterba <dsterba@suse.com> > > Reviewed-by: David Sterba <dsterba@suse.com> > > I guess we can live with the extra NULL argument, in some cases it does > not make sense to put a string there. > >> --- a/fs/btrfs/ioctl.c >> +++ b/fs/btrfs/ioctl.c >> @@ -4852,7 +4852,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) >> /* update qgroup status and info */ >> err = btrfs_run_qgroups(trans, root->fs_info); >> if (err < 0) >> - btrfs_error(root->fs_info, ret, >> + btrfs_std_error(root->fs_info, ret, > > This looks like a bug, ret instead of err. The value of 'ret' is set by > add/del qgroup relation which might fail if the relations are there, but > we do not care. We're likely interested in the return code of > btrfs_run_qgroups, ie. err. Can you please send a new patch on top of this? > >> "failed to update qgroup status and info\n"); >> err = btrfs_end_transaction(trans, root); >> if (err && !ret) > -- > 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/ctree.c b/fs/btrfs/ctree.c index 5f745ea..1063315 100644 --- a/fs/btrfs/ctree.c +++ b/fs/btrfs/ctree.c @@ -1011,7 +1011,7 @@ static noinline int update_ref_for_cow(struct btrfs_trans_handle *trans, return ret; if (refs == 0) { ret = -EROFS; - btrfs_std_error(root->fs_info, ret); + btrfs_std_error(root->fs_info, ret, NULL); return ret; } } else { @@ -1927,7 +1927,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, child = read_node_slot(root, mid, 0); if (!child) { ret = -EROFS; - btrfs_std_error(root->fs_info, ret); + btrfs_std_error(root->fs_info, ret, NULL); goto enospc; } @@ -2030,7 +2030,7 @@ static noinline int balance_level(struct btrfs_trans_handle *trans, */ if (!left) { ret = -EROFS; - btrfs_std_error(root->fs_info, ret); + btrfs_std_error(root->fs_info, ret, NULL); goto enospc; } wret = balance_node_right(trans, root, mid, left); diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index 4484063..a86051e 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -4127,14 +4127,7 @@ do { \ __LINE__, (errno)); \ } while (0) -#define btrfs_std_error(fs_info, errno) \ -do { \ - if ((errno)) \ - __btrfs_std_error((fs_info), __func__, \ - __LINE__, (errno), NULL); \ -} while (0) - -#define btrfs_error(fs_info, errno, fmt, args...) \ +#define btrfs_std_error(fs_info, errno, fmt, args...) \ do { \ __btrfs_std_error((fs_info), __func__, __LINE__, \ (errno), fmt, ##args); \ diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index e0dbe41..18796c9 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2377,7 +2377,7 @@ static int btrfs_replay_log(struct btrfs_fs_info *fs_info, /* returns with log_tree_root freed on success */ ret = btrfs_recover_log_trees(log_tree_root); if (ret) { - btrfs_error(tree_root->fs_info, ret, + btrfs_std_error(tree_root->fs_info, ret, "Failed to recover log tree"); free_extent_buffer(log_tree_root->node); kfree(log_tree_root); @@ -3570,7 +3570,7 @@ static int write_all_supers(struct btrfs_root *root, int max_mirrors) if (ret) { mutex_unlock( &root->fs_info->fs_devices->device_list_mutex); - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "errors while submitting device barriers."); return ret; } @@ -3610,7 +3610,7 @@ static int write_all_supers(struct btrfs_root *root, int max_mirrors) mutex_unlock(&root->fs_info->fs_devices->device_list_mutex); /* FUA is masked off if unsupported and can't be the reason */ - btrfs_error(root->fs_info, -EIO, + btrfs_std_error(root->fs_info, -EIO, "%d errors while writing supers", total_errors); return -EIO; } @@ -3628,7 +3628,7 @@ static int write_all_supers(struct btrfs_root *root, int max_mirrors) } mutex_unlock(&root->fs_info->fs_devices->device_list_mutex); if (total_errors > max_errors) { - btrfs_error(root->fs_info, -EIO, + btrfs_std_error(root->fs_info, -EIO, "%d errors while writing supers", total_errors); return -EIO; } diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c index 5411f0a..2d13e08 100644 --- a/fs/btrfs/extent-tree.c +++ b/fs/btrfs/extent-tree.c @@ -8691,7 +8691,7 @@ out: if (!for_reloc && root_dropped == false) btrfs_add_dead_root(root); if (err && err != -EAGAIN) - btrfs_std_error(root->fs_info, err); + btrfs_std_error(root->fs_info, err, NULL); return err; } diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c index 265e03c..be4d22a 100644 --- a/fs/btrfs/inode-item.c +++ b/fs/btrfs/inode-item.c @@ -157,7 +157,7 @@ static int btrfs_del_inode_extref(struct btrfs_trans_handle *trans, */ if (!btrfs_find_name_in_ext_backref(path, ref_objectid, name, name_len, &extref)) { - btrfs_std_error(root->fs_info, -ENOENT); + btrfs_std_error(root->fs_info, -ENOENT, NULL); ret = -EROFS; goto out; } diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 6c9e58c..f7108c8 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -4852,7 +4852,7 @@ static long btrfs_ioctl_qgroup_assign(struct file *file, void __user *arg) /* update qgroup status and info */ err = btrfs_run_qgroups(trans, root->fs_info); if (err < 0) - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "failed to update qgroup status and info\n"); err = btrfs_end_transaction(trans, root); if (err && !ret) diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index 303babe..58ede0a 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -2418,7 +2418,7 @@ again: } out: if (ret) { - btrfs_std_error(root->fs_info, ret); + btrfs_std_error(root->fs_info, ret, NULL); if (!list_empty(&reloc_roots)) free_reloc_roots(&reloc_roots); diff --git a/fs/btrfs/root-tree.c b/fs/btrfs/root-tree.c index 360a728..6d90851 100644 --- a/fs/btrfs/root-tree.c +++ b/fs/btrfs/root-tree.c @@ -283,7 +283,7 @@ int btrfs_find_orphan_roots(struct btrfs_root *tree_root) trans = btrfs_join_transaction(tree_root); if (IS_ERR(trans)) { err = PTR_ERR(trans); - btrfs_error(tree_root->fs_info, err, + btrfs_std_error(tree_root->fs_info, err, "Failed to start trans to delete " "orphan item"); break; @@ -292,7 +292,7 @@ int btrfs_find_orphan_roots(struct btrfs_root *tree_root) root_key.objectid); btrfs_end_transaction(trans, tree_root); if (err) { - btrfs_error(tree_root->fs_info, err, + btrfs_std_error(tree_root->fs_info, err, "Failed to delete root orphan " "item"); break; diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index 8f259b3..86a8759 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -2103,7 +2103,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans, ret = btrfs_write_and_wait_transaction(trans, root); if (ret) { - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "Error while writing out transaction"); mutex_unlock(&root->fs_info->tree_log_mutex); goto scrub_continue; diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c index 1bbaace..c3f9a9c 100644 --- a/fs/btrfs/tree-log.c +++ b/fs/btrfs/tree-log.c @@ -5314,7 +5314,7 @@ int btrfs_recover_log_trees(struct btrfs_root *log_root_tree) ret = walk_log_tree(trans, log_root_tree, &wc); if (ret) { - btrfs_error(fs_info, ret, "Failed to pin buffers while " + btrfs_std_error(fs_info, ret, "Failed to pin buffers while " "recovering log root tree."); goto error; } @@ -5328,7 +5328,7 @@ again: ret = btrfs_search_slot(NULL, log_root_tree, &key, path, 0, 0); if (ret < 0) { - btrfs_error(fs_info, ret, + btrfs_std_error(fs_info, ret, "Couldn't find tree log root."); goto error; } @@ -5346,7 +5346,7 @@ again: log = btrfs_read_fs_root(log_root_tree, &found_key); if (IS_ERR(log)) { ret = PTR_ERR(log); - btrfs_error(fs_info, ret, + btrfs_std_error(fs_info, ret, "Couldn't read tree log root."); goto error; } @@ -5361,7 +5361,7 @@ again: free_extent_buffer(log->node); free_extent_buffer(log->commit_root); kfree(log); - btrfs_error(fs_info, ret, "Couldn't read target root " + btrfs_std_error(fs_info, ret, "Couldn't read target root " "for tree log recovery."); goto error; } diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index 9e7a73d..8752a4b 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1411,7 +1411,7 @@ again: extent = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dev_extent); } else { - btrfs_error(root->fs_info, ret, "Slot search failed"); + btrfs_std_error(root->fs_info, ret, "Slot search failed"); goto out; } @@ -1419,7 +1419,7 @@ again: ret = btrfs_del_item(trans, root, path); if (ret) { - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "Failed to remove dev extent item"); } else { trans->transaction->have_free_bgs = 1; @@ -2331,7 +2331,7 @@ int btrfs_init_new_device(struct btrfs_root *root, char *device_path) ret = btrfs_relocate_sys_chunks(root); if (ret < 0) - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "Failed to relocate sys chunks after " "device initialization. This can be fixed " "using the \"btrfs balance\" command."); @@ -2576,7 +2576,7 @@ static int btrfs_free_chunk(struct btrfs_trans_handle *trans, if (ret < 0) goto out; else if (ret > 0) { /* Logic error or corruption */ - btrfs_error(root->fs_info, -ENOENT, + btrfs_std_error(root->fs_info, -ENOENT, "Failed lookup while freeing chunk."); ret = -ENOENT; goto out; @@ -2584,7 +2584,7 @@ static int btrfs_free_chunk(struct btrfs_trans_handle *trans, ret = btrfs_del_item(trans, root, path); if (ret < 0) - btrfs_error(root->fs_info, ret, + btrfs_std_error(root->fs_info, ret, "Failed to delete chunk item."); out: btrfs_free_path(path); @@ -2769,7 +2769,7 @@ static int btrfs_relocate_chunk(struct btrfs_root *root, u64 chunk_offset) trans = btrfs_start_transaction(root, 0); if (IS_ERR(trans)) { ret = PTR_ERR(trans); - btrfs_std_error(root->fs_info, ret); + btrfs_std_error(root->fs_info, ret, NULL); return ret; } @@ -3424,7 +3424,7 @@ static void __cancel_balance(struct btrfs_fs_info *fs_info) unset_balance_control(fs_info); ret = del_balance_item(fs_info->tree_root); if (ret) - btrfs_std_error(fs_info, ret); + btrfs_std_error(fs_info, ret, NULL); atomic_set(&fs_info->mutually_exclusive_operation_running, 0); }
btrfs_error() and btrfs_std_error() does the same thing and calls _btrfs_std_error(), so consolidate them together. And the main motivation is that btrfs_error() is closely named with btrfs_err(), one handles error action the other is to log the error, so don't closely name them. Signed-off-by: Anand Jain <anand.jain@oracle.com> Suggested-by: David Sterba <dsterba@suse.com> --- fs/btrfs/ctree.c | 6 +++--- fs/btrfs/ctree.h | 9 +-------- fs/btrfs/disk-io.c | 8 ++++---- fs/btrfs/extent-tree.c | 2 +- fs/btrfs/inode-item.c | 2 +- fs/btrfs/ioctl.c | 2 +- fs/btrfs/relocation.c | 2 +- fs/btrfs/root-tree.c | 4 ++-- fs/btrfs/transaction.c | 2 +- fs/btrfs/tree-log.c | 8 ++++---- fs/btrfs/volumes.c | 14 +++++++------- 11 files changed, 26 insertions(+), 33 deletions(-)