Re: [PATCH] btrfs: fix leak of qgroup extent records after transaction abort

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2024/6/4 08:58, Filipe Manana 写道:
> On Tue, Jun 4, 2024 at 12:21 AM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
[...]
>>
>> I'd prefer to call btrfs_qgroup_destory_extent_records() inside the "if
>> (atomic_read() == 0)" branch to be a little more easier to read.
>> But it's only a preference.
>
> Maybe it makes the diff more immediate to understand.
> But the final code is certainly less verbose and clear enough:
>
> while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) {
>      (...)
> }
> btrfs_qgroup_destroy_extent_records(trans);
>
> As opposed to:
>
> if (atomic_read(&delayed_refs->num_entries) == 0) {
>      btrfs_qgroup_destroy_extent_records(trans);
>      spin_unlock(&delayed_refs->lock);
>      return;
> }
>
> while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) {
>      (...)
> }
> btrfs_qgroup_destroy_extent_records(trans);
>
>
> More code for absolutely nothing, not even better readability.

It looks like I'm still tending to do empty check, other than let the
loop to  handle empty cases.

And indeed the extra empty check is only going to bloat the code without
much need, and I should change my tendency.

Forgot to give a reviewed by tag:

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
>
>
>>
>> Thanks,
>> Qu
>>
>>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>>
>>>>> Reported-by: syzbot+0fecc032fa134afd49df@syzkaller.appspotmail.com
>>>>> Link: https://lore.kernel.org/linux-btrfs/0000000000004e7f980619f91835@google.com/
>>>>> Signed-off-by: Filipe Manana <fdmanana@suse.com>
>>>>> ---
>>>>>     fs/btrfs/disk-io.c | 10 +---------
>>>>>     1 file changed, 1 insertion(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>>> index 8693893744a0..b1daaaec0614 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -4522,18 +4522,10 @@ static void btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
>>>>>                                        struct btrfs_fs_info *fs_info)
>>>>>     {
>>>>>         struct rb_node *node;
>>>>> -     struct btrfs_delayed_ref_root *delayed_refs;
>>>>> +     struct btrfs_delayed_ref_root *delayed_refs = &trans->delayed_refs;
>>>>>         struct btrfs_delayed_ref_node *ref;
>>>>>
>>>>> -     delayed_refs = &trans->delayed_refs;
>>>>> -
>>>>>         spin_lock(&delayed_refs->lock);
>>>>> -     if (atomic_read(&delayed_refs->num_entries) == 0) {
>>>>> -             spin_unlock(&delayed_refs->lock);
>>>>> -             btrfs_debug(fs_info, "delayed_refs has NO entry");
>>>>> -             return;
>>>>> -     }
>>>>> -
>>>>>         while ((node = rb_first_cached(&delayed_refs->href_root)) != NULL) {
>>>>>                 struct btrfs_delayed_ref_head *head;
>>>>>                 struct rb_node *n;
>