[PATCH v5 0/2] Fix dead loop in ext4_mb_new_blocks

[PATCH v5 0/2] Fix dead loop in ext4_mb_new_blocks

[Ye Bin]

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(-)

-- 
2.25.4

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

[Ye Bin]

From: Jan Kara <jack@suse.cz>

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 <jack@suse.cz>
Signed-off-by: Ye Bin <yebin10@huawei.com>
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/mballoc.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index 132c118d12e1..f736819a381b 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -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);
-- 
2.25.4

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

[Ritesh Harjani]

On 9/16/20 5:08 PM, Ye Bin wrote:
> From: Jan Kara <jack@suse.cz>
> 
> 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 <jack@suse.cz>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ---
>   fs/ext4/mballoc.c | 28 +++++++++++-----------------
>   1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 132c118d12e1..f736819a381b 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -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;

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

> +		goto repeat;
> +	}
> +
>   	ext4_unlock_group(sb, group);
>   	ext4_mb_unload_buddy(&e4b);
>   	put_bh(bitmap_bh);
> 

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

[Jan Kara]

On Fri 18-09-20 14:37:15, Ritesh Harjani wrote:
> 
> 
> On 9/16/20 5:08 PM, Ye Bin wrote:
> > From: Jan Kara <jack@suse.cz>
> > 
> > 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 <jack@suse.cz>
> > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
...
> > +	/* 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.

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Theodore Y. Ts'o]

On Fri, Sep 18, 2020 at 11:56:53AM +0200, Jan Kara wrote:
> On Fri 18-09-20 14:37:15, Ritesh Harjani wrote:
> > 
> > 
> > On 9/16/20 5:08 PM, Ye Bin wrote:
> > > From: Jan Kara <jack@suse.cz>
> > > 
> > > 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 <jack@suse.cz>
> > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> ...
> > > +	/* 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.
> 
> 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

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

[Jan Kara]

On Thu 24-09-20 11:00:34, Theodore Y. Ts'o wrote:
> On Fri, Sep 18, 2020 at 11:56:53AM +0200, Jan Kara wrote:
> > On Fri 18-09-20 14:37:15, Ritesh Harjani wrote:
> > > 
> > > 
> > > On 9/16/20 5:08 PM, Ye Bin wrote:
> > > > From: Jan Kara <jack@suse.cz>
> > > > 
> > > > 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 <jack@suse.cz>
> > > > Signed-off-by: Ye Bin <yebin10@huawei.com>
> > > > Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
> > ...
> > > > +	/* 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.
> > 
> > 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/20200924150959.4335-1-jack@suse.cz

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

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

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

[Theodore Y. Ts'o]

On Wed, Sep 16, 2020 at 07:38:58PM +0800, Ye Bin wrote:
> From: Jan Kara <jack@suse.cz>
> 
> 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 <jack@suse.cz>
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Thanks, applied.

					- Ted

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

[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 <yebin10@huawei.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>
---
 fs/ext4/mballoc.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index f736819a381b..4d40d8dc518c 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -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;
 
-- 
2.25.4

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

[Theodore Y. Ts'o]

On Wed, Sep 16, 2020 at 07:38:59PM +0800, Ye Bin wrote:
> 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 <yebin10@huawei.com>
> Reviewed-by: Jan Kara <jack@suse.cz>
> Reviewed-by: Ritesh Harjani <riteshh@linux.ibm.com>

Thanks, applied.

						- Ted