| Message ID | 20200117141245.42971-1-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] btrfs: drop log root for dropped roots | expand |
On Fri, Jan 17, 2020 at 2:14 PM Josef Bacik <josef@toxicpanda.com> wrote: > > If we fsync on a subvolume and create a log root for that volume, and > then later delete that subvolume we'll never clean up its log root. Fix > this by making switch_commit_roots free the log for any dropped roots we > encounter. The extra churn is because we need a btrfs_trans_handle, not > the btrfs_transaction. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> Reviewed-by: Filipe Manana <fdmanana@suse.com> Had already reviewed it last year, and still looks right to me. Thanks. > --- > v1->v2: > - Update commit message to indicate we need the trans_handle instead of the > transaciton. > > fs/btrfs/transaction.c | 22 ++++++++++++---------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index cfc08ef9b876..55d8fd68775a 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -147,13 +147,14 @@ void btrfs_put_transaction(struct btrfs_transaction *transaction) > } > } > > -static noinline void switch_commit_roots(struct btrfs_transaction *trans) > +static noinline void switch_commit_roots(struct btrfs_trans_handle *trans) > { > + struct btrfs_transaction *cur_trans = trans->transaction; > struct btrfs_fs_info *fs_info = trans->fs_info; > struct btrfs_root *root, *tmp; > > down_write(&fs_info->commit_root_sem); > - list_for_each_entry_safe(root, tmp, &trans->switch_commits, > + list_for_each_entry_safe(root, tmp, &cur_trans->switch_commits, > dirty_list) { > list_del_init(&root->dirty_list); > free_extent_buffer(root->commit_root); > @@ -165,16 +166,17 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans) > } > > /* We can free old roots now. */ > - spin_lock(&trans->dropped_roots_lock); > - while (!list_empty(&trans->dropped_roots)) { > - root = list_first_entry(&trans->dropped_roots, > + spin_lock(&cur_trans->dropped_roots_lock); > + while (!list_empty(&cur_trans->dropped_roots)) { > + root = list_first_entry(&cur_trans->dropped_roots, > struct btrfs_root, root_list); > list_del_init(&root->root_list); > - spin_unlock(&trans->dropped_roots_lock); > + spin_unlock(&cur_trans->dropped_roots_lock); > + btrfs_free_log(trans, root); > btrfs_drop_and_free_fs_root(fs_info, root); > - spin_lock(&trans->dropped_roots_lock); > + spin_lock(&cur_trans->dropped_roots_lock); > } > - spin_unlock(&trans->dropped_roots_lock); > + spin_unlock(&cur_trans->dropped_roots_lock); > up_write(&fs_info->commit_root_sem); > } > > @@ -1421,7 +1423,7 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans, > ret = commit_cowonly_roots(trans); > if (ret) > goto out; > - switch_commit_roots(trans->transaction); > + switch_commit_roots(trans); > ret = btrfs_write_and_wait_transaction(trans); > if (ret) > btrfs_handle_fs_error(fs_info, ret, > @@ -2301,7 +2303,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) > list_add_tail(&fs_info->chunk_root->dirty_list, > &cur_trans->switch_commits); > > - switch_commit_roots(cur_trans); > + switch_commit_roots(trans); > > ASSERT(list_empty(&cur_trans->dirty_bgs)); > ASSERT(list_empty(&cur_trans->io_bgs)); > -- > 2.24.1 >
On Fri, Jan 17, 2020 at 09:12:45AM -0500, Josef Bacik wrote: > If we fsync on a subvolume and create a log root for that volume, and > then later delete that subvolume we'll never clean up its log root. Fix > this by making switch_commit_roots free the log for any dropped roots we > encounter. The extra churn is because we need a btrfs_trans_handle, not > the btrfs_transaction. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > v1->v2: > - Update commit message to indicate we need the trans_handle instead of the > transaciton. Thanks, added to misc-next.
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index cfc08ef9b876..55d8fd68775a 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -147,13 +147,14 @@ void btrfs_put_transaction(struct btrfs_transaction *transaction) } } -static noinline void switch_commit_roots(struct btrfs_transaction *trans) +static noinline void switch_commit_roots(struct btrfs_trans_handle *trans) { + struct btrfs_transaction *cur_trans = trans->transaction; struct btrfs_fs_info *fs_info = trans->fs_info; struct btrfs_root *root, *tmp; down_write(&fs_info->commit_root_sem); - list_for_each_entry_safe(root, tmp, &trans->switch_commits, + list_for_each_entry_safe(root, tmp, &cur_trans->switch_commits, dirty_list) { list_del_init(&root->dirty_list); free_extent_buffer(root->commit_root); @@ -165,16 +166,17 @@ static noinline void switch_commit_roots(struct btrfs_transaction *trans) } /* We can free old roots now. */ - spin_lock(&trans->dropped_roots_lock); - while (!list_empty(&trans->dropped_roots)) { - root = list_first_entry(&trans->dropped_roots, + spin_lock(&cur_trans->dropped_roots_lock); + while (!list_empty(&cur_trans->dropped_roots)) { + root = list_first_entry(&cur_trans->dropped_roots, struct btrfs_root, root_list); list_del_init(&root->root_list); - spin_unlock(&trans->dropped_roots_lock); + spin_unlock(&cur_trans->dropped_roots_lock); + btrfs_free_log(trans, root); btrfs_drop_and_free_fs_root(fs_info, root); - spin_lock(&trans->dropped_roots_lock); + spin_lock(&cur_trans->dropped_roots_lock); } - spin_unlock(&trans->dropped_roots_lock); + spin_unlock(&cur_trans->dropped_roots_lock); up_write(&fs_info->commit_root_sem); } @@ -1421,7 +1423,7 @@ static int qgroup_account_snapshot(struct btrfs_trans_handle *trans, ret = commit_cowonly_roots(trans); if (ret) goto out; - switch_commit_roots(trans->transaction); + switch_commit_roots(trans); ret = btrfs_write_and_wait_transaction(trans); if (ret) btrfs_handle_fs_error(fs_info, ret, @@ -2301,7 +2303,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans) list_add_tail(&fs_info->chunk_root->dirty_list, &cur_trans->switch_commits); - switch_commit_roots(cur_trans); + switch_commit_roots(trans); ASSERT(list_empty(&cur_trans->dirty_bgs)); ASSERT(list_empty(&cur_trans->io_bgs));
If we fsync on a subvolume and create a log root for that volume, and then later delete that subvolume we'll never clean up its log root. Fix this by making switch_commit_roots free the log for any dropped roots we encounter. The extra churn is because we need a btrfs_trans_handle, not the btrfs_transaction. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- v1->v2: - Update commit message to indicate we need the trans_handle instead of the transaciton. fs/btrfs/transaction.c | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-)