Re: [PATCH 5/7] btrfs: free pertrans at end of cleanup_transaction

[Boris Burkov <boris@bur.io>]

On Wed, Mar 27, 2024 at 08:46:39AM +1030, Qu Wenruo wrote:
> 
> 
> 在 2024/3/27 08:09, Boris Burkov 写道:
> > Some of the operations after the free might convert more pertrans
> > metadata. Do the freeing as late as possible to eliminate a source of
> > leaked pertrans metadata.
> > 
> > Helps with the pass rate of generic/269 and generic/475.
> > 
> > Signed-off-by: Boris Burkov <boris@bur.io>
> 
> Well, you can also move other fs level cleanup out of the
> btrfs_cleanup_one_transaction() call.
> (e.g. destory_delayed_inodes()).
> 
> For qgroup part, it looks fine to me as a precautious behavior.

Since the call isn't per transaction, do you think it just makes more
sense to call it once per cleanup not once per trans per cleanup?

Or would you rather I refactored it some other way?

> 
> Thanks,
> Qu
> 
> > ---
> >   fs/btrfs/disk-io.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index 3df5477d48a8..4d7893cc0d4e 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4850,8 +4850,6 @@ void btrfs_cleanup_one_transaction(struct btrfs_transaction *cur_trans,
> >   				     EXTENT_DIRTY);
> >   	btrfs_destroy_pinned_extent(fs_info, &cur_trans->pinned_extents);
> > -	btrfs_free_all_qgroup_pertrans(fs_info);
> > -
> >   	cur_trans->state =TRANS_STATE_COMPLETED;
> >   	wake_up(&cur_trans->commit_wait);
> >   }
> > @@ -4904,6 +4902,7 @@ static int btrfs_cleanup_transaction(struct btrfs_fs_info *fs_info)
> >   	btrfs_assert_delayed_root_empty(fs_info);
> >   	btrfs_destroy_all_delalloc_inodes(fs_info);
> >   	btrfs_drop_all_logs(fs_info);
> > +	btrfs_free_all_qgroup_pertrans(fs_info);
> >   	mutex_unlock(&fs_info->transaction_kthread_mutex);
> >   	return 0;