Hi Jan & riteshh:
Thanks for your time to review this patch. But kernel test robot has
reported an warning for the v2 version. I have fixed it and submitted
the v3 version, maybe we should goto v3 version, thanks all.
Jan Kara wrote on 2021/11/23 1:50:
> Hello,
>
> it seems this patch has fallen through the cracks.
>
> On Wed 29-09-21 09:12:25, brookxu wrote:
>> From: Chunguang Xu <brookxu@xxxxxxxxxxx>
>>
>> We found on older kernel (3.10) that in the scenario of insufficient
>> disk space, system may trigger an ABBA deadlock problem, it seems that
>> this problem still exists in latest kernel, try to fix it here. The
>> main process triggered by this problem is that task A occupies the PA
>> and waits for the jbd2 transaction finish, the jbd2 transaction waits
>> for the completion of task B's IO (plug_list), but task B waits for
>> the release of PA by task A to finish discard, which indirectly forms
>> an ABBA deadlock. The related calltrace is as follows:
>>
>> Task A
>> vfs_write
>> ext4_mb_new_blocks()
>> ext4_mb_mark_diskspace_used() JBD2
>> jbd2_journal_get_write_access() -> jbd2_journal_commit_transaction()
>> ->schedule() filemap_fdatawait()
>> | |
>> | Task B |
>> | do_unlinkat() |
>> | ext4_evict_inode() |
>> | jbd2_journal_begin_ordered_truncate() |
>> | filemap_fdatawrite_range() |
>> | ext4_mb_new_blocks() |
>> -ext4_mb_discard_group_preallocations() <-----
>>
>> Here, try to cancel ext4_mb_discard_group_preallocations() internal
>> retry due to PA busy, and do a limited number of retries inside
>> ext4_mb_discard_preallocations(), which can circumvent the above
>> problems, but also has some advantages:
>>
>> 1. Since the PA is in a busy state, if other groups have free PAs,
>> keeping the current PA may help to reduce fragmentation.
>> 2. Continue to traverse forward instead of waiting for the current
>> group PA to be released. In most scenarios, the PA discard time
>> can be reduced.
>>
>> However, in the case of smaller free space, if only a few groups have
>> space, then due to multiple traversals of the group, it may increase
>> CPU overhead. But in contrast, I feel that the overall benefit is
>> better than the cost.
>>
>> Signed-off-by: Chunguang Xu <brookxu@xxxxxxxxxxx>
>
> Thanks for the patch! I guess this is a reasonable middle-ground so feel
> free to add:
>
> Reviewed-by: Jan Kara <jack@xxxxxxx>
>
> I think Ritesh was last touching this logic so let's check he doesn't see
> anything wrong with this change. Ritesh?
>
> Honza
>
>
>> ---
>> v2: reset busy to zero before goto repeat.
>>
>> fs/ext4/mballoc.c | 35 ++++++++++++++++-------------------
>> 1 file changed, 16 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
>> index 72bfac2..72de6c1 100644
>> --- a/fs/ext4/mballoc.c
>> +++ b/fs/ext4/mballoc.c
>> @@ -4814,7 +4814,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
>> */
>> static noinline_for_stack int
>> ext4_mb_discard_group_preallocations(struct super_block *sb,
>> - ext4_group_t group, int needed)
>> + ext4_group_t group, int needed, int *busy)
>> {
>> struct ext4_group_info *grp = ext4_get_group_info(sb, group);
>> struct buffer_head *bitmap_bh = NULL;
>> @@ -4822,8 +4822,7 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
>> struct list_head list;
>> struct ext4_buddy e4b;
>> int err;
>> - int busy = 0;
>> - int free, free_total = 0;
>> + int free = 0;
>>
>> mb_debug(sb, "discard preallocation for group %u\n", group);
>> if (list_empty(&grp->bb_prealloc_list))
>> @@ -4850,15 +4849,13 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
>> needed = EXT4_CLUSTERS_PER_GROUP(sb) + 1;
>>
>> INIT_LIST_HEAD(&list);
>> -repeat:
>> - free = 0;
>> ext4_lock_group(sb, group);
>> list_for_each_entry_safe(pa, tmp,
>> &grp->bb_prealloc_list, pa_group_list) {
>> spin_lock(&pa->pa_lock);
>> if (atomic_read(&pa->pa_count)) {
>> spin_unlock(&pa->pa_lock);
>> - busy = 1;
>> + *busy = 1;
>> continue;
>> }
>> if (pa->pa_deleted) {
>> @@ -4898,22 +4895,13 @@ static void ext4_mb_new_preallocation(struct ext4_allocation_context *ac)
>> call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
>> }
>>
>> - free_total += free;
>> -
>> - /* if we still need more blocks and some PAs were used, try again */
>> - if (free_total < needed && busy) {
>> - ext4_unlock_group(sb, group);
>> - cond_resched();
>> - busy = 0;
>> - goto repeat;
>> - }
>> ext4_unlock_group(sb, group);
>> ext4_mb_unload_buddy(&e4b);
>> put_bh(bitmap_bh);
>> out_dbg:
>> mb_debug(sb, "discarded (%d) blocks preallocated for group %u bb_free (%d)\n",
>> - free_total, group, grp->bb_free);
>> - return free_total;
>> + free, group, grp->bb_free);
>> + return free;
>> }
>>
>> /*
>> @@ -5455,13 +5443,22 @@ static int ext4_mb_discard_preallocations(struct super_block *sb, int needed)
>> {
>> ext4_group_t i, ngroups = ext4_get_groups_count(sb);
>> int ret;
>> - int freed = 0;
>> + int freed = 0, busy = 0;
>> + int retry = 0;
>>
>> trace_ext4_mb_discard_preallocations(sb, needed);
>> + repeat:
>> + retry++;
>> for (i = 0; i < ngroups && needed > 0; i++) {
>> - ret = ext4_mb_discard_group_preallocations(sb, i, needed);
>> + ret = ext4_mb_discard_group_preallocations(sb, i, needed, &busy);
>> freed += ret;
>> needed -= ret;
>> + cond_resched();
>> + }
>> +
>> + if (needed > 0 && busy && retry < 3) {
>> + busy = 0;
>> + goto repeat;
>> }
>>
>> return freed;
>> --
>> 1.8.3.1
>>
