Re: [PATCH] btrfs: retry flushing for del_balance_item() if the transaction is interrupted

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

On 2023/8/17 15:39, Filipe Manana wrote:
> On Wed, Aug 16, 2023 at 10:54 PM Qu Wenruo <quwenruo.btrfs@gmx.com> wrote:
[...]
>>>
>>> My only concern is if this can turn into an infinite loop due to a high enough rate of
>>> signals being sent to the process...
>>
>> Yep, that's indeed a concern.
>>
>> The other solution is to introduce a flag to disallow signal for the
>> ticket system (aka non-killable wait), which can get rid of the frequent
>> signal problems.
>>
>> In fact, we may not want certain reclaim to be interrupted at all,
>> especially for BTRFS_RESERVE_FLUSH_ALL_STEAL, which are only utilized
>> for very critical operations like unlink and other deletion operations.
>
> We shouldn't need to call
> btrfs_start_transaction_fallback_global_rsv() because we don't need to
> reserve space.
> A join transaction would be enough, because:
>
> 1) We pass 0 items to the start transaction call;
>
> 2) More importantly, we are updating the root tree and the root tree
> uses the global block reserve, see btrfs_init_root_block_rsv().
>
> So the start transaction call should not be reserving any space
> because "num_items == 0" and "flush != BTRFS_RESERVE_FLUSH_ALL".

My bad, this is caused by the difference between SLE and the upstream.

For upstream, we go BTRFS_RESERVE_FLUSH_ALL_STEAL for this call site
(btrfs_start_transaction_fallback_global_rsv()), but for the SLE kernel
it's just a plain btrfs_start_transaction() which is going to need to do
block rsv.

I should just backport the patch changing the start transaction helper.

And indeed your analyse is correct, for upstream kernel we won't flush
for btrfs_start_transaction_fallback_global_rsv(), thus we don't need to
handle it at all.

> Are you sure the -EINTR is coming from the
> btrfs_start_transaction_fallback_global_rsv() and not from the
> btrfs_search_slot() call at del_balance_item() for example?

Just to fill my curiosity, I checked the tree read code which waits
using UNINTERRUPTIBLE, but it's no longer relevant anymore...

Thanks for pointing out the hole!
Qu

>
> Nothing in the partial log you pasted can tell the -EINTR comes from
> btrfs_start_transaction_fallback_global_rsv(), which would be very
> surprising
> because it's not reserving any space for those reasons mentioned above.
>
>>
>>>
>>> Instead of this I would make reset_balance_state() just print a warning, and not
>>> call btrfs_handle_fs_error()  and then change insert_balance_item() to not fail in
>>> case the item already exists - instead just overwrite it.
>>
>> This means, if a unlucky interruption happened, the left balance item
>> can cause us to resume a balance on the next mount, which can be
>> unexpected for the end user.
>
> Yes, but maybe not much work is done unless after that some block
> groups got fragmented enough to trigger the stored balance filters in
> the item.
> The worst case is without filters, where all block groups are always relocated.
> Not ideal, yes.
>
> But having had a closer look, my concern is that I don't see how
> btrfs_start_transaction_fallback_global_rsv() can return -EINTR, and I
> suspect
> it comes from somewhere else.
>
> Thanks.
>
>>
>> Thanks,
>> Qu
>>>
>>> Thanks.
>>>
>>>
>>>>       if (IS_ERR(trans)) {
>>>>               btrfs_free_path(path);
>>>>               return PTR_ERR(trans);
>>>> @@ -3594,7 +3602,7 @@ static void reset_balance_state(struct btrfs_fs_info *fs_info)
>>>>       kfree(bctl);
>>>>       ret = del_balance_item(fs_info);
>>>>       if (ret)
>>>> -            btrfs_handle_fs_error(fs_info, ret, NULL);
>>>> +            btrfs_handle_fs_error(fs_info, ret, "failed to delete balance item");
>>>>    }
>>>>
>>>>    /*
>>>> --
>>>> 2.41.0
>>>>