[PATCH v2 0/2] ext4: fix WARNING in ext4_da_update_reserve_space

[PATCH v2 0/2] ext4: fix WARNING in ext4_da_update_reserve_space

[Baokun Li]

V1->V2:
	Modify the patch 1 description and add the Fixes tag.
	Add the patch 2 as suggested by Jan Kara.

Baokun Li (2):
  ext4: only update i_reserved_data_blocks on successful block
    allocation
  ext4: use __GFP_NOFAIL if allocating extents_status cannot fail

 fs/ext4/extents_status.c | 36 +++++++++++++++++++++++++++++-------
 fs/ext4/indirect.c       |  8 ++++++++
 fs/ext4/inode.c          | 10 ----------
 3 files changed, 37 insertions(+), 17 deletions(-)

-- 
2.31.1

[PATCH v2 1/2] ext4: only update i_reserved_data_blocks on successful block allocation

[Baokun Li]

In our fault injection test, we create an ext4 file, migrate it to
non-extent based file, then punch a hole and finally trigger a WARN_ON
in the ext4_da_update_reserve_space():

EXT4-fs warning (device sda): ext4_da_update_reserve_space:369:
ino 14, used 11 with only 10 reserved data blocks

When writing back a non-extent based file, if we enable delalloc, the
number of reserved blocks will be subtracted from the number of blocks
mapped by ext4_ind_map_blocks(), and the extent status tree will be
updated. We update the extent status tree by first removing the old
extent_status and then inserting the new extent_status. If the block range
we remove happens to be in an extent, then we need to allocate another
extent_status with ext4_es_alloc_extent().

       use old    to remove   to add new
    |----------|------------|------------|
              old extent_status

The problem is that the allocation of a new extent_status failed due to a
fault injection, and __es_shrink() did not get free memory, resulting in
a return of -ENOMEM. Then do_writepages() retries after receiving -ENOMEM,
we map to the same extent again, and the number of reserved blocks is again
subtracted from the number of blocks in that extent. Since the blocks in
the same extent are subtracted twice, we end up triggering WARN_ON at
ext4_da_update_reserve_space() because used > ei->i_reserved_data_blocks.

For non-extent based file, we update the number of reserved blocks after
ext4_ind_map_blocks() is executed, which causes a problem that when we call
ext4_ind_map_blocks() to create a block, it doesn't always create a block,
but we always reduce the number of reserved blocks. So we move the logic
for updating reserved blocks to ext4_ind_map_blocks() to ensure that the
number of reserved blocks is updated only after we do succeed in allocating
some new blocks.

Fixes: 5f634d064c70 ("ext4: Fix quota accounting error with fallocate")
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V1->V2:
	Modify the patch description and add the Fixes tag.

 fs/ext4/indirect.c |  8 ++++++++
 fs/ext4/inode.c    | 10 ----------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index c68bebe7ff4b..9acab70ddf5e 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -651,6 +651,14 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
 
 	ext4_update_inode_fsync_trans(handle, inode, 1);
 	count = ar.len;
+
+	/*
+	 * Update reserved blocks/metadata blocks after successful block
+	 * allocation which had been deferred till now.
+	 */
+	if ((count > 0) && (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))
+		ext4_da_update_reserve_space(inode, count, 1);
+
 got_it:
 	map->m_flags |= EXT4_MAP_MAPPED;
 	map->m_pblk = le32_to_cpu(chain[depth-1].key);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 97eb728cb958..33ae92f0ddfb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -659,16 +659,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 			 */
 			ext4_clear_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
 		}
-
-		/*
-		 * Update reserved blocks/metadata blocks after successful
-		 * block allocation which had been deferred till now. We don't
-		 * support fallocate for non extent files. So we can update
-		 * reserve space here.
-		 */
-		if ((retval > 0) &&
-			(flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))
-			ext4_da_update_reserve_space(inode, retval, 1);
 	}
 
 	if (retval > 0) {
-- 
2.31.1

Re: [PATCH v2 1/2] ext4: only update i_reserved_data_blocks on successful block allocation

[Jan Kara]

On Thu 06-04-23 21:28:33, Baokun Li wrote:
> In our fault injection test, we create an ext4 file, migrate it to
> non-extent based file, then punch a hole and finally trigger a WARN_ON
> in the ext4_da_update_reserve_space():
> 
> EXT4-fs warning (device sda): ext4_da_update_reserve_space:369:
> ino 14, used 11 with only 10 reserved data blocks
> 
> When writing back a non-extent based file, if we enable delalloc, the
> number of reserved blocks will be subtracted from the number of blocks
> mapped by ext4_ind_map_blocks(), and the extent status tree will be
> updated. We update the extent status tree by first removing the old
> extent_status and then inserting the new extent_status. If the block range
> we remove happens to be in an extent, then we need to allocate another
> extent_status with ext4_es_alloc_extent().
> 
>        use old    to remove   to add new
>     |----------|------------|------------|
>               old extent_status
> 
> The problem is that the allocation of a new extent_status failed due to a
> fault injection, and __es_shrink() did not get free memory, resulting in
> a return of -ENOMEM. Then do_writepages() retries after receiving -ENOMEM,
> we map to the same extent again, and the number of reserved blocks is again
> subtracted from the number of blocks in that extent. Since the blocks in
> the same extent are subtracted twice, we end up triggering WARN_ON at
> ext4_da_update_reserve_space() because used > ei->i_reserved_data_blocks.
> 
> For non-extent based file, we update the number of reserved blocks after
> ext4_ind_map_blocks() is executed, which causes a problem that when we call
> ext4_ind_map_blocks() to create a block, it doesn't always create a block,
> but we always reduce the number of reserved blocks. So we move the logic
> for updating reserved blocks to ext4_ind_map_blocks() to ensure that the
> number of reserved blocks is updated only after we do succeed in allocating
> some new blocks.
> 
> Fixes: 5f634d064c70 ("ext4: Fix quota accounting error with fallocate")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good, just one nit below.

> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
> index c68bebe7ff4b..9acab70ddf5e 100644
> --- a/fs/ext4/indirect.c
> +++ b/fs/ext4/indirect.c
> @@ -651,6 +651,14 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
>  
>  	ext4_update_inode_fsync_trans(handle, inode, 1);
>  	count = ar.len;
> +
> +	/*
> +	 * Update reserved blocks/metadata blocks after successful block
> +	 * allocation which had been deferred till now.
> +	 */
> +	if ((count > 0) && (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))

You don't need the count > 0 condition here. It should be always true.

								Honza

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

Re: [PATCH v2 1/2] ext4: only update i_reserved_data_blocks on successful block allocation

[Baokun Li]

On 2023/4/11 17:08, Jan Kara wrote:
> On Thu 06-04-23 21:28:33, Baokun Li wrote:
>> In our fault injection test, we create an ext4 file, migrate it to
>> non-extent based file, then punch a hole and finally trigger a WARN_ON
>> in the ext4_da_update_reserve_space():
>>
>> EXT4-fs warning (device sda): ext4_da_update_reserve_space:369:
>> ino 14, used 11 with only 10 reserved data blocks
>>
>> When writing back a non-extent based file, if we enable delalloc, the
>> number of reserved blocks will be subtracted from the number of blocks
>> mapped by ext4_ind_map_blocks(), and the extent status tree will be
>> updated. We update the extent status tree by first removing the old
>> extent_status and then inserting the new extent_status. If the block range
>> we remove happens to be in an extent, then we need to allocate another
>> extent_status with ext4_es_alloc_extent().
>>
>>         use old    to remove   to add new
>>      |----------|------------|------------|
>>                old extent_status
>>
>> The problem is that the allocation of a new extent_status failed due to a
>> fault injection, and __es_shrink() did not get free memory, resulting in
>> a return of -ENOMEM. Then do_writepages() retries after receiving -ENOMEM,
>> we map to the same extent again, and the number of reserved blocks is again
>> subtracted from the number of blocks in that extent. Since the blocks in
>> the same extent are subtracted twice, we end up triggering WARN_ON at
>> ext4_da_update_reserve_space() because used > ei->i_reserved_data_blocks.
>>
>> For non-extent based file, we update the number of reserved blocks after
>> ext4_ind_map_blocks() is executed, which causes a problem that when we call
>> ext4_ind_map_blocks() to create a block, it doesn't always create a block,
>> but we always reduce the number of reserved blocks. So we move the logic
>> for updating reserved blocks to ext4_ind_map_blocks() to ensure that the
>> number of reserved blocks is updated only after we do succeed in allocating
>> some new blocks.
>>
>> Fixes: 5f634d064c70 ("ext4: Fix quota accounting error with fallocate")
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> Looks good, just one nit below.
>
>> diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
>> index c68bebe7ff4b..9acab70ddf5e 100644
>> --- a/fs/ext4/indirect.c
>> +++ b/fs/ext4/indirect.c
>> @@ -651,6 +651,14 @@ int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
>>   
>>   	ext4_update_inode_fsync_trans(handle, inode, 1);
>>   	count = ar.len;
>> +
>> +	/*
>> +	 * Update reserved blocks/metadata blocks after successful block
>> +	 * allocation which had been deferred till now.
>> +	 */
>> +	if ((count > 0) && (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))
> You don't need the count > 0 condition here. It should be always true.
>
> 								Honza
>
Indeed!

Thanks a million!

I will remove this condition in patch v3.

-- 
With Best Regards,
Baokun Li
.

[PATCH v2 2/2] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail

[Baokun Li]

If extent status tree update fails, we have inconsistency between what is
stored in the extent status tree and what is stored on disk. And that can
cause even data corruption issues in some cases.

In the extent status tree, we have extents which we can just drop without
issues and extents we must not drop - this depends on the extent's status
- currently ext4_es_is_delayed() extents must stay, others may be dropped.

For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
A helper function is also added to help determine if the current extent can
be dropped, although only ext4_es_is_delayed() extents cannot be dropped
currently. In addition, with the above logic, the undo operation in
__es_remove_extent that may cause inconsistency if the split extent fails
is unnecessary, so we remove it as well.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V1->V2:
	Add the patch 2 as suggested by Jan Kara.

 fs/ext4/extents_status.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 7bc221038c6c..8eed17f35b11 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -448,12 +448,29 @@ static void ext4_es_list_del(struct inode *inode)
 	spin_unlock(&sbi->s_es_lock);
 }
 
+/*
+ * Helper function to help determine if memory allocation for this
+ * extent_status is allowed to fail.
+ */
+static inline bool ext4_es_alloc_should_nofail(struct extent_status *es)
+{
+	if (ext4_es_is_delayed(es))
+		return true;
+
+	return false;
+}
+
 static struct extent_status *
 ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
-		     ext4_fsblk_t pblk)
+		     ext4_fsblk_t pblk, int nofail)
 {
 	struct extent_status *es;
-	es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
+	gfp_t gfp_flags = GFP_ATOMIC;
+
+	if (nofail)
+		gfp_flags |= __GFP_NOFAIL;
+
+	es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
 	if (es == NULL)
 		return NULL;
 	es->es_lblk = lblk;
@@ -792,9 +809,16 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
 	}
 
 	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
-				  newes->es_pblk);
-	if (!es)
-		return -ENOMEM;
+				  newes->es_pblk, 0);
+	if (!es) {
+		/* Use GFP_NOFAIL if the allocation cannot fail. */
+		if (ext4_es_alloc_should_nofail(newes))
+			es = ext4_es_alloc_extent(inode, newes->es_lblk,
+					newes->es_len, newes->es_pblk, 1);
+		else
+			return -ENOMEM;
+	}
+
 	rb_link_node(&es->rb_node, parent, p);
 	rb_insert_color(&es->rb_node, &tree->root);
 
@@ -1349,8 +1373,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 						    ext4_es_status(&orig_es));
 			err = __es_insert_extent(inode, &newes);
 			if (err) {
-				es->es_lblk = orig_es.es_lblk;
-				es->es_len = orig_es.es_len;
 				if ((err == -ENOMEM) &&
 				    __es_shrink(EXT4_SB(inode->i_sb),
 							128, EXT4_I(inode)))
-- 
2.31.1

Re: [PATCH v2 2/2] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail

[Jan Kara]

On Thu 06-04-23 21:28:34, Baokun Li wrote:
> If extent status tree update fails, we have inconsistency between what is
> stored in the extent status tree and what is stored on disk. And that can
> cause even data corruption issues in some cases.
> 
> In the extent status tree, we have extents which we can just drop without
> issues and extents we must not drop - this depends on the extent's status
> - currently ext4_es_is_delayed() extents must stay, others may be dropped.
> 
> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
> A helper function is also added to help determine if the current extent can
> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
> currently. In addition, with the above logic, the undo operation in
> __es_remove_extent that may cause inconsistency if the split extent fails
> is unnecessary, so we remove it as well.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
> ---
> V1->V2:
> 	Add the patch 2 as suggested by Jan Kara.
> 
>  fs/ext4/extents_status.c | 36 +++++++++++++++++++++++++++++-------
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7bc221038c6c..8eed17f35b11 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -448,12 +448,29 @@ static void ext4_es_list_del(struct inode *inode)
>  	spin_unlock(&sbi->s_es_lock);
>  }
>  
> +/*
> + * Helper function to help determine if memory allocation for this
> + * extent_status is allowed to fail.
> + */
> +static inline bool ext4_es_alloc_should_nofail(struct extent_status *es)

I'd call this function ext4_es_must_keep() and also use it in
es_do_reclaim_extents() instead of ext4_es_is_delayed(). Do this as a
preparatory patch please.

> @@ -792,9 +809,16 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>  	}
>  
>  	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
> -				  newes->es_pblk);
> -	if (!es)
> -		return -ENOMEM;
> +				  newes->es_pblk, 0);

I would just call this like:

	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
				newes->es_pblk, ext4_es_must_keep(newes));

to save the ifs below.

> +	if (!es) {
> +		/* Use GFP_NOFAIL if the allocation cannot fail. */
> +		if (ext4_es_alloc_should_nofail(newes))
> +			es = ext4_es_alloc_extent(inode, newes->es_lblk,
> +					newes->es_len, newes->es_pblk, 1);
> +		else
> +			return -ENOMEM;
> +	}
> +
>  	rb_link_node(&es->rb_node, parent, p);
>  	rb_insert_color(&es->rb_node, &tree->root);
>  
> @@ -1349,8 +1373,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  						    ext4_es_status(&orig_es));
>  			err = __es_insert_extent(inode, &newes);
>  			if (err) {
> -				es->es_lblk = orig_es.es_lblk;
> -				es->es_len = orig_es.es_len;
>  				if ((err == -ENOMEM) &&
>  				    __es_shrink(EXT4_SB(inode->i_sb),
>  							128, EXT4_I(inode)))

Also now __es_remove_extent() cannot fail (it will always remove what it
should, maybe more) so please just make it void function (as a separate
cleanup patch afterwards). Thanks!

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

Re: [PATCH v2 2/2] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail

[Baokun Li]

On 2023/4/11 17:19, Jan Kara wrote:
> On Thu 06-04-23 21:28:34, Baokun Li wrote:
>> If extent status tree update fails, we have inconsistency between what is
>> stored in the extent status tree and what is stored on disk. And that can
>> cause even data corruption issues in some cases.
>>
>> In the extent status tree, we have extents which we can just drop without
>> issues and extents we must not drop - this depends on the extent's status
>> - currently ext4_es_is_delayed() extents must stay, others may be dropped.
>>
>> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
>> A helper function is also added to help determine if the current extent can
>> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
>> currently. In addition, with the above logic, the undo operation in
>> __es_remove_extent that may cause inconsistency if the split extent fails
>> is unnecessary, so we remove it as well.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: Baokun Li <libaokun1@huawei.com>
>> ---
>> V1->V2:
>> 	Add the patch 2 as suggested by Jan Kara.
>>
>>   fs/ext4/extents_status.c | 36 +++++++++++++++++++++++++++++-------
>>   1 file changed, 29 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
>> index 7bc221038c6c..8eed17f35b11 100644
>> --- a/fs/ext4/extents_status.c
>> +++ b/fs/ext4/extents_status.c
>> @@ -448,12 +448,29 @@ static void ext4_es_list_del(struct inode *inode)
>>   	spin_unlock(&sbi->s_es_lock);
>>   }
>>   
>> +/*
>> + * Helper function to help determine if memory allocation for this
>> + * extent_status is allowed to fail.
>> + */
>> +static inline bool ext4_es_alloc_should_nofail(struct extent_status *es)
> I'd call this function ext4_es_must_keep() and also use it in
> es_do_reclaim_extents() instead of ext4_es_is_delayed(). Do this as a
> preparatory patch please.

Totally agree! ext4_es_must_keep() is short and clear. It does make more 
sense to
replace ext4_es_is_delayed() in es_do_reclaim_extents() with the new 
helper, I'll try
to find out if there are any ext4_es_is_delayed() that need to be 
replaced as well.

>> @@ -792,9 +809,16 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>>   	}
>>   
>>   	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
>> -				  newes->es_pblk);
>> -	if (!es)
>> -		return -ENOMEM;
>> +				  newes->es_pblk, 0);
> I would just call this like:
>
> 	es = ext4_es_alloc_extent(inode, newes->es_lblk, newes->es_len,
> 				newes->es_pblk, ext4_es_must_keep(newes));
>
> to save the ifs below.

Yes! It does get a little long-winded here.

>
>> +	if (!es) {
>> +		/* Use GFP_NOFAIL if the allocation cannot fail. */
>> +		if (ext4_es_alloc_should_nofail(newes))
>> +			es = ext4_es_alloc_extent(inode, newes->es_lblk,
>> +					newes->es_len, newes->es_pblk, 1);
>> +		else
>> +			return -ENOMEM;
>> +	}
>> +
>>   	rb_link_node(&es->rb_node, parent, p);
>>   	rb_insert_color(&es->rb_node, &tree->root);
>>   
>> @@ -1349,8 +1373,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>>   						    ext4_es_status(&orig_es));
>>   			err = __es_insert_extent(inode, &newes);
>>   			if (err) {
>> -				es->es_lblk = orig_es.es_lblk;
>> -				es->es_len = orig_es.es_len;
>>   				if ((err == -ENOMEM) &&
>>   				    __es_shrink(EXT4_SB(inode->i_sb),
>>   							128, EXT4_I(inode)))
> Also now __es_remove_extent() cannot fail (it will always remove what it
> should, maybe more) so please just make it void function (as a separate
> cleanup patch afterwards). Thanks!
>
> 								Honza
Yes! Thank you very much for the review!
I will send a patch V3 with the changes suggested by you.

-- 
With Best Regards,
Baokun Li
.