Re: [PATCH 1/5] btrfs: drop log root for dropped roots

[David Sterba <dsterba@suse.cz>]

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.