[1/5] btrfs: drop log root for dropped roots
diff mbox series

Message ID 20191206143718.167998-2-josef@toxicpanda.com
State New
Headers show
Series
  • Various fixes
Related show

Commit Message

Josef Bacik Dec. 6, 2019, 2:37 p.m. UTC
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.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/transaction.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

Comments

Filipe Manana Dec. 6, 2019, 3:02 p.m. UTC | #1
On Fri, Dec 6, 2019 at 2:38 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.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  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.23.0
>
Nikolay Borisov Dec. 6, 2019, 3:03 p.m. UTC | #2
On 6.12.19 г. 16:37 ч., 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.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  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);

THis patch should really have been this line and converting
switch_commit_roots to taking a trans handle another patch. Otherwise
this is lost in the mechanical refactoring.

>  		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));
>
David Sterba Dec. 9, 2019, 5:39 p.m. UTC | #3
On Fri, Dec 06, 2019 at 05:03:36PM +0200, Nikolay Borisov wrote:
> On 6.12.19 г. 16:37 ч., 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.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  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);
> 
> THis patch should really have been this line and converting
> switch_commit_roots to taking a trans handle another patch. Otherwise
> this is lost in the mechanical refactoring.

Agreed, as this is a bugfix, we want it backported to older threes so
minimizing the potential conflicts is desired. For that the order 1)
fix 2) cleanup, seems appropriate.
Josef Bacik Dec. 10, 2019, 8:05 p.m. UTC | #4
On 12/6/19 10:03 AM, Nikolay Borisov wrote:
> 
> 
> On 6.12.19 г. 16:37 ч., 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.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   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);
> 
> THis patch should really have been this line and converting
> switch_commit_roots to taking a trans handle another patch. Otherwise
> this is lost in the mechanical refactoring.
> 

We need the trans handle to even call btrfs_free_log, we're just fixing it so 
the trans handle can be passed in, making its separate is just superfluous.  Thanks,

Josef
Nikolay Borisov Dec. 10, 2019, 9:19 p.m. UTC | #5
On 10.12.19 г. 22:05 ч., Josef Bacik wrote:
> On 12/6/19 10:03 AM, Nikolay Borisov wrote:
>>
>>
>> On 6.12.19 г. 16:37 ч., 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.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>   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);
>>
>> THis patch should really have been this line and converting
>> switch_commit_roots to taking a trans handle another patch. Otherwise
>> this is lost in the mechanical refactoring.
>>
> 
> We need the trans handle to even call btrfs_free_log, we're just fixing
> it so the trans handle can be passed in, making its separate is just
> superfluous.  Thanks,

Actually no because callees handle the case when trans is not passed
(i.e. walk_log_tree and walk_(up|down)_log_tree. If passing valid
trances changes the call logic then this needs to be explained in the
changelog. And there is currently one caller calling that function
without a trans - btrfs_drop_and_free_fs_root in BTRFS_FS_STATE_ERROR case.

> 
> Josef
Josef Bacik Dec. 10, 2019, 9:28 p.m. UTC | #6
On 12/10/19 4:19 PM, Nikolay Borisov wrote:
> 
> 
> On 10.12.19 г. 22:05 ч., Josef Bacik wrote:
>> On 12/6/19 10:03 AM, Nikolay Borisov wrote:
>>>
>>>
>>> On 6.12.19 г. 16:37 ч., 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.
>>>>
>>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>>> ---
>>>>    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);
>>>
>>> THis patch should really have been this line and converting
>>> switch_commit_roots to taking a trans handle another patch. Otherwise
>>> this is lost in the mechanical refactoring.
>>>
>>
>> We need the trans handle to even call btrfs_free_log, we're just fixing
>> it so the trans handle can be passed in, making its separate is just
>> superfluous.  Thanks,
> 
> Actually no because callees handle the case when trans is not passed
> (i.e. walk_log_tree and walk_(up|down)_log_tree. If passing valid
> trances changes the call logic then this needs to be explained in the
> changelog. And there is currently one caller calling that function
> without a trans - btrfs_drop_and_free_fs_root in BTRFS_FS_STATE_ERROR case.
> 

Yeah, it's clear that NULL is used only in the error case.  I'm not going to 
explain the entirety of how the log tree works in a basic fix for not freeing up 
a tree log when we should be doing it.  Thanks,

Josef
David Sterba Dec. 13, 2019, 3:17 p.m. UTC | #7
On Tue, Dec 10, 2019 at 04:28:17PM -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.
> >>>> +        btrfs_free_log(trans, root);
> >>>
> >>> THis patch should really have been this line and converting
> >>> switch_commit_roots to taking a trans handle another patch. Otherwise
> >>> this is lost in the mechanical refactoring.
> >>
> >> We need the trans handle to even call btrfs_free_log, we're just fixing
> >> it so the trans handle can be passed in, making its separate is just
> >> superfluous.  Thanks,
> > 
> > Actually no because callees handle the case when trans is not passed
> > (i.e. walk_log_tree and walk_(up|down)_log_tree. If passing valid
> > trances changes the call logic then this needs to be explained in the
> > changelog. And there is currently one caller calling that function
> > without a trans - btrfs_drop_and_free_fs_root in BTRFS_FS_STATE_ERROR case.
> 
> Yeah, it's clear that NULL is used only in the error case.  I'm not going to 
> explain the entirety of how the log tree works in a basic fix for not freeing up 
> a tree log when we should be doing it.  Thanks,

Sure you can at least note that the parameter type needs to be changed,
so we don't have to spend too much time looking for the real fix.

Patch
diff mbox series

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));