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

[Qu Wenruo <wqu@suse.com>]

在 2024/3/28 03:52, Boris Burkov 写道:
> 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?

Yes, just like what you did for btrfs_free_all_qgroup_pertrans().

> 
> Or would you rather I refactored it some other way?

Just an idea for future cleanups (moving all global cleanups out of 
btrfs_cleanup_one_transaction()).

Thanks,
Qu

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