| Message ID | 133258557ae4387d6a1d01bafa3e5214ca91228d.1582302545.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minor cleanups | expand |
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
On 2/22/20 12:31 AM, David Sterba wrote: > The tree_log_mutex and reloc_mutex locks are properly nested so we can > simplify error handling and add labels for them. This reduces line count > of the function. > > Signed-off-by: David Sterba <dsterba@suse.com> > --- > fs/btrfs/transaction.c | 57 +++++++++++++++--------------------------- > 1 file changed, 20 insertions(+), 37 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index fdfdfc426539..3610b6fec627 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > * core function of the snapshot creation. > */ > ret = create_pending_snapshots(trans); > - if (ret) { > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_reloc; > > /* > * We insert the dir indexes of the snapshots and update the inode > @@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > * the nodes and leaves. > */ > ret = btrfs_run_delayed_items(trans); > - if (ret) { > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_reloc; > > ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); > - if (ret) { > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_reloc; > > /* > * make sure none of the code above managed to slip in a > @@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > mutex_lock(&fs_info->tree_log_mutex); > > ret = commit_fs_roots(trans); > - if (ret) { > - mutex_unlock(&fs_info->tree_log_mutex); > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_reloc; goto unlock_tree_log; Otherwise looks good. When fixed this you may add. Reviewed-by: Anand Jain <anand.jain@oracle.com> Thanks, Anand > /* > * Since the transaction is done, we can apply the pending changes > @@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > * new delayed refs. Must handle them or qgroup can be wrong. > */ > ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); > - if (ret) { > - mutex_unlock(&fs_info->tree_log_mutex); > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_tree_log; > > /* > * Since fs roots are all committed, we can get a quite accurate > * new_roots. So let's do quota accounting. > */ > ret = btrfs_qgroup_account_extents(trans); > - if (ret < 0) { > - mutex_unlock(&fs_info->tree_log_mutex); > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret < 0) > + goto unlock_tree_log; > > ret = commit_cowonly_roots(trans); > - if (ret) { > - mutex_unlock(&fs_info->tree_log_mutex); > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > - } > + if (ret) > + goto unlock_tree_log; > > /* > * The tasks which save the space cache and inode cache may also > @@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > */ > if (TRANS_ABORTED(cur_trans)) { > ret = cur_trans->aborted; > - mutex_unlock(&fs_info->tree_log_mutex); > - mutex_unlock(&fs_info->reloc_mutex); > - goto scrub_continue; > + goto unlock_tree_log; > } > > btrfs_prepare_extent_commit(fs_info); > @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > if (ret) { > btrfs_handle_fs_error(fs_info, ret, > "Error while writing out transaction"); > - mutex_unlock(&fs_info->tree_log_mutex); > - goto scrub_continue; > + goto unlock_tree_log; > } > > ret = write_all_supers(fs_info, 0); > @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > > return ret; > > +unlock_tree_log: > + mutex_unlock(&fs_info->tree_log_mutex); > +unlock_reloc: > + mutex_unlock(&fs_info->reloc_mutex); > scrub_continue: > btrfs_scrub_continue(fs_info); > cleanup_transaction: >
On Fri, Feb 21, 2020 at 05:31:22PM +0100, David Sterba wrote: > btrfs_prepare_extent_commit(fs_info); > @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > if (ret) { > btrfs_handle_fs_error(fs_info, ret, > "Error while writing out transaction"); > - mutex_unlock(&fs_info->tree_log_mutex); > - goto scrub_continue; > + goto unlock_tree_log; Hm and this one is also wrong, in other cases that jump to unlock_tree_log the unlocking order is tree_log_mutex/reloc_mutex, while a few lines before there is unlock of reloc_mutex (while tree_log_mutex is still held). This means the unlocking order is reversed compared to the other cases and we can't jump to the label as this would lead to double unlock of reloc_mutex. So the above hunk must stay as is, with a comment. > } > > ret = write_all_supers(fs_info, 0); > @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > > return ret; > > +unlock_tree_log: > + mutex_unlock(&fs_info->tree_log_mutex); > +unlock_reloc: > + mutex_unlock(&fs_info->reloc_mutex); > scrub_continue: > btrfs_scrub_continue(fs_info); > cleanup_transaction: > -- > 2.25.0
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index fdfdfc426539..3610b6fec627 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -2194,10 +2194,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) * core function of the snapshot creation. */ ret = create_pending_snapshots(trans); - if (ret) { - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_reloc; /* * We insert the dir indexes of the snapshots and update the inode @@ -2210,16 +2208,12 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) * the nodes and leaves. */ ret = btrfs_run_delayed_items(trans); - if (ret) { - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_reloc; ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); - if (ret) { - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_reloc; /* * make sure none of the code above managed to slip in a @@ -2245,11 +2239,8 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) mutex_lock(&fs_info->tree_log_mutex); ret = commit_fs_roots(trans); - if (ret) { - mutex_unlock(&fs_info->tree_log_mutex); - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_reloc; /* * Since the transaction is done, we can apply the pending changes @@ -2267,29 +2258,20 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) * new delayed refs. Must handle them or qgroup can be wrong. */ ret = btrfs_run_delayed_refs(trans, (unsigned long)-1); - if (ret) { - mutex_unlock(&fs_info->tree_log_mutex); - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_tree_log; /* * Since fs roots are all committed, we can get a quite accurate * new_roots. So let's do quota accounting. */ ret = btrfs_qgroup_account_extents(trans); - if (ret < 0) { - mutex_unlock(&fs_info->tree_log_mutex); - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret < 0) + goto unlock_tree_log; ret = commit_cowonly_roots(trans); - if (ret) { - mutex_unlock(&fs_info->tree_log_mutex); - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; - } + if (ret) + goto unlock_tree_log; /* * The tasks which save the space cache and inode cache may also @@ -2297,9 +2279,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) */ if (TRANS_ABORTED(cur_trans)) { ret = cur_trans->aborted; - mutex_unlock(&fs_info->tree_log_mutex); - mutex_unlock(&fs_info->reloc_mutex); - goto scrub_continue; + goto unlock_tree_log; } btrfs_prepare_extent_commit(fs_info); @@ -2346,8 +2326,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) if (ret) { btrfs_handle_fs_error(fs_info, ret, "Error while writing out transaction"); - mutex_unlock(&fs_info->tree_log_mutex); - goto scrub_continue; + goto unlock_tree_log; } ret = write_all_supers(fs_info, 0); @@ -2394,6 +2373,10 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) return ret; +unlock_tree_log: + mutex_unlock(&fs_info->tree_log_mutex); +unlock_reloc: + mutex_unlock(&fs_info->reloc_mutex); scrub_continue: btrfs_scrub_continue(fs_info); cleanup_transaction:
The tree_log_mutex and reloc_mutex locks are properly nested so we can simplify error handling and add labels for them. This reduces line count of the function. Signed-off-by: David Sterba <dsterba@suse.com> --- fs/btrfs/transaction.c | 57 +++++++++++++++--------------------------- 1 file changed, 20 insertions(+), 37 deletions(-)