Skip to contentSkip to conversation

Fix dead loop in ext4_mb_new_blocks

PATCH Applied Ye Bin 4 participants 9 messages Sep 16, 2020 – Sep 24, 2020 ext4 file system

lore
Share & tools
lore

Ye Bin

author
#0 lore raw

v5 cover Fix dead loop in ext4_mb_new_blocks

Jan Kara (1):
ext4: Discard preallocations before releasing group lock

Ye Bin (1):
ext4: Fix dead loop in ext4_mb_new_blocks

 fs/ext4/mballoc.c | 32 ++++++++++++++------------------
 1 file changed, 14 insertions(+), 18 deletions(-)
Signature · 1 line
2.25.4
author
#1 lore raw

PATCH v5 1/2 ext4: Discard preallocations before releasing group lock

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>
---
 fs/ext4/mballoc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
fs/ext4/mballoc.c +11 -17
index 132c118d12e1..f736819a381b 100644
@@ -4215,22 +4215,6 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
 		list_add(&pa->u.pa_tmp_list, &list);
 	}
 
-	/* if we still need more blocks and some PAs were used, try again */
-	if (free < needed && busy) {
-		busy = 0;
-		ext4_unlock_group(sb, group);
-		cond_resched();
-		goto repeat;
-	}
-
-	/* found anything to free? */
-	if (list_empty(&list)) {
-		BUG_ON(free != 0);
-		mb_debug(sb, "Someone else may have freed PA for this group %u\n",
-			 group);
-		goto out;
-	}
-
 	/* now free all selected PAs */
 	list_for_each_entry_safe(pa, tmp, &list, u.pa_tmp_list) {
 

@@ -4248,7 +4232,17 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
 		call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
 	}
 
-out:
+	/* if we still need more blocks and some PAs were used, try again */
+	if (free < needed && busy) {
+		ext4_unlock_group(sb, group);
+		cond_resched();
+		busy = 0;
+		/* Make sure we increment discard_pa_seq again */
+		needed -= free;
+		free = 0;
+		goto repeat;
+	}
+
 	ext4_unlock_group(sb, group);
 	ext4_mb_unload_buddy(&e4b);
 	put_bh(bitmap_bh);
Signature · 1 line
2.25.4
#2 lore raw

Re: [PATCH v5 1/2] ext4: Discard preallocations before releasing group lock

Quoted 48 lines · Ye Bin

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>
---
  fs/ext4/mballoc.c | 28 +++++++++++-----------------
  1 file changed, 11 insertions(+), 17 deletions(-)
fs/ext4/mballoc.c +0 -16
index 132c118d12e1..f736819a381b 100644
@@ -4215,22 +4215,6 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
  		list_add(&pa->u.pa_tmp_list, &list);
  	}
 
-	/* if we still need more blocks and some PAs were used, try again */
-	if (free < needed && busy) {
-		busy = 0;
-		ext4_unlock_group(sb, group);
-		cond_resched();
-		goto repeat;
-	}
-
-	/* found anything to free? */
-	if (list_empty(&list)) {
-		BUG_ON(free != 0);
-		mb_debug(sb, "Someone else may have freed PA for this group %u\n",
-			 group);
-		goto out;
-	}
-
  	/* now free all selected PAs */
  	list_for_each_entry_safe(pa, tmp, &list, u.pa_tmp_list) {
 

@@ -4248,7 +4232,17 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
  		call_rcu(&(pa)->u.pa_rcu, ext4_mb_pa_callback);
  	}

fs/ext4/mballoc.c ext4_mb_discard_group_preallocations(struct super_block *sb, · line 4242

Quoted 10 lines
fs/ext4/mballoc.c +8 -1
 
-out:
+	/* if we still need more blocks and some PAs were used, try again */
+	if (free < needed && busy) {
+		ext4_unlock_group(sb, group);
+		cond_resched();
+		busy = 0;
+		/* Make sure we increment discard_pa_seq again */
+		needed -= free;
+		free = 0;

Oops sorry about getting back to this.
But if we are making free 0 here so we may return a wrong free value when we return from this function. We should fix that by also accounting previous freed blocks at the time of final return from this function.

-ritesh

Quoted 7 lines
fs/ext4/mballoc.c +3 -0
+		goto repeat;
+	}
+
  	ext4_unlock_group(sb, group);
  	ext4_mb_unload_buddy(&e4b);
  	put_bh(bitmap_bh);
reviewer
#3 lore raw

Re: [PATCH v5 1/2] ext4: Discard preallocations before releasing group lock

Quoted 17 lines · Ritesh Harjani
Quoted 14 lines · Ye Bin

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

...

Quoted 5 lines
Quoted 5 lines
fs/ext4/mballoc.c +5 -0
+	/* if we still need more blocks and some PAs were used, try again */
+	if (free < needed && busy) {
+		ext4_unlock_group(sb, group);
+		cond_resched();
+		busy = 0;
Quoted 8 lines

fs/ext4/mballoc.c ext4_mb_discard_group_preallocations(struct super_block *sb, · line 4242

Quoted 3 lines
fs/ext4/mballoc.c +3 -0
+		/* Make sure we increment discard_pa_seq again */
+		needed -= free;
+		free = 0;

Oops sorry about getting back to this.
But if we are making free 0 here so we may return a wrong free value when we return from this function. We should fix that by also accounting previous freed blocks at the time of final return from this function.

Ah, good catch! I'll send v2 with this fixed up.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
maintainer
#4 lore raw

Re: [PATCH v5 1/2] ext4: Discard preallocations before releasing group lock

Quoted 33 lines · Jan Kara
Quoted 17 lines · Ritesh Harjani
Quoted 14 lines · Ye Bin

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

...

Quoted 5 lines
Quoted 5 lines
fs/ext4/mballoc.c +5 -0
+	/* if we still need more blocks and some PAs were used, try again */
+	if (free < needed && busy) {
+		ext4_unlock_group(sb, group);
+		cond_resched();
+		busy = 0;
Quoted 8 lines

fs/ext4/mballoc.c ext4_mb_discard_group_preallocations(struct super_block *sb, · line 4242

Quoted 3 lines
fs/ext4/mballoc.c +3 -0
+		/* Make sure we increment discard_pa_seq again */
+		needed -= free;
+		free = 0;

Oops sorry about getting back to this.
But if we are making free 0 here so we may return a wrong free value when we return from this function. We should fix that by also accounting previous freed blocks at the time of final return from this function.

Quoted 1 line

Ah, good catch! I'll send v2 with this fixed up.

Did you send a v2 of this patch? I can't find it in my inbox...

Thanks!

- Ted

reviewer
#5 lore raw

Re: [PATCH v5 1/2] ext4: Discard preallocations before releasing group lock

Quoted 34 lines · Theodore Y. Ts'o
Quoted 33 lines · Jan Kara
Quoted 17 lines · Ritesh Harjani
Quoted 14 lines · Ye Bin

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

...

Quoted 13 lines

fs/ext4/mballoc.c ext4_mb_discard_group_preallocations(struct super_block *sb, · line 4242

Quoted 8 lines
fs/ext4/mballoc.c +8 -0
+	/* if we still need more blocks and some PAs were used, try again */
+	if (free < needed && busy) {
+		ext4_unlock_group(sb, group);
+		cond_resched();
+		busy = 0;
+		/* Make sure we increment discard_pa_seq again */
+		needed -= free;
+		free = 0;

Oops sorry about getting back to this.
But if we are making free 0 here so we may return a wrong free value when we return from this function. We should fix that by also accounting previous freed blocks at the time of final return from this function.

Quoted 3 lines
Quoted 1 line

Ah, good catch! I'll send v2 with this fixed up.

Did you send a v2 of this patch? I can't find it in my inbox...

Yeah, somehow I forgot to send it. I've sent it now:

https://lore.kernel.org/linux-ext4/[email protected]

Note that Ye Bin's patch will need trivial context fixup after applying this...

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
maintainer Applied
#6 lore raw

Re: [PATCH v5 1/2] ext4: Discard preallocations before releasing group lock

In reply to Ye Bin
Quoted 10 lines · Ye Bin

From: Jan Kara <[email protected]>

ext4_mb_discard_group_preallocations() can be releasing group lock with preallocations accumulated on its local list. Thus although discard_pa_seq was incremented and concurrent allocating processes will be retrying allocations, it can happen that premature ENOSPC error is returned because blocks used for preallocations are not available for reuse yet. Make sure we always free locally accumulated preallocations before releasing group lock.

Quoted 4 lines
Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Jan Kara <[email protected]>
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

Thanks, applied.

- Ted

author
#7 lore raw

PATCH v5 2/2 ext4: Fix dead loop in ext4_mb_new_blocks

In reply to Ye Bin

As we test disk offline/online with running fsstress, we find fsstress process is keeping running state.
kworker/u32:3-262 [004] ...1 140.787471: ext4_mb_discard_preallocations: dev 8,32 needed 114
....
kworker/u32:3-262 [004] ...1 140.787471: ext4_mb_discard_preallocations: dev 8,32 needed 114

ext4_mb_new_blocks
repeat:

        ext4_mb_discard_preallocations_should_retry(sb, ac, &seq)
                freed = ext4_mb_discard_preallocations
                        ext4_mb_discard_group_preallocations
                                this_cpu_inc(discard_pa_seq);
                ---> freed == 0
                seq_retry = ext4_get_discard_pa_seq_sum
                        for_each_possible_cpu(__cpu)
                                __seq += per_cpu(discard_pa_seq, __cpu);
                if (seq_retry != *seq) {
                        *seq = seq_retry;
                        ret = true;
                }

As we see seq_retry is sum of discard_pa_seq every cpu, if ext4_mb_discard_group_preallocations return zero discard_pa_seq in this cpu maybe increase one, so condition "seq_retry != *seq" have always been met.
Ritesh Harjani suggest to in ext4_mb_discard_group_preallocations function we only increase discard_pa_seq when there is some PA to free.

Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>
---
 fs/ext4/mballoc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
fs/ext4/mballoc.c +3 -1
index f736819a381b..4d40d8dc518c 100644
@@ -4189,7 +4189,6 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
 	INIT_LIST_HEAD(&list);
 repeat:
 	ext4_lock_group(sb, group);
-	this_cpu_inc(discard_pa_seq);
 	list_for_each_entry_safe(pa, tmp,
 				&grp->bb_prealloc_list, pa_group_list) {
 		spin_lock(&pa->pa_lock);

@@ -4206,6 +4205,9 @@ ext4_mb_discard_group_preallocations(struct super_block *sb,
 		/* seems this one can be freed ... */
 		ext4_mb_mark_pa_deleted(sb, pa);
 
+		if (!free)
+			this_cpu_inc(discard_pa_seq);
+
 		/* we can trust pa_free ... */
 		free += pa->pa_free;
Signature · 1 line
2.25.4
maintainer Applied
#8 lore raw

Re: [PATCH v5 2/2] ext4: Fix dead loop in ext4_mb_new_blocks

Quoted 28 lines · Ye Bin

As we test disk offline/online with running fsstress, we find fsstress process is keeping running state.
kworker/u32:3-262 [004] ...1 140.787471: ext4_mb_discard_preallocations: dev 8,32 needed 114
....
kworker/u32:3-262 [004] ...1 140.787471: ext4_mb_discard_preallocations: dev 8,32 needed 114

ext4_mb_new_blocks
repeat:

        ext4_mb_discard_preallocations_should_retry(sb, ac, &seq)
                freed = ext4_mb_discard_preallocations
                        ext4_mb_discard_group_preallocations
                                this_cpu_inc(discard_pa_seq);
                ---> freed == 0
                seq_retry = ext4_get_discard_pa_seq_sum
                        for_each_possible_cpu(__cpu)
                                __seq += per_cpu(discard_pa_seq, __cpu);
                if (seq_retry != *seq) {
                        *seq = seq_retry;
                        ret = true;
                }

As we see seq_retry is sum of discard_pa_seq every cpu, if ext4_mb_discard_group_preallocations return zero discard_pa_seq in this cpu maybe increase one, so condition "seq_retry != *seq" have always been met.
Ritesh Harjani suggest to in ext4_mb_discard_group_preallocations function we only increase discard_pa_seq when there is some PA to free.

Quoted 4 lines
Fixes 07b5b8e1ac40 ("ext4: mballoc: introduce pcpu seqcnt for freeing PA to improve ENOSPC handling")
Signed-off-by Ye Bin <[email protected]>
Reviewed-by Jan Kara <[email protected]>
Reviewed-by Ritesh Harjani <[email protected]>

Thanks, applied.

- Ted