[PATCH v4 00/12] ext4: fix WARNING in ext4_da_update_reserve_space

[PATCH v4 00/12] 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.
V2->V3:
        Remove the redundant judgment of count in Patch [1].
        Rename ext4_es_alloc_should_nofail to ext4_es_must_keep.
        Split Patch [2].
        Make some functions return void to simplify the code.
V3->V4:
        using nofail preallocation.

This patch set consists of three parts:
1. Patch [1] fix WARNING in ext4_da_update_reserve_space.
2. Patch [2]-[8] fix extent tree inconsistencies that may be caused
   by memory allocation failures.
3. Patch [9]-[12] is cleanup.

Baokun Li (12):
  ext4: only update i_reserved_data_blocks on successful block
    allocation
  ext4: add a new helper to check if es must be kept
  ext4: factor out __es_alloc_extent() and __es_free_extent()
  ext4: use pre-allocated es in __es_insert_extent()
  ext4: use pre-allocated es in __es_remove_extent()
  ext4: using nofail preallocation in ext4_es_remove_extent()
  ext4: using nofail preallocation in ext4_es_insert_delayed_block()
  ext4: using nofail preallocation in ext4_es_insert_extent()
  ext4: make ext4_es_remove_extent() return void
  ext4: make ext4_es_insert_delayed_block() return void
  ext4: make ext4_es_insert_extent() return void
  ext4: make ext4_zeroout_es() return void

 fs/ext4/extents.c        |  49 +++------
 fs/ext4/extents_status.c | 207 ++++++++++++++++++++++++---------------
 fs/ext4/extents_status.h |  14 +--
 fs/ext4/indirect.c       |   8 ++
 fs/ext4/inline.c         |  12 +--
 fs/ext4/inode.c          |  49 ++-------
 6 files changed, 169 insertions(+), 170 deletions(-)

-- 
2.31.1

[PATCH v4 01/12] 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>
Reviewed-by: Jan Kara <jack@suse.cz>
---
 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..a9f3716119d3 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 (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 974747a6eb99..20de04399c8b 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

[PATCH v4 02/12] ext4: add a new helper to check if es must be kept

[Baokun Li]

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.

A helper function is added to help determine if the current extent can
be dropped, although only ext4_es_is_delayed() extents cannot be dropped
currently.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 7bc221038c6c..573723b23d19 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -448,6 +448,19 @@ static void ext4_es_list_del(struct inode *inode)
 	spin_unlock(&sbi->s_es_lock);
 }
 
+/*
+ * Returns true if we cannot fail to allocate memory for this extent_status
+ * entry and cannot reclaim it until its status changes.
+ */
+static inline bool ext4_es_must_keep(struct extent_status *es)
+{
+	/* fiemap, bigalloc, and seek_data/hole need to use it. */
+	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)
@@ -460,10 +473,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
 	es->es_len = len;
 	es->es_pblk = pblk;
 
-	/*
-	 * We don't count delayed extent because we never try to reclaim them
-	 */
-	if (!ext4_es_is_delayed(es)) {
+	/* We never try to reclaim a must kept extent, so we don't count it. */
+	if (!ext4_es_must_keep(es)) {
 		if (!EXT4_I(inode)->i_es_shk_nr++)
 			ext4_es_list_add(inode);
 		percpu_counter_inc(&EXT4_SB(inode->i_sb)->
@@ -481,8 +492,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
 	EXT4_I(inode)->i_es_all_nr--;
 	percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
 
-	/* Decrease the shrink counter when this es is not delayed */
-	if (!ext4_es_is_delayed(es)) {
+	/* Decrease the shrink counter when we can reclaim the extent. */
+	if (!ext4_es_must_keep(es)) {
 		BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0);
 		if (!--EXT4_I(inode)->i_es_shk_nr)
 			ext4_es_list_del(inode);
@@ -853,7 +864,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
 					  128, EXT4_I(inode)))
 		goto retry;
-	if (err == -ENOMEM && !ext4_es_is_delayed(&newes))
+	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
 		err = 0;
 
 	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
@@ -1706,11 +1717,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end,
 
 		(*nr_to_scan)--;
 		node = rb_next(&es->rb_node);
-		/*
-		 * We can't reclaim delayed extent from status tree because
-		 * fiemap, bigallic, and seek_data/hole need to use it.
-		 */
-		if (ext4_es_is_delayed(es))
+
+		if (ext4_es_must_keep(es))
 			goto next;
 		if (ext4_es_is_referenced(es)) {
 			ext4_es_clear_referenced(es);
@@ -1774,7 +1782,7 @@ void ext4_clear_inode_es(struct inode *inode)
 	while (node) {
 		es = rb_entry(node, struct extent_status, rb_node);
 		node = rb_next(node);
-		if (!ext4_es_is_delayed(es)) {
+		if (!ext4_es_must_keep(es)) {
 			rb_erase(&es->rb_node, &tree->root);
 			ext4_es_free_extent(inode, es);
 		}
-- 
2.31.1

Re: [PATCH v4 02/12] ext4: add a new helper to check if es must be kept

[Jan Kara]

On Mon 24-04-23 11:38:36, Baokun Li wrote:
> 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.
> 
> A helper function is added to help determine if the current extent can
> be dropped, although only ext4_es_is_delayed() extents cannot be dropped
> currently.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7bc221038c6c..573723b23d19 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -448,6 +448,19 @@ static void ext4_es_list_del(struct inode *inode)
>  	spin_unlock(&sbi->s_es_lock);
>  }
>  
> +/*
> + * Returns true if we cannot fail to allocate memory for this extent_status
> + * entry and cannot reclaim it until its status changes.
> + */
> +static inline bool ext4_es_must_keep(struct extent_status *es)
> +{
> +	/* fiemap, bigalloc, and seek_data/hole need to use it. */
> +	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)
> @@ -460,10 +473,8 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>  	es->es_len = len;
>  	es->es_pblk = pblk;
>  
> -	/*
> -	 * We don't count delayed extent because we never try to reclaim them
> -	 */
> -	if (!ext4_es_is_delayed(es)) {
> +	/* We never try to reclaim a must kept extent, so we don't count it. */
> +	if (!ext4_es_must_keep(es)) {
>  		if (!EXT4_I(inode)->i_es_shk_nr++)
>  			ext4_es_list_add(inode);
>  		percpu_counter_inc(&EXT4_SB(inode->i_sb)->
> @@ -481,8 +492,8 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
>  	EXT4_I(inode)->i_es_all_nr--;
>  	percpu_counter_dec(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
>  
> -	/* Decrease the shrink counter when this es is not delayed */
> -	if (!ext4_es_is_delayed(es)) {
> +	/* Decrease the shrink counter when we can reclaim the extent. */
> +	if (!ext4_es_must_keep(es)) {
>  		BUG_ON(EXT4_I(inode)->i_es_shk_nr == 0);
>  		if (!--EXT4_I(inode)->i_es_shk_nr)
>  			ext4_es_list_del(inode);
> @@ -853,7 +864,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
>  					  128, EXT4_I(inode)))
>  		goto retry;
> -	if (err == -ENOMEM && !ext4_es_is_delayed(&newes))
> +	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
>  		err = 0;
>  
>  	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
> @@ -1706,11 +1717,8 @@ static int es_do_reclaim_extents(struct ext4_inode_info *ei, ext4_lblk_t end,
>  
>  		(*nr_to_scan)--;
>  		node = rb_next(&es->rb_node);
> -		/*
> -		 * We can't reclaim delayed extent from status tree because
> -		 * fiemap, bigallic, and seek_data/hole need to use it.
> -		 */
> -		if (ext4_es_is_delayed(es))
> +
> +		if (ext4_es_must_keep(es))
>  			goto next;
>  		if (ext4_es_is_referenced(es)) {
>  			ext4_es_clear_referenced(es);
> @@ -1774,7 +1782,7 @@ void ext4_clear_inode_es(struct inode *inode)
>  	while (node) {
>  		es = rb_entry(node, struct extent_status, rb_node);
>  		node = rb_next(node);
> -		if (!ext4_es_is_delayed(es)) {
> +		if (!ext4_es_must_keep(es)) {
>  			rb_erase(&es->rb_node, &tree->root);
>  			ext4_es_free_extent(inode, es);
>  		}
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 03/12] ext4: factor out __es_alloc_extent() and __es_free_extent()

[Baokun Li]

Factor out __es_alloc_extent() and __es_free_extent(), which only allocate
and free extent_status in these two helpers.

The ext4_es_alloc_extent() function is split into __es_alloc_extent()
and ext4_es_init_extent(). In __es_alloc_extent() we allocate memory using
GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO if the memory allocation cannot
fail, otherwise we use GFP_ATOMIC. and the ext4_es_init_extent() is used to
initialize extent_status and update related variables after a successful
allocation.

This is to prepare for the use of pre-allocated extent_status later.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 573723b23d19..18665394392f 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -461,14 +461,17 @@ static inline bool ext4_es_must_keep(struct extent_status *es)
 	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)
+static inline struct extent_status *__es_alloc_extent(bool nofail)
+{
+	if (!nofail)
+		return kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
+
+	return kmem_cache_zalloc(ext4_es_cachep, GFP_KERNEL | __GFP_NOFAIL);
+}
+
+static void ext4_es_init_extent(struct inode *inode, struct extent_status *es,
+		ext4_lblk_t lblk, ext4_lblk_t len, ext4_fsblk_t pblk)
 {
-	struct extent_status *es;
-	es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
-	if (es == NULL)
-		return NULL;
 	es->es_lblk = lblk;
 	es->es_len = len;
 	es->es_pblk = pblk;
@@ -483,8 +486,11 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
 
 	EXT4_I(inode)->i_es_all_nr++;
 	percpu_counter_inc(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
+}
 
-	return es;
+static inline void __es_free_extent(struct extent_status *es)
+{
+	kmem_cache_free(ext4_es_cachep, es);
 }
 
 static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
@@ -501,7 +507,7 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
 					s_es_stats.es_stats_shk_cnt);
 	}
 
-	kmem_cache_free(ext4_es_cachep, es);
+	__es_free_extent(es);
 }
 
 /*
@@ -802,10 +808,12 @@ 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);
+	es = __es_alloc_extent(false);
 	if (!es)
 		return -ENOMEM;
+	ext4_es_init_extent(inode, es, newes->es_lblk, newes->es_len,
+			    newes->es_pblk);
+
 	rb_link_node(&es->rb_node, parent, p);
 	rb_insert_color(&es->rb_node, &tree->root);
 
-- 
2.31.1

Re: [PATCH v4 03/12] ext4: factor out __es_alloc_extent() and __es_free_extent()

[Jan Kara]

On Mon 24-04-23 11:38:37, Baokun Li wrote:
> Factor out __es_alloc_extent() and __es_free_extent(), which only allocate
> and free extent_status in these two helpers.
> 
> The ext4_es_alloc_extent() function is split into __es_alloc_extent()
> and ext4_es_init_extent(). In __es_alloc_extent() we allocate memory using
> GFP_KERNEL | __GFP_NOFAIL | __GFP_ZERO if the memory allocation cannot
> fail, otherwise we use GFP_ATOMIC. and the ext4_es_init_extent() is used to
> initialize extent_status and update related variables after a successful
> allocation.
> 
> This is to prepare for the use of pre-allocated extent_status later.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 573723b23d19..18665394392f 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -461,14 +461,17 @@ static inline bool ext4_es_must_keep(struct extent_status *es)
>  	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)
> +static inline struct extent_status *__es_alloc_extent(bool nofail)
> +{
> +	if (!nofail)
> +		return kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
> +
> +	return kmem_cache_zalloc(ext4_es_cachep, GFP_KERNEL | __GFP_NOFAIL);
> +}
> +
> +static void ext4_es_init_extent(struct inode *inode, struct extent_status *es,
> +		ext4_lblk_t lblk, ext4_lblk_t len, ext4_fsblk_t pblk)
>  {
> -	struct extent_status *es;
> -	es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
> -	if (es == NULL)
> -		return NULL;
>  	es->es_lblk = lblk;
>  	es->es_len = len;
>  	es->es_pblk = pblk;
> @@ -483,8 +486,11 @@ ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>  
>  	EXT4_I(inode)->i_es_all_nr++;
>  	percpu_counter_inc(&EXT4_SB(inode->i_sb)->s_es_stats.es_stats_all_cnt);
> +}
>  
> -	return es;
> +static inline void __es_free_extent(struct extent_status *es)
> +{
> +	kmem_cache_free(ext4_es_cachep, es);
>  }
>  
>  static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
> @@ -501,7 +507,7 @@ static void ext4_es_free_extent(struct inode *inode, struct extent_status *es)
>  					s_es_stats.es_stats_shk_cnt);
>  	}
>  
> -	kmem_cache_free(ext4_es_cachep, es);
> +	__es_free_extent(es);
>  }
>  
>  /*
> @@ -802,10 +808,12 @@ 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);
> +	es = __es_alloc_extent(false);
>  	if (!es)
>  		return -ENOMEM;
> +	ext4_es_init_extent(inode, es, newes->es_lblk, newes->es_len,
> +			    newes->es_pblk);
> +
>  	rb_link_node(&es->rb_node, parent, p);
>  	rb_insert_color(&es->rb_node, &tree->root);
>  
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 04/12] ext4: use pre-allocated es in __es_insert_extent()

[Baokun Li]

Pass a extent_status pointer prealloc to __es_insert_extent(). If the
pointer is non-null, it is used directly when a new extent_status is
needed to avoid memory allocation failures.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 18665394392f..a6a62a744e83 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -144,7 +144,8 @@
 static struct kmem_cache *ext4_es_cachep;
 static struct kmem_cache *ext4_pending_cachep;
 
-static int __es_insert_extent(struct inode *inode, struct extent_status *newes);
+static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
+			      struct extent_status *prealloc);
 static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 			      ext4_lblk_t end, int *reserved);
 static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
@@ -768,7 +769,8 @@ static inline void ext4_es_insert_extent_check(struct inode *inode,
 }
 #endif
 
-static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
+static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
+			      struct extent_status *prealloc)
 {
 	struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
 	struct rb_node **p = &tree->root.rb_node;
@@ -808,7 +810,10 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
 		}
 	}
 
-	es = __es_alloc_extent(false);
+	if (prealloc)
+		es = prealloc;
+	else
+		es = __es_alloc_extent(false);
 	if (!es)
 		return -ENOMEM;
 	ext4_es_init_extent(inode, es, newes->es_lblk, newes->es_len,
@@ -868,7 +873,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 	if (err != 0)
 		goto error;
 retry:
-	err = __es_insert_extent(inode, &newes);
+	err = __es_insert_extent(inode, &newes, NULL);
 	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
 					  128, EXT4_I(inode)))
 		goto retry;
@@ -918,7 +923,7 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
 
 	es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
 	if (!es || es->es_lblk > end)
-		__es_insert_extent(inode, &newes);
+		__es_insert_extent(inode, &newes, NULL);
 	write_unlock(&EXT4_I(inode)->i_es_lock);
 }
 
@@ -1366,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 					orig_es.es_len - len2;
 			ext4_es_store_pblock_status(&newes, block,
 						    ext4_es_status(&orig_es));
-			err = __es_insert_extent(inode, &newes);
+			err = __es_insert_extent(inode, &newes, NULL);
 			if (err) {
 				es->es_lblk = orig_es.es_lblk;
 				es->es_len = orig_es.es_len;
@@ -2020,7 +2025,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 	if (err != 0)
 		goto error;
 retry:
-	err = __es_insert_extent(inode, &newes);
+	err = __es_insert_extent(inode, &newes, NULL);
 	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
 					  128, EXT4_I(inode)))
 		goto retry;
-- 
2.31.1

Re: [PATCH v4 04/12] ext4: use pre-allocated es in __es_insert_extent()

[Jan Kara]

On Mon 24-04-23 11:38:38, Baokun Li wrote:
> Pass a extent_status pointer prealloc to __es_insert_extent(). If the
> pointer is non-null, it is used directly when a new extent_status is
> needed to avoid memory allocation failures.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 18665394392f..a6a62a744e83 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -144,7 +144,8 @@
>  static struct kmem_cache *ext4_es_cachep;
>  static struct kmem_cache *ext4_pending_cachep;
>  
> -static int __es_insert_extent(struct inode *inode, struct extent_status *newes);
> +static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
> +			      struct extent_status *prealloc);
>  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  			      ext4_lblk_t end, int *reserved);
>  static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
> @@ -768,7 +769,8 @@ static inline void ext4_es_insert_extent_check(struct inode *inode,
>  }
>  #endif
>  
> -static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
> +static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
> +			      struct extent_status *prealloc)
>  {
>  	struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
>  	struct rb_node **p = &tree->root.rb_node;
> @@ -808,7 +810,10 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes)
>  		}
>  	}
>  
> -	es = __es_alloc_extent(false);
> +	if (prealloc)
> +		es = prealloc;
> +	else
> +		es = __es_alloc_extent(false);
>  	if (!es)
>  		return -ENOMEM;
>  	ext4_es_init_extent(inode, es, newes->es_lblk, newes->es_len,
> @@ -868,7 +873,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	if (err != 0)
>  		goto error;
>  retry:
> -	err = __es_insert_extent(inode, &newes);
> +	err = __es_insert_extent(inode, &newes, NULL);
>  	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
>  					  128, EXT4_I(inode)))
>  		goto retry;
> @@ -918,7 +923,7 @@ void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>  
>  	es = __es_tree_search(&EXT4_I(inode)->i_es_tree.root, lblk);
>  	if (!es || es->es_lblk > end)
> -		__es_insert_extent(inode, &newes);
> +		__es_insert_extent(inode, &newes, NULL);
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
>  }
>  
> @@ -1366,7 +1371,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  					orig_es.es_len - len2;
>  			ext4_es_store_pblock_status(&newes, block,
>  						    ext4_es_status(&orig_es));
> -			err = __es_insert_extent(inode, &newes);
> +			err = __es_insert_extent(inode, &newes, NULL);
>  			if (err) {
>  				es->es_lblk = orig_es.es_lblk;
>  				es->es_len = orig_es.es_len;
> @@ -2020,7 +2025,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  	if (err != 0)
>  		goto error;
>  retry:
> -	err = __es_insert_extent(inode, &newes);
> +	err = __es_insert_extent(inode, &newes, NULL);
>  	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
>  					  128, EXT4_I(inode)))
>  		goto retry;
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 05/12] ext4: use pre-allocated es in __es_remove_extent()

[Baokun Li]

When splitting extent, if the second extent can not be dropped, we return
-ENOMEM and use GFP_NOFAIL to preallocate an extent_status outside of
i_es_lock and pass it to __es_remove_extent() to be used as the second
extent. This ensures that __es_remove_extent() is executed successfully,
thus ensuring consistency in the extent status tree. If the second extent
is not undroppable, we simply drop it and return 0. Then retry is no longer
necessary, remove it.

Now, __es_remove_extent() will always remove what it should, maybe more.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index a6a62a744e83..7219116e0d68 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -147,7 +147,8 @@ static struct kmem_cache *ext4_pending_cachep;
 static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
 			      struct extent_status *prealloc);
 static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
-			      ext4_lblk_t end, int *reserved);
+			      ext4_lblk_t end, int *reserved,
+			      struct extent_status *prealloc);
 static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
 static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
 		       struct ext4_inode_info *locked_ei);
@@ -869,7 +870,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 	ext4_es_insert_extent_check(inode, &newes);
 
 	write_lock(&EXT4_I(inode)->i_es_lock);
-	err = __es_remove_extent(inode, lblk, end, NULL);
+	err = __es_remove_extent(inode, lblk, end, NULL, NULL);
 	if (err != 0)
 		goto error;
 retry:
@@ -1315,6 +1316,7 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
  * @lblk - first block in range
  * @end - last block in range
  * @reserved - number of cluster reservations released
+ * @prealloc - pre-allocated es to avoid memory allocation failures
  *
  * If @reserved is not NULL and delayed allocation is enabled, counts
  * block/cluster reservations freed by removing range and if bigalloc
@@ -1322,7 +1324,8 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
  * error code on failure.
  */
 static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
-			      ext4_lblk_t end, int *reserved)
+			      ext4_lblk_t end, int *reserved,
+			      struct extent_status *prealloc)
 {
 	struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
 	struct rb_node *node;
@@ -1330,14 +1333,12 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	struct extent_status orig_es;
 	ext4_lblk_t len1, len2;
 	ext4_fsblk_t block;
-	int err;
+	int err = 0;
 	bool count_reserved = true;
 	struct rsvd_count rc;
 
 	if (reserved == NULL || !test_opt(inode->i_sb, DELALLOC))
 		count_reserved = false;
-retry:
-	err = 0;
 
 	es = __es_tree_search(&tree->root, lblk);
 	if (!es)
@@ -1371,14 +1372,13 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 					orig_es.es_len - len2;
 			ext4_es_store_pblock_status(&newes, block,
 						    ext4_es_status(&orig_es));
-			err = __es_insert_extent(inode, &newes, NULL);
+			err = __es_insert_extent(inode, &newes, prealloc);
 			if (err) {
+				if (!ext4_es_must_keep(&newes))
+					return 0;
+
 				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)))
-					goto retry;
 				goto out;
 			}
 		} else {
@@ -1478,7 +1478,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	 * is reclaimed.
 	 */
 	write_lock(&EXT4_I(inode)->i_es_lock);
-	err = __es_remove_extent(inode, lblk, end, &reserved);
+	err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
 	write_unlock(&EXT4_I(inode)->i_es_lock);
 	ext4_es_print_tree(inode);
 	ext4_da_release_space(inode, reserved);
@@ -2021,7 +2021,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 
 	write_lock(&EXT4_I(inode)->i_es_lock);
 
-	err = __es_remove_extent(inode, lblk, lblk, NULL);
+	err = __es_remove_extent(inode, lblk, lblk, NULL, NULL);
 	if (err != 0)
 		goto error;
 retry:
-- 
2.31.1

Re: [PATCH v4 05/12] ext4: use pre-allocated es in __es_remove_extent()

[Jan Kara]

On Mon 24-04-23 11:38:39, Baokun Li wrote:
> When splitting extent, if the second extent can not be dropped, we return
> -ENOMEM and use GFP_NOFAIL to preallocate an extent_status outside of
> i_es_lock and pass it to __es_remove_extent() to be used as the second
> extent. This ensures that __es_remove_extent() is executed successfully,
> thus ensuring consistency in the extent status tree. If the second extent
> is not undroppable, we simply drop it and return 0. Then retry is no longer
> necessary, remove it.
> 
> Now, __es_remove_extent() will always remove what it should, maybe more.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index a6a62a744e83..7219116e0d68 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -147,7 +147,8 @@ static struct kmem_cache *ext4_pending_cachep;
>  static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
>  			      struct extent_status *prealloc);
>  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -			      ext4_lblk_t end, int *reserved);
> +			      ext4_lblk_t end, int *reserved,
> +			      struct extent_status *prealloc);
>  static int es_reclaim_extents(struct ext4_inode_info *ei, int *nr_to_scan);
>  static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
>  		       struct ext4_inode_info *locked_ei);
> @@ -869,7 +870,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	ext4_es_insert_extent_check(inode, &newes);
>  
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, NULL);
> +	err = __es_remove_extent(inode, lblk, end, NULL, NULL);
>  	if (err != 0)
>  		goto error;
>  retry:
> @@ -1315,6 +1316,7 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
>   * @lblk - first block in range
>   * @end - last block in range
>   * @reserved - number of cluster reservations released
> + * @prealloc - pre-allocated es to avoid memory allocation failures
>   *
>   * If @reserved is not NULL and delayed allocation is enabled, counts
>   * block/cluster reservations freed by removing range and if bigalloc
> @@ -1322,7 +1324,8 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
>   * error code on failure.
>   */
>  static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -			      ext4_lblk_t end, int *reserved)
> +			      ext4_lblk_t end, int *reserved,
> +			      struct extent_status *prealloc)
>  {
>  	struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
>  	struct rb_node *node;
> @@ -1330,14 +1333,12 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	struct extent_status orig_es;
>  	ext4_lblk_t len1, len2;
>  	ext4_fsblk_t block;
> -	int err;
> +	int err = 0;
>  	bool count_reserved = true;
>  	struct rsvd_count rc;
>  
>  	if (reserved == NULL || !test_opt(inode->i_sb, DELALLOC))
>  		count_reserved = false;
> -retry:
> -	err = 0;
>  
>  	es = __es_tree_search(&tree->root, lblk);
>  	if (!es)
> @@ -1371,14 +1372,13 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  					orig_es.es_len - len2;
>  			ext4_es_store_pblock_status(&newes, block,
>  						    ext4_es_status(&orig_es));
> -			err = __es_insert_extent(inode, &newes, NULL);
> +			err = __es_insert_extent(inode, &newes, prealloc);
>  			if (err) {
> +				if (!ext4_es_must_keep(&newes))
> +					return 0;
> +
>  				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)))
> -					goto retry;
>  				goto out;
>  			}
>  		} else {
> @@ -1478,7 +1478,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	 * is reclaimed.
>  	 */
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, &reserved);
> +	err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
>  	ext4_es_print_tree(inode);
>  	ext4_da_release_space(inode, reserved);
> @@ -2021,7 +2021,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  
>  	write_lock(&EXT4_I(inode)->i_es_lock);
>  
> -	err = __es_remove_extent(inode, lblk, lblk, NULL);
> +	err = __es_remove_extent(inode, lblk, lblk, NULL, NULL);
>  	if (err != 0)
>  		goto error;
>  retry:
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 06/12] ext4: using nofail preallocation in ext4_es_remove_extent()

[Baokun Li]

If __es_remove_extent() returns an error it means that when splitting
extent, allocating an extent that must be kept failed, where returning
an error directly would cause the extent tree to be inconsistent. So we
use GFP_NOFAIL to pre-allocate an extent_status and pass it to
__es_remove_extent() to avoid this problem.

In addition, since the allocated memory is outside the i_es_lock, the
extent_status tree may change and the pre-allocated extent_status is
no longer needed, so we release the pre-allocated extent_status when
es->es_len is not initialized.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 7219116e0d68..f4d50cd501fc 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1458,6 +1458,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	ext4_lblk_t end;
 	int err = 0;
 	int reserved = 0;
+	struct extent_status *es = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
 		return 0;
@@ -1472,17 +1473,25 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	end = lblk + len - 1;
 	BUG_ON(end < lblk);
 
+retry:
+	if (err && !es)
+		es = __es_alloc_extent(true);
 	/*
 	 * ext4_clear_inode() depends on us taking i_es_lock unconditionally
 	 * so that we are sure __es_shrink() is done with the inode before it
 	 * is reclaimed.
 	 */
 	write_lock(&EXT4_I(inode)->i_es_lock);
-	err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
+	err = __es_remove_extent(inode, lblk, end, &reserved, es);
+	if (es && !es->es_len)
+		__es_free_extent(es);
 	write_unlock(&EXT4_I(inode)->i_es_lock);
+	if (err)
+		goto retry;
+
 	ext4_es_print_tree(inode);
 	ext4_da_release_space(inode, reserved);
-	return err;
+	return 0;
 }
 
 static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
-- 
2.31.1

Re: [PATCH v4 06/12] ext4: using nofail preallocation in ext4_es_remove_extent()

[Jan Kara]

On Mon 24-04-23 11:38:40, Baokun Li wrote:
> If __es_remove_extent() returns an error it means that when splitting
> extent, allocating an extent that must be kept failed, where returning
> an error directly would cause the extent tree to be inconsistent. So we
> use GFP_NOFAIL to pre-allocate an extent_status and pass it to
> __es_remove_extent() to avoid this problem.
> 
> In addition, since the allocated memory is outside the i_es_lock, the
> extent_status tree may change and the pre-allocated extent_status is
> no longer needed, so we release the pre-allocated extent_status when
> es->es_len is not initialized.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7219116e0d68..f4d50cd501fc 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1458,6 +1458,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	ext4_lblk_t end;
>  	int err = 0;
>  	int reserved = 0;
> +	struct extent_status *es = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
>  		return 0;
> @@ -1472,17 +1473,25 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	end = lblk + len - 1;
>  	BUG_ON(end < lblk);
>  
> +retry:
> +	if (err && !es)
> +		es = __es_alloc_extent(true);
>  	/*
>  	 * ext4_clear_inode() depends on us taking i_es_lock unconditionally
>  	 * so that we are sure __es_shrink() is done with the inode before it
>  	 * is reclaimed.
>  	 */
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, &reserved, NULL);
> +	err = __es_remove_extent(inode, lblk, end, &reserved, es);
> +	if (es && !es->es_len)
> +		__es_free_extent(es);
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
> +	if (err)
> +		goto retry;
> +
>  	ext4_es_print_tree(inode);
>  	ext4_da_release_space(inode, reserved);
> -	return err;
> +	return 0;
>  }
>  
>  static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 07/12] ext4: using nofail preallocation in ext4_es_insert_delayed_block()

[Baokun Li]

Similar to in ext4_es_remove_extent(), we use a no-fail preallocation
to avoid inconsistencies, except that here we may have to preallocate
two extent_status.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 33 ++++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index f4d50cd501fc..f892277155fa 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -2013,7 +2013,10 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 				 bool allocated)
 {
 	struct extent_status newes;
-	int err = 0;
+	int err1 = 0;
+	int err2 = 0;
+	struct extent_status *es1 = NULL;
+	struct extent_status *es2 = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
 		return 0;
@@ -2028,29 +2031,37 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 
 	ext4_es_insert_extent_check(inode, &newes);
 
+retry:
+	if (err1 && !es1)
+		es1 = __es_alloc_extent(true);
+	if ((err1 || err2) && !es2)
+		es2 = __es_alloc_extent(true);
 	write_lock(&EXT4_I(inode)->i_es_lock);
 
-	err = __es_remove_extent(inode, lblk, lblk, NULL, NULL);
-	if (err != 0)
+	err1 = __es_remove_extent(inode, lblk, lblk, NULL, es1);
+	if (err1 != 0)
 		goto error;
-retry:
-	err = __es_insert_extent(inode, &newes, NULL);
-	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
-					  128, EXT4_I(inode)))
-		goto retry;
-	if (err != 0)
+
+	err2 = __es_insert_extent(inode, &newes, es2);
+	if (err2 != 0)
 		goto error;
 
 	if (allocated)
 		__insert_pending(inode, lblk);
 
+	/* es is pre-allocated but not used, free it. */
+	if (es1 && !es1->es_len)
+		__es_free_extent(es1);
+	if (es2 && !es2->es_len)
+		__es_free_extent(es2);
 error:
 	write_unlock(&EXT4_I(inode)->i_es_lock);
+	if (err1 || err2)
+		goto retry;
 
 	ext4_es_print_tree(inode);
 	ext4_print_pending_tree(inode);
-
-	return err;
+	return 0;
 }
 
 /*
-- 
2.31.1

Re: [PATCH v4 07/12] ext4: using nofail preallocation in ext4_es_insert_delayed_block()

[Jan Kara]

On Mon 24-04-23 11:38:41, Baokun Li wrote:
> Similar to in ext4_es_remove_extent(), we use a no-fail preallocation
> to avoid inconsistencies, except that here we may have to preallocate
> two extent_status.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 33 ++++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index f4d50cd501fc..f892277155fa 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -2013,7 +2013,10 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  				 bool allocated)
>  {
>  	struct extent_status newes;
> -	int err = 0;
> +	int err1 = 0;
> +	int err2 = 0;
> +	struct extent_status *es1 = NULL;
> +	struct extent_status *es2 = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
>  		return 0;
> @@ -2028,29 +2031,37 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  
>  	ext4_es_insert_extent_check(inode, &newes);
>  
> +retry:
> +	if (err1 && !es1)
> +		es1 = __es_alloc_extent(true);
> +	if ((err1 || err2) && !es2)
> +		es2 = __es_alloc_extent(true);
>  	write_lock(&EXT4_I(inode)->i_es_lock);
>  
> -	err = __es_remove_extent(inode, lblk, lblk, NULL, NULL);
> -	if (err != 0)
> +	err1 = __es_remove_extent(inode, lblk, lblk, NULL, es1);
> +	if (err1 != 0)
>  		goto error;
> -retry:
> -	err = __es_insert_extent(inode, &newes, NULL);
> -	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
> -					  128, EXT4_I(inode)))
> -		goto retry;
> -	if (err != 0)
> +
> +	err2 = __es_insert_extent(inode, &newes, es2);
> +	if (err2 != 0)
>  		goto error;
>  
>  	if (allocated)
>  		__insert_pending(inode, lblk);
>  
> +	/* es is pre-allocated but not used, free it. */
> +	if (es1 && !es1->es_len)
> +		__es_free_extent(es1);
> +	if (es2 && !es2->es_len)
> +		__es_free_extent(es2);
>  error:
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
> +	if (err1 || err2)
> +		goto retry;
>  
>  	ext4_es_print_tree(inode);
>  	ext4_print_pending_tree(inode);
> -
> -	return err;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 08/12] ext4: using nofail preallocation in ext4_es_insert_extent()

[Baokun Li]

Similar to in ext4_es_insert_delayed_block(), we use preallocations that
do not fail to avoid inconsistencies, but we do not care about es that are
not must be kept, and we return 0 even if such es memory allocation fails.

Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 38 ++++++++++++++++++++++++++------------
 1 file changed, 26 insertions(+), 12 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index f892277155fa..91828cf7395b 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -840,8 +840,11 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 {
 	struct extent_status newes;
 	ext4_lblk_t end = lblk + len - 1;
-	int err = 0;
+	int err1 = 0;
+	int err2 = 0;
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
+	struct extent_status *es1 = NULL;
+	struct extent_status *es2 = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
 		return 0;
@@ -869,29 +872,40 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 
 	ext4_es_insert_extent_check(inode, &newes);
 
+retry:
+	if (err1 && !es1)
+		es1 = __es_alloc_extent(true);
+	if ((err1 || err2) && !es2)
+		es2 = __es_alloc_extent(true);
 	write_lock(&EXT4_I(inode)->i_es_lock);
-	err = __es_remove_extent(inode, lblk, end, NULL, NULL);
-	if (err != 0)
+
+	err1 = __es_remove_extent(inode, lblk, end, NULL, es1);
+	if (err1 != 0)
+		goto error;
+
+	err2 = __es_insert_extent(inode, &newes, es2);
+	if (err2 == -ENOMEM && !ext4_es_must_keep(&newes))
+		err2 = 0;
+	if (err2 != 0)
 		goto error;
-retry:
-	err = __es_insert_extent(inode, &newes, NULL);
-	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
-					  128, EXT4_I(inode)))
-		goto retry;
-	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
-		err = 0;
 
 	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
 	    (status & EXTENT_STATUS_WRITTEN ||
 	     status & EXTENT_STATUS_UNWRITTEN))
 		__revise_pending(inode, lblk, len);
 
+	/* es is pre-allocated but not used, free it. */
+	if (es1 && !es1->es_len)
+		__es_free_extent(es1);
+	if (es2 && !es2->es_len)
+		__es_free_extent(es2);
 error:
 	write_unlock(&EXT4_I(inode)->i_es_lock);
+	if (err1 || err2)
+		goto retry;
 
 	ext4_es_print_tree(inode);
-
-	return err;
+	return 0;
 }
 
 /*
-- 
2.31.1

Re: [PATCH v4 08/12] ext4: using nofail preallocation in ext4_es_insert_extent()

[Jan Kara]

On Mon 24-04-23 11:38:42, Baokun Li wrote:
> Similar to in ext4_es_insert_delayed_block(), we use preallocations that
> do not fail to avoid inconsistencies, but we do not care about es that are
> not must be kept, and we return 0 even if such es memory allocation fails.
> 
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 38 ++++++++++++++++++++++++++------------
>  1 file changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index f892277155fa..91828cf7395b 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -840,8 +840,11 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  {
>  	struct extent_status newes;
>  	ext4_lblk_t end = lblk + len - 1;
> -	int err = 0;
> +	int err1 = 0;
> +	int err2 = 0;
>  	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> +	struct extent_status *es1 = NULL;
> +	struct extent_status *es2 = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
>  		return 0;
> @@ -869,29 +872,40 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  
>  	ext4_es_insert_extent_check(inode, &newes);
>  
> +retry:
> +	if (err1 && !es1)
> +		es1 = __es_alloc_extent(true);
> +	if ((err1 || err2) && !es2)
> +		es2 = __es_alloc_extent(true);
>  	write_lock(&EXT4_I(inode)->i_es_lock);
> -	err = __es_remove_extent(inode, lblk, end, NULL, NULL);
> -	if (err != 0)
> +
> +	err1 = __es_remove_extent(inode, lblk, end, NULL, es1);
> +	if (err1 != 0)
> +		goto error;
> +
> +	err2 = __es_insert_extent(inode, &newes, es2);
> +	if (err2 == -ENOMEM && !ext4_es_must_keep(&newes))
> +		err2 = 0;
> +	if (err2 != 0)
>  		goto error;
> -retry:
> -	err = __es_insert_extent(inode, &newes, NULL);
> -	if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
> -					  128, EXT4_I(inode)))
> -		goto retry;
> -	if (err == -ENOMEM && !ext4_es_must_keep(&newes))
> -		err = 0;
>  
>  	if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
>  	    (status & EXTENT_STATUS_WRITTEN ||
>  	     status & EXTENT_STATUS_UNWRITTEN))
>  		__revise_pending(inode, lblk, len);
>  
> +	/* es is pre-allocated but not used, free it. */
> +	if (es1 && !es1->es_len)
> +		__es_free_extent(es1);
> +	if (es2 && !es2->es_len)
> +		__es_free_extent(es2);
>  error:
>  	write_unlock(&EXT4_I(inode)->i_es_lock);
> +	if (err1 || err2)
> +		goto retry;
>  
>  	ext4_es_print_tree(inode);
> -
> -	return err;
> +	return 0;
>  }
>  
>  /*
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 09/12] ext4: make ext4_es_remove_extent() return void

[Baokun Li]

Now ext4_es_remove_extent() never fails, so make it return void.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c        | 34 ++++++----------------------------
 fs/ext4/extents_status.c | 12 ++++++------
 fs/ext4/extents_status.h |  4 ++--
 fs/ext4/inline.c         | 12 ++----------
 fs/ext4/inode.c          |  8 ++------
 5 files changed, 18 insertions(+), 52 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 3559ea6b0781..e6695fec59af 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -4403,15 +4403,8 @@ int ext4_ext_truncate(handle_t *handle, struct inode *inode)
 
 	last_block = (inode->i_size + sb->s_blocksize - 1)
 			>> EXT4_BLOCK_SIZE_BITS(sb);
-retry:
-	err = ext4_es_remove_extent(inode, last_block,
-				    EXT_MAX_BLOCKS - last_block);
-	if (err == -ENOMEM) {
-		memalloc_retry_wait(GFP_ATOMIC);
-		goto retry;
-	}
-	if (err)
-		return err;
+	ext4_es_remove_extent(inode, last_block, EXT_MAX_BLOCKS - last_block);
+
 retry_remove_space:
 	err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
 	if (err == -ENOMEM) {
@@ -5363,13 +5356,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
 
 	down_write(&EXT4_I(inode)->i_data_sem);
 	ext4_discard_preallocations(inode, 0);
-
-	ret = ext4_es_remove_extent(inode, punch_start,
-				    EXT_MAX_BLOCKS - punch_start);
-	if (ret) {
-		up_write(&EXT4_I(inode)->i_data_sem);
-		goto out_stop;
-	}
+	ext4_es_remove_extent(inode, punch_start, EXT_MAX_BLOCKS - punch_start);
 
 	ret = ext4_ext_remove_space(inode, punch_start, punch_stop - 1);
 	if (ret) {
@@ -5554,12 +5541,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
 		ext4_free_ext_path(path);
 	}
 
-	ret = ext4_es_remove_extent(inode, offset_lblk,
-			EXT_MAX_BLOCKS - offset_lblk);
-	if (ret) {
-		up_write(&EXT4_I(inode)->i_data_sem);
-		goto out_stop;
-	}
+	ext4_es_remove_extent(inode, offset_lblk, EXT_MAX_BLOCKS - offset_lblk);
 
 	/*
 	 * if offset_lblk lies in a hole which is at start of file, use
@@ -5617,12 +5599,8 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
 	BUG_ON(!inode_is_locked(inode1));
 	BUG_ON(!inode_is_locked(inode2));
 
-	*erp = ext4_es_remove_extent(inode1, lblk1, count);
-	if (unlikely(*erp))
-		return 0;
-	*erp = ext4_es_remove_extent(inode2, lblk2, count);
-	if (unlikely(*erp))
-		return 0;
+	ext4_es_remove_extent(inode1, lblk1, count);
+	ext4_es_remove_extent(inode2, lblk2, count);
 
 	while (count) {
 		struct ext4_extent *ex1, *ex2, tmp_ex;
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 91828cf7395b..2a394c40f4b7 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1464,10 +1464,10 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
  * @len - number of blocks to remove
  *
  * Reduces block/cluster reservation count and for bigalloc cancels pending
- * reservations as needed. Returns 0 on success, error code on failure.
+ * reservations as needed.
  */
-int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
-			  ext4_lblk_t len)
+void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
+			   ext4_lblk_t len)
 {
 	ext4_lblk_t end;
 	int err = 0;
@@ -1475,14 +1475,14 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 	struct extent_status *es = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
-		return 0;
+		return;
 
 	trace_ext4_es_remove_extent(inode, lblk, len);
 	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
 		 lblk, len, inode->i_ino);
 
 	if (!len)
-		return err;
+		return;
 
 	end = lblk + len - 1;
 	BUG_ON(end < lblk);
@@ -1505,7 +1505,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
 
 	ext4_es_print_tree(inode);
 	ext4_da_release_space(inode, reserved);
-	return 0;
+	return;
 }
 
 static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
index 4ec30a798260..526a68890aa6 100644
--- a/fs/ext4/extents_status.h
+++ b/fs/ext4/extents_status.h
@@ -133,8 +133,8 @@ extern int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
 				 ext4_lblk_t len, ext4_fsblk_t pblk,
 				 unsigned int status);
-extern int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
-				 ext4_lblk_t len);
+extern void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
+				  ext4_lblk_t len);
 extern void ext4_es_find_extent_range(struct inode *inode,
 				      int (*match_fn)(struct extent_status *es),
 				      ext4_lblk_t lblk, ext4_lblk_t end,
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index b9fb1177fff6..d58cd0331474 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1952,16 +1952,8 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
 		 * the extent status cache must be cleared to avoid leaving
 		 * behind stale delayed allocated extent entries
 		 */
-		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
-retry:
-			err = ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
-			if (err == -ENOMEM) {
-				memalloc_retry_wait(GFP_ATOMIC);
-				goto retry;
-			}
-			if (err)
-				goto out_error;
-		}
+		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
+			ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
 
 		/* Clear the content in the xattr space. */
 		if (inline_size > EXT4_MIN_INLINE_DATA_SIZE) {
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 20de04399c8b..a0bfe77d5537 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4034,12 +4034,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
 		down_write(&EXT4_I(inode)->i_data_sem);
 		ext4_discard_preallocations(inode, 0);
 
-		ret = ext4_es_remove_extent(inode, first_block,
-					    stop_block - first_block);
-		if (ret) {
-			up_write(&EXT4_I(inode)->i_data_sem);
-			goto out_stop;
-		}
+		ext4_es_remove_extent(inode, first_block,
+				      stop_block - first_block);
 
 		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
 			ret = ext4_ext_remove_space(inode, first_block,
-- 
2.31.1

Re: [PATCH v4 09/12] ext4: make ext4_es_remove_extent() return void

[Jan Kara]

On Mon 24-04-23 11:38:43, Baokun Li wrote:
> Now ext4_es_remove_extent() never fails, so make it return void.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Nice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c        | 34 ++++++----------------------------
>  fs/ext4/extents_status.c | 12 ++++++------
>  fs/ext4/extents_status.h |  4 ++--
>  fs/ext4/inline.c         | 12 ++----------
>  fs/ext4/inode.c          |  8 ++------
>  5 files changed, 18 insertions(+), 52 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 3559ea6b0781..e6695fec59af 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4403,15 +4403,8 @@ int ext4_ext_truncate(handle_t *handle, struct inode *inode)
>  
>  	last_block = (inode->i_size + sb->s_blocksize - 1)
>  			>> EXT4_BLOCK_SIZE_BITS(sb);
> -retry:
> -	err = ext4_es_remove_extent(inode, last_block,
> -				    EXT_MAX_BLOCKS - last_block);
> -	if (err == -ENOMEM) {
> -		memalloc_retry_wait(GFP_ATOMIC);
> -		goto retry;
> -	}
> -	if (err)
> -		return err;
> +	ext4_es_remove_extent(inode, last_block, EXT_MAX_BLOCKS - last_block);
> +
>  retry_remove_space:
>  	err = ext4_ext_remove_space(inode, last_block, EXT_MAX_BLOCKS - 1);
>  	if (err == -ENOMEM) {
> @@ -5363,13 +5356,7 @@ static int ext4_collapse_range(struct file *file, loff_t offset, loff_t len)
>  
>  	down_write(&EXT4_I(inode)->i_data_sem);
>  	ext4_discard_preallocations(inode, 0);
> -
> -	ret = ext4_es_remove_extent(inode, punch_start,
> -				    EXT_MAX_BLOCKS - punch_start);
> -	if (ret) {
> -		up_write(&EXT4_I(inode)->i_data_sem);
> -		goto out_stop;
> -	}
> +	ext4_es_remove_extent(inode, punch_start, EXT_MAX_BLOCKS - punch_start);
>  
>  	ret = ext4_ext_remove_space(inode, punch_start, punch_stop - 1);
>  	if (ret) {
> @@ -5554,12 +5541,7 @@ static int ext4_insert_range(struct file *file, loff_t offset, loff_t len)
>  		ext4_free_ext_path(path);
>  	}
>  
> -	ret = ext4_es_remove_extent(inode, offset_lblk,
> -			EXT_MAX_BLOCKS - offset_lblk);
> -	if (ret) {
> -		up_write(&EXT4_I(inode)->i_data_sem);
> -		goto out_stop;
> -	}
> +	ext4_es_remove_extent(inode, offset_lblk, EXT_MAX_BLOCKS - offset_lblk);
>  
>  	/*
>  	 * if offset_lblk lies in a hole which is at start of file, use
> @@ -5617,12 +5599,8 @@ ext4_swap_extents(handle_t *handle, struct inode *inode1,
>  	BUG_ON(!inode_is_locked(inode1));
>  	BUG_ON(!inode_is_locked(inode2));
>  
> -	*erp = ext4_es_remove_extent(inode1, lblk1, count);
> -	if (unlikely(*erp))
> -		return 0;
> -	*erp = ext4_es_remove_extent(inode2, lblk2, count);
> -	if (unlikely(*erp))
> -		return 0;
> +	ext4_es_remove_extent(inode1, lblk1, count);
> +	ext4_es_remove_extent(inode2, lblk2, count);
>  
>  	while (count) {
>  		struct ext4_extent *ex1, *ex2, tmp_ex;
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 91828cf7395b..2a394c40f4b7 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -1464,10 +1464,10 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>   * @len - number of blocks to remove
>   *
>   * Reduces block/cluster reservation count and for bigalloc cancels pending
> - * reservations as needed. Returns 0 on success, error code on failure.
> + * reservations as needed.
>   */
> -int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -			  ext4_lblk_t len)
> +void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> +			   ext4_lblk_t len)
>  {
>  	ext4_lblk_t end;
>  	int err = 0;
> @@ -1475,14 +1475,14 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  	struct extent_status *es = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
> -		return 0;
> +		return;
>  
>  	trace_ext4_es_remove_extent(inode, lblk, len);
>  	es_debug("remove [%u/%u) from extent status tree of inode %lu\n",
>  		 lblk, len, inode->i_ino);
>  
>  	if (!len)
> -		return err;
> +		return;
>  
>  	end = lblk + len - 1;
>  	BUG_ON(end < lblk);
> @@ -1505,7 +1505,7 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
>  
>  	ext4_es_print_tree(inode);
>  	ext4_da_release_space(inode, reserved);
> -	return 0;
> +	return;
>  }
>  
>  static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> index 4ec30a798260..526a68890aa6 100644
> --- a/fs/ext4/extents_status.h
> +++ b/fs/ext4/extents_status.h
> @@ -133,8 +133,8 @@ extern int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>  				 ext4_lblk_t len, ext4_fsblk_t pblk,
>  				 unsigned int status);
> -extern int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> -				 ext4_lblk_t len);
> +extern void ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
> +				  ext4_lblk_t len);
>  extern void ext4_es_find_extent_range(struct inode *inode,
>  				      int (*match_fn)(struct extent_status *es),
>  				      ext4_lblk_t lblk, ext4_lblk_t end,
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index b9fb1177fff6..d58cd0331474 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1952,16 +1952,8 @@ int ext4_inline_data_truncate(struct inode *inode, int *has_inline)
>  		 * the extent status cache must be cleared to avoid leaving
>  		 * behind stale delayed allocated extent entries
>  		 */
> -		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA)) {
> -retry:
> -			err = ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
> -			if (err == -ENOMEM) {
> -				memalloc_retry_wait(GFP_ATOMIC);
> -				goto retry;
> -			}
> -			if (err)
> -				goto out_error;
> -		}
> +		if (!ext4_test_inode_state(inode, EXT4_STATE_MAY_INLINE_DATA))
> +			ext4_es_remove_extent(inode, 0, EXT_MAX_BLOCKS);
>  
>  		/* Clear the content in the xattr space. */
>  		if (inline_size > EXT4_MIN_INLINE_DATA_SIZE) {
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 20de04399c8b..a0bfe77d5537 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4034,12 +4034,8 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  		down_write(&EXT4_I(inode)->i_data_sem);
>  		ext4_discard_preallocations(inode, 0);
>  
> -		ret = ext4_es_remove_extent(inode, first_block,
> -					    stop_block - first_block);
> -		if (ret) {
> -			up_write(&EXT4_I(inode)->i_data_sem);
> -			goto out_stop;
> -		}
> +		ext4_es_remove_extent(inode, first_block,
> +				      stop_block - first_block);
>  
>  		if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
>  			ret = ext4_ext_remove_space(inode, first_block,
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Baokun Li]

Now it never fails when inserting a delay extent, so the return value in
ext4_es_insert_delayed_block is no longer necessary, let it return void.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents_status.c | 10 ++++------
 fs/ext4/extents_status.h |  4 ++--
 fs/ext4/inode.c          | 10 ++--------
 3 files changed, 8 insertions(+), 16 deletions(-)

diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 2a394c40f4b7..b12c5cfdf601 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -2020,11 +2020,9 @@ bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk)
  * @lblk - logical block to be added
  * @allocated - indicates whether a physical cluster has been allocated for
  *              the logical cluster that contains the block
- *
- * Returns 0 on success, negative error code on failure.
  */
-int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
-				 bool allocated)
+void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
+				  bool allocated)
 {
 	struct extent_status newes;
 	int err1 = 0;
@@ -2033,7 +2031,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 	struct extent_status *es2 = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
-		return 0;
+		return;
 
 	es_debug("add [%u/1) delayed to extent status tree of inode %lu\n",
 		 lblk, inode->i_ino);
@@ -2075,7 +2073,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
 
 	ext4_es_print_tree(inode);
 	ext4_print_pending_tree(inode);
-	return 0;
+	return;
 }
 
 /*
diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
index 526a68890aa6..c22edb931f1b 100644
--- a/fs/ext4/extents_status.h
+++ b/fs/ext4/extents_status.h
@@ -249,8 +249,8 @@ extern void ext4_exit_pending(void);
 extern void ext4_init_pending_tree(struct ext4_pending_tree *tree);
 extern void ext4_remove_pending(struct inode *inode, ext4_lblk_t lblk);
 extern bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk);
-extern int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
-					bool allocated);
+extern void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
+					 bool allocated);
 extern unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk,
 					ext4_lblk_t len);
 extern void ext4_clear_inode_es(struct inode *inode);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index a0bfe77d5537..4221b2dafeb5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1641,9 +1641,8 @@ static void ext4_print_free_blocks(struct inode *inode)
 static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 {
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
-	int ret;
+	int ret = 0;
 	bool allocated = false;
-	bool reserved = false;
 
 	/*
 	 * If the cluster containing lblk is shared with a delayed,
@@ -1660,7 +1659,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		ret = ext4_da_reserve_space(inode);
 		if (ret != 0)   /* ENOSPC */
 			goto errout;
-		reserved = true;
 	} else {   /* bigalloc */
 		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
 			if (!ext4_es_scan_clu(inode,
@@ -1673,7 +1671,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 					ret = ext4_da_reserve_space(inode);
 					if (ret != 0)   /* ENOSPC */
 						goto errout;
-					reserved = true;
 				} else {
 					allocated = true;
 				}
@@ -1683,10 +1680,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		}
 	}
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
 errout:
 	return ret;
 }
-- 
2.31.1

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Jan Kara]

On Mon 24-04-23 11:38:44, Baokun Li wrote:
> Now it never fails when inserting a delay extent, so the return value in
> ext4_es_insert_delayed_block is no longer necessary, let it return void.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Nice. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents_status.c | 10 ++++------
>  fs/ext4/extents_status.h |  4 ++--
>  fs/ext4/inode.c          | 10 ++--------
>  3 files changed, 8 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 2a394c40f4b7..b12c5cfdf601 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -2020,11 +2020,9 @@ bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk)
>   * @lblk - logical block to be added
>   * @allocated - indicates whether a physical cluster has been allocated for
>   *              the logical cluster that contains the block
> - *
> - * Returns 0 on success, negative error code on failure.
>   */
> -int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> -				 bool allocated)
> +void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> +				  bool allocated)
>  {
>  	struct extent_status newes;
>  	int err1 = 0;
> @@ -2033,7 +2031,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  	struct extent_status *es2 = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
> -		return 0;
> +		return;
>  
>  	es_debug("add [%u/1) delayed to extent status tree of inode %lu\n",
>  		 lblk, inode->i_ino);
> @@ -2075,7 +2073,7 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
>  
>  	ext4_es_print_tree(inode);
>  	ext4_print_pending_tree(inode);
> -	return 0;
> +	return;
>  }
>  
>  /*
> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> index 526a68890aa6..c22edb931f1b 100644
> --- a/fs/ext4/extents_status.h
> +++ b/fs/ext4/extents_status.h
> @@ -249,8 +249,8 @@ extern void ext4_exit_pending(void);
>  extern void ext4_init_pending_tree(struct ext4_pending_tree *tree);
>  extern void ext4_remove_pending(struct inode *inode, ext4_lblk_t lblk);
>  extern bool ext4_is_pending(struct inode *inode, ext4_lblk_t lblk);
> -extern int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> -					bool allocated);
> +extern void ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
> +					 bool allocated);
>  extern unsigned int ext4_es_delayed_clu(struct inode *inode, ext4_lblk_t lblk,
>  					ext4_lblk_t len);
>  extern void ext4_clear_inode_es(struct inode *inode);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index a0bfe77d5537..4221b2dafeb5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1641,9 +1641,8 @@ static void ext4_print_free_blocks(struct inode *inode)
>  static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> -	int ret;
> +	int ret = 0;
>  	bool allocated = false;
> -	bool reserved = false;
>  
>  	/*
>  	 * If the cluster containing lblk is shared with a delayed,
> @@ -1660,7 +1659,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>  		ret = ext4_da_reserve_space(inode);
>  		if (ret != 0)   /* ENOSPC */
>  			goto errout;
> -		reserved = true;
>  	} else {   /* bigalloc */
>  		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
>  			if (!ext4_es_scan_clu(inode,
> @@ -1673,7 +1671,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>  					ret = ext4_da_reserve_space(inode);
>  					if (ret != 0)   /* ENOSPC */
>  						goto errout;
> -					reserved = true;
>  				} else {
>  					allocated = true;
>  				}
> @@ -1683,10 +1680,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>  		}
>  	}
>  
> -	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
> -	if (ret && reserved)
> -		ext4_da_release_space(inode, 1);
> -
> +	ext4_es_insert_delayed_block(inode, lblk, allocated);
>  errout:
>  	return ret;
>  }
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Theodore Ts'o]

On Mon, Apr 24, 2023 at 11:38:44AM +0800, Baokun Li wrote:
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index a0bfe77d5537..4221b2dafeb5 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1641,9 +1641,8 @@ static void ext4_print_free_blocks(struct inode *inode)
>  static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>  {
>  	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> -	int ret;
> +	int ret = 0;
>  	bool allocated = false;
> -	bool reserved = false;
>  
>  	/*
>  	 * If the cluster containing lblk is shared with a delayed,

Unforuntately, the changes to ext4_insert_delayed_block() in this
patch were buggy, and were causing tests to hang when running
ext4/encrypt, ext4/bigalloc_4k, and ext4/bigalloc_1k test scenarios.
A bisect using "gce-xfstests -c ext4/bigalloc_4k -C 5 generic/579"
pinpointed the problem.

The problem is that ext4_clu_mapped can return a positive value, and
so there are times when we do need to release the space even though
there are no errors.

So I've fixed up your commit with the following changes.  With this
change, the test regressions go away.

Cheers,

						- Ted

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index fa38092ef868..3a10427240cb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1630,6 +1630,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	int ret = 0;
 	bool allocated = false;
+	bool reserved = false;
 
 	/*
 	 * If the cluster containing lblk is shared with a delayed,
@@ -1646,6 +1647,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		ret = ext4_da_reserve_space(inode);
 		if (ret != 0)   /* ENOSPC */
 			goto errout;
+		reserved = true;
 	} else {   /* bigalloc */
 		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
 			if (!ext4_es_scan_clu(inode,
@@ -1658,6 +1660,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 					ret = ext4_da_reserve_space(inode);
 					if (ret != 0)   /* ENOSPC */
 						goto errout;
+					reserved = true;
 				} else {
 					allocated = true;
 				}
@@ -1668,6 +1671,9 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	}
 
 	ext4_es_insert_delayed_block(inode, lblk, allocated);
+	if (ret && reserved)
+		ext4_da_release_space(inode, 1);
+
 errout:
 	return ret;
 }

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Theodore Ts'o]

On Sat, Jun 10, 2023 at 03:03:19PM -0400, Theodore Ts'o wrote:
> Unforuntately, the changes to ext4_insert_delayed_block() in this
> patch were buggy, and were causing tests to hang when running
> ext4/encrypt, ext4/bigalloc_4k, and ext4/bigalloc_1k test scenarios.
> A bisect using "gce-xfstests -c ext4/bigalloc_4k -C 5 generic/579"
> pinpointed the problem.
> 
> The problem is that ext4_clu_mapped can return a positive value, and
> so there are times when we do need to release the space even though
> there are no errors.
> 
> So I've fixed up your commit with the following changes.  With this
> change, the test regressions go away.

It turns out my fix was not correct, because I misread the fundamental
problem with the patch.  The issue was in the last patch hunk:
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
 errout:
 	return ret;
 }

The problem is that entering this code hunk, ret could be non-zero.
But when we made ext4_es_insert_delayed_block() to return void.  So
the changes to fs/ext4/inode.c needed to be replaced by this:

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 129b9af53d62..7700db1782dd 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1630,7 +1630,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
 	int ret;
 	bool allocated = false;
-	bool reserved = false;
 
 	/*
 	 * If the cluster containing lblk is shared with a delayed,
@@ -1646,8 +1645,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 	if (sbi->s_cluster_ratio == 1) {
 		ret = ext4_da_reserve_space(inode);
 		if (ret != 0)   /* ENOSPC */
-			goto errout;
-		reserved = true;
+			return ret;
 	} else {   /* bigalloc */
 		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
 			if (!ext4_es_scan_clu(inode,
@@ -1655,12 +1653,11 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 				ret = ext4_clu_mapped(inode,
 						      EXT4_B2C(sbi, lblk));
 				if (ret < 0)
-					goto errout;
+					return ret;
 				if (ret == 0) {
 					ret = ext4_da_reserve_space(inode);
 					if (ret != 0)   /* ENOSPC */
-						goto errout;
-					reserved = true;
+						return ret;
 				} else {
 					allocated = true;
 				}
@@ -1670,12 +1667,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
 		}
 	}
 
-	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
-	if (ret && reserved)
-		ext4_da_release_space(inode, 1);
-
-errout:
-	return ret;
+	ext4_es_insert_delayed_block(inode, lblk, allocated);
+	return 0;
 }
 
 /*

						- Ted

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Baokun Li]

On 2023/6/12 11:04, Theodore Ts'o wrote:
> On Sat, Jun 10, 2023 at 03:03:19PM -0400, Theodore Ts'o wrote:
>> Unforuntately, the changes to ext4_insert_delayed_block() in this
>> patch were buggy, and were causing tests to hang when running
>> ext4/encrypt, ext4/bigalloc_4k, and ext4/bigalloc_1k test scenarios.
>> A bisect using "gce-xfstests -c ext4/bigalloc_4k -C 5 generic/579"
>> pinpointed the problem.
I'm very sorry, I didn't turn on encrypt or bigalloc when I tested it.
>>
>> The problem is that ext4_clu_mapped can return a positive value, and
>> so there are times when we do need to release the space even though
>> there are no errors.

Yes, ext4_clu_mapped may return a positive value,

but when it does, reserved is false and we never need to release the space.

>> So I've fixed up your commit with the following changes.  With this
>> change, the test regressions go away.
The previous reply was very confusing to me because the changes
in the previous reply have nothing to do with ext4_clu_mapped
and ret is always 0 when reserved is true, so we don't need
ext4_da_release_space to perform a rollback.
> It turns out my fix was not correct, because I misread the fundamental
> problem with the patch.  The issue was in the last patch hunk:
>   
> -	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
> -	if (ret && reserved)
> -		ext4_da_release_space(inode, 1);
> -
> +	ext4_es_insert_delayed_block(inode, lblk, allocated);
>   errout:
>   	return ret;
>   }

Indeed, there is a behavioral change in ret here.

Before modification:

ext4_da_map_blocks             --> return 0
  ext4_insert_delayed_block     --> return 0
   ext4_clu_mapped              --> return 1
   ext4_es_insert_delayed_block --> return 0

After modification:

ext4_da_map_blocks             --> return 1
  ext4_insert_delayed_block     --> return 1
   ext4_clu_mapped              --> return 1
   ext4_es_insert_delayed_block --> void

>
> The problem is that entering this code hunk, ret could be non-zero.
> But when we made ext4_es_insert_delayed_block() to return void.  So
> the changes to fs/ext4/inode.c needed to be replaced by this:
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 129b9af53d62..7700db1782dd 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1630,7 +1630,6 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>   	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
>   	int ret;
>   	bool allocated = false;
> -	bool reserved = false;
>   
>   	/*
>   	 * If the cluster containing lblk is shared with a delayed,
> @@ -1646,8 +1645,7 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>   	if (sbi->s_cluster_ratio == 1) {
>   		ret = ext4_da_reserve_space(inode);
>   		if (ret != 0)   /* ENOSPC */
> -			goto errout;
> -		reserved = true;
> +			return ret;
>   	} else {   /* bigalloc */
>   		if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
>   			if (!ext4_es_scan_clu(inode,
> @@ -1655,12 +1653,11 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>   				ret = ext4_clu_mapped(inode,
>   						      EXT4_B2C(sbi, lblk));
>   				if (ret < 0)
> -					goto errout;
> +					return ret;
>   				if (ret == 0) {
>   					ret = ext4_da_reserve_space(inode);
>   					if (ret != 0)   /* ENOSPC */
> -						goto errout;
> -					reserved = true;
> +						return ret;
>   				} else {
>   					allocated = true;
>   				}
> @@ -1670,12 +1667,8 @@ static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
>   		}
>   	}
>   
> -	ret = ext4_es_insert_delayed_block(inode, lblk, allocated);
> -	if (ret && reserved)
> -		ext4_da_release_space(inode, 1);
> -
> -errout:
> -	return ret;
> +	ext4_es_insert_delayed_block(inode, lblk, allocated);
> +	return 0;
>   }
>   
>   /*
>
> 						- Ted
>
Yes, it looks very good！A million thanks for the fix!

I am very sorry for taking your time to locate and fix this issue!

I will do more checks later.

-- 
With Best Regards,
Baokun Li
.

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Theodore Ts'o]

On Mon, Jun 12, 2023 at 11:47:07AM +0800, Baokun Li wrote:
> I'm very sorry, I didn't turn on encrypt or bigalloc when I tested it.

For complex series, it's helpful if you could run the equivalent of:

   {gce,kvm}-xfstests -c ext4/all -g auto

It takes about 24 hours (plus or minus, depending on the speed of your
storage device; it will also take about twice as long if
CONFIG_LOCKDEP is enabled) if you use kvm-xfstests.  Using
gce-xfstests with the ltm takes about around 1.75 hours (w/o LOCKDEP)
since it runs the tests in parallel, using separate VM's for each file
system config.

There are a small number of failures (especially flaky test failures),
however, (a) if a VM ever crashes, that's definitely a problem, and
(b) the ext4/4k config should be failure-free.  For example, here's a
current "good" test run that I'm checking the dev branch against.
(Currently, we have some kind of issue with -c ext4/adv generic/475
that I'm still chasing down.)

					- Ted

The failures seen below are known failures that we need to work
through.  Bill Whitney is working on the bigalloc_1k shared/298
failure, for example.  If you would like to work on one of the test
failures, especially if it's a file system config that you use in
production, please feel free to do so.  :-)   Also, if you are
interested in adapting the xfstests-bld codebase to support other
cloud services beyond Google Cloud Engine, again, let me know.

The gce-xfstests run below was generated using:

% gce-xfstests install-kconfig --lockdep
% gce-xfstests kbuild --dpkg
% gce-xfstests launch-ltm
% gce-xfstests ltm full

(Using the --dpkg is needed because is because there is a kexec bug
showing up when running on a Google Cloud VM's that I haven't been
able to fix, and it's been easier to just work around the kexec
problem.  Kexec works just fine on kvm-xfstests, though, so there's no
need to use kbuild --dpkg if you are just using kvm-xfstests.)

TESTRUNID: ltm-20230611154922
KERNEL:    kernel 6.4.0-rc5-xfstests-lockdep-00002-gdea9d8f7643f #170 SMP PREEMPT_DYNAMIC Sun Jun 11 15:21:52 EDT 2023 x86_64
CMDLINE:   full
CPUS:      2
MEM:       7680

ext4/4k: 549 tests, 51 skipped, 6895 seconds
ext4/1k: 545 tests, 54 skipped, 10730 seconds
ext4/ext3: 541 tests, 140 skipped, 8547 seconds
ext4/encrypt: 527 tests, 3 failures, 158 skipped, 5783 seconds
  Failures: generic/681 generic/682 generic/691
ext4/nojournal: 544 tests, 3 failures, 119 skipped, 8394 seconds
  Failures: ext4/301 ext4/304 generic/455
ext4/ext3conv: 546 tests, 52 skipped, 9024 seconds
ext4/adv: 546 tests, 2 failures, 59 skipped, 8454 seconds
  Failures: generic/477
  Flaky: generic/475: 60% (3/5)
ext4/dioread_nolock: 547 tests, 51 skipped, 7883 seconds
ext4/data_journal: 545 tests, 2 failures, 119 skipped, 7605 seconds
  Failures: generic/455 generic/484
ext4/bigalloc_4k: 521 tests, 56 skipped, 6650 seconds
ext4/bigalloc_1k: 521 tests, 1 failures, 64 skipped, 8074 seconds
  Failures: shared/298
ext4/dax: 536 tests, 154 skipped, 5118 seconds
Totals: 6512 tests, 1077 skipped, 53 failures, 0 errors, 82214s

FSTESTIMG: gce-xfstests/xfstests-amd64-202305310154
FSTESTPRJ: gce-xfstests
FSTESTVER: blktests 676d42c (Thu, 2 Mar 2023 15:25:44 +0900)
FSTESTVER: e2fsprogs 1.47.0-2-4-gd4745c4a (Tue, 30 May 2023 16:20:44 -0400)
FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
FSTESTVER: fsverity v1.5-6-g5d6f7c4 (Mon, 30 Jan 2023 23:22:45 -0800)
FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
FSTESTVER: quota  v4.05-53-gd90b7d5 (Tue, 6 Dec 2022 12:59:03 +0100)
FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
FSTESTVER: xfsprogs v6.1.1 (Fri, 13 Jan 2023 19:06:37 +0100)
FSTESTVER: xfstests-bld 6599baba-dirty (Wed, 19 Apr 2023 23:16:10 -0400)
FSTESTVER: xfstests v2023.04.09-8-g2525b7af5-dirty (Wed, 19 Apr 2023 13:42:14 -0400)
FSTESTVER: zz_build-distro bullseye
FSTESTSET: -g auto
FSTESTOPT: aex

Re: [PATCH v4 10/12] ext4: make ext4_es_insert_delayed_block() return void

[Baokun Li]

On 2023/6/12 23:26, Theodore Ts'o wrote:
> On Mon, Jun 12, 2023 at 11:47:07AM +0800, Baokun Li wrote:
>> I'm very sorry, I didn't turn on encrypt or bigalloc when I tested it.
> For complex series, it's helpful if you could run the equivalent of:
>
>     {gce,kvm}-xfstests -c ext4/all -g auto
>
> It takes about 24 hours (plus or minus, depending on the speed of your
> storage device; it will also take about twice as long if
> CONFIG_LOCKDEP is enabled) if you use kvm-xfstests.  Using
> gce-xfstests with the ltm takes about around 1.75 hours (w/o LOCKDEP)
> since it runs the tests in parallel, using separate VM's for each file
> system config.
>
> There are a small number of failures (especially flaky test failures),
> however, (a) if a VM ever crashes, that's definitely a problem, and
> (b) the ext4/4k config should be failure-free.  For example, here's a
> current "good" test run that I'm checking the dev branch against.
> (Currently, we have some kind of issue with -c ext4/adv generic/475
> that I'm still chasing down.)
>
> 					- Ted
>
> The failures seen below are known failures that we need to work
> through.  Bill Whitney is working on the bigalloc_1k shared/298
> failure, for example.  If you would like to work on one of the test
> failures, especially if it's a file system config that you use in
> production, please feel free to do so.  :-)   Also, if you are
> interested in adapting the xfstests-bld codebase to support other
> cloud services beyond Google Cloud Engine, again, let me know.

It looks very good and I'll try to use it.

I'll try to locate some test case failures when I get free.

>
> The gce-xfstests run below was generated using:
>
> % gce-xfstests install-kconfig --lockdep
> % gce-xfstests kbuild --dpkg
> % gce-xfstests launch-ltm
> % gce-xfstests ltm full
>
> (Using the --dpkg is needed because is because there is a kexec bug
> showing up when running on a Google Cloud VM's that I haven't been
> able to fix, and it's been easier to just work around the kexec
> problem.  Kexec works just fine on kvm-xfstests, though, so there's no
> need to use kbuild --dpkg if you are just using kvm-xfstests.)
>
> TESTRUNID: ltm-20230611154922
> KERNEL:    kernel 6.4.0-rc5-xfstests-lockdep-00002-gdea9d8f7643f #170 SMP PREEMPT_DYNAMIC Sun Jun 11 15:21:52 EDT 2023 x86_64
> CMDLINE:   full
> CPUS:      2
> MEM:       7680
>
> ext4/4k: 549 tests, 51 skipped, 6895 seconds
> ext4/1k: 545 tests, 54 skipped, 10730 seconds
> ext4/ext3: 541 tests, 140 skipped, 8547 seconds
> ext4/encrypt: 527 tests, 3 failures, 158 skipped, 5783 seconds
>    Failures: generic/681 generic/682 generic/691
> ext4/nojournal: 544 tests, 3 failures, 119 skipped, 8394 seconds
>    Failures: ext4/301 ext4/304 generic/455
> ext4/ext3conv: 546 tests, 52 skipped, 9024 seconds
> ext4/adv: 546 tests, 2 failures, 59 skipped, 8454 seconds
>    Failures: generic/477
>    Flaky: generic/475: 60% (3/5)
> ext4/dioread_nolock: 547 tests, 51 skipped, 7883 seconds
> ext4/data_journal: 545 tests, 2 failures, 119 skipped, 7605 seconds
>    Failures: generic/455 generic/484
> ext4/bigalloc_4k: 521 tests, 56 skipped, 6650 seconds
> ext4/bigalloc_1k: 521 tests, 1 failures, 64 skipped, 8074 seconds
>    Failures: shared/298
> ext4/dax: 536 tests, 154 skipped, 5118 seconds
> Totals: 6512 tests, 1077 skipped, 53 failures, 0 errors, 82214s
>
> FSTESTIMG: gce-xfstests/xfstests-amd64-202305310154
> FSTESTPRJ: gce-xfstests
> FSTESTVER: blktests 676d42c (Thu, 2 Mar 2023 15:25:44 +0900)
> FSTESTVER: e2fsprogs 1.47.0-2-4-gd4745c4a (Tue, 30 May 2023 16:20:44 -0400)
> FSTESTVER: fio  fio-3.31 (Tue, 9 Aug 2022 14:41:25 -0600)
> FSTESTVER: fsverity v1.5-6-g5d6f7c4 (Mon, 30 Jan 2023 23:22:45 -0800)
> FSTESTVER: ima-evm-utils v1.3.2 (Wed, 28 Oct 2020 13:18:08 -0400)
> FSTESTVER: nvme-cli v1.16 (Thu, 11 Nov 2021 13:09:06 -0800)
> FSTESTVER: quota  v4.05-53-gd90b7d5 (Tue, 6 Dec 2022 12:59:03 +0100)
> FSTESTVER: util-linux v2.38.1 (Thu, 4 Aug 2022 11:06:21 +0200)
> FSTESTVER: xfsprogs v6.1.1 (Fri, 13 Jan 2023 19:06:37 +0100)
> FSTESTVER: xfstests-bld 6599baba-dirty (Wed, 19 Apr 2023 23:16:10 -0400)
> FSTESTVER: xfstests v2023.04.09-8-g2525b7af5-dirty (Wed, 19 Apr 2023 13:42:14 -0400)
> FSTESTVER: zz_build-distro bullseye
> FSTESTSET: -g auto
> FSTESTOPT: aex
>
I was using native xfstests for my previous tests, and I feel that

gce-xfstests is much easier to use and the results are very clear,

thanks for the great recommendation!

-- 
With Best Regards,
Baokun Li
.

[PATCH v4 11/12] ext4: make ext4_es_insert_extent() return void

[Baokun Li]

Now ext4_es_insert_extent() never return error, so make it return void.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c        |  5 +++--
 fs/ext4/extents_status.c | 14 ++++++--------
 fs/ext4/extents_status.h |  6 +++---
 fs/ext4/inode.c          | 21 ++++++---------------
 4 files changed, 18 insertions(+), 28 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index e6695fec59af..d555ed924f37 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3136,8 +3136,9 @@ static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
 	if (ee_len == 0)
 		return 0;
 
-	return ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
-				     EXTENT_STATUS_WRITTEN);
+	ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
+			      EXTENT_STATUS_WRITTEN);
+	return 0;
 }
 
 /* FIXME!! we need to try to merge to left or right after zero-out  */
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index b12c5cfdf601..fcbfd2f26650 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -831,12 +831,10 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
 /*
  * ext4_es_insert_extent() adds information to an inode's extent
  * status tree.
- *
- * Return 0 on success, error code on failure.
  */
-int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
-			  ext4_lblk_t len, ext4_fsblk_t pblk,
-			  unsigned int status)
+void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
+			   ext4_lblk_t len, ext4_fsblk_t pblk,
+			   unsigned int status)
 {
 	struct extent_status newes;
 	ext4_lblk_t end = lblk + len - 1;
@@ -847,13 +845,13 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 	struct extent_status *es2 = NULL;
 
 	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
-		return 0;
+		return;
 
 	es_debug("add [%u/%u) %llu %x to extent status tree of inode %lu\n",
 		 lblk, len, pblk, status, inode->i_ino);
 
 	if (!len)
-		return 0;
+		return;
 
 	BUG_ON(end < lblk);
 
@@ -905,7 +903,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
 		goto retry;
 
 	ext4_es_print_tree(inode);
-	return 0;
+	return;
 }
 
 /*
diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
index c22edb931f1b..d9847a4a25db 100644
--- a/fs/ext4/extents_status.h
+++ b/fs/ext4/extents_status.h
@@ -127,9 +127,9 @@ extern int __init ext4_init_es(void);
 extern void ext4_exit_es(void);
 extern void ext4_es_init_tree(struct ext4_es_tree *tree);
 
-extern int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
-				 ext4_lblk_t len, ext4_fsblk_t pblk,
-				 unsigned int status);
+extern void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
+				  ext4_lblk_t len, ext4_fsblk_t pblk,
+				  unsigned int status);
 extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
 				 ext4_lblk_t len, ext4_fsblk_t pblk,
 				 unsigned int status);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 4221b2dafeb5..ffa40ce04c27 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -594,10 +594,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 		    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
 				       map->m_lblk + map->m_len - 1))
 			status |= EXTENT_STATUS_DELAYED;
-		ret = ext4_es_insert_extent(inode, map->m_lblk,
-					    map->m_len, map->m_pblk, status);
-		if (ret < 0)
-			retval = ret;
+		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
+				      map->m_pblk, status);
 	}
 	up_read((&EXT4_I(inode)->i_data_sem));
 
@@ -706,12 +704,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
 		    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
 				       map->m_lblk + map->m_len - 1))
 			status |= EXTENT_STATUS_DELAYED;
-		ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
-					    map->m_pblk, status);
-		if (ret < 0) {
-			retval = ret;
-			goto out_sem;
-		}
+		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
+				      map->m_pblk, status);
 	}
 
 out_sem:
@@ -1779,7 +1773,6 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 		set_buffer_new(bh);
 		set_buffer_delay(bh);
 	} else if (retval > 0) {
-		int ret;
 		unsigned int status;
 
 		if (unlikely(retval != map->m_len)) {
@@ -1792,10 +1785,8 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
 
 		status = map->m_flags & EXT4_MAP_UNWRITTEN ?
 				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
-		ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
-					    map->m_pblk, status);
-		if (ret != 0)
-			retval = ret;
+		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
+				      map->m_pblk, status);
 	}
 
 out_unlock:
-- 
2.31.1

Re: [PATCH v4 11/12] ext4: make ext4_es_insert_extent() return void

[Jan Kara]

On Mon 24-04-23 11:38:45, Baokun Li wrote:
> Now ext4_es_insert_extent() never return error, so make it return void.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c        |  5 +++--
>  fs/ext4/extents_status.c | 14 ++++++--------
>  fs/ext4/extents_status.h |  6 +++---
>  fs/ext4/inode.c          | 21 ++++++---------------
>  4 files changed, 18 insertions(+), 28 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index e6695fec59af..d555ed924f37 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3136,8 +3136,9 @@ static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
>  	if (ee_len == 0)
>  		return 0;
>  
> -	return ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
> -				     EXTENT_STATUS_WRITTEN);
> +	ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
> +			      EXTENT_STATUS_WRITTEN);
> +	return 0;
>  }
>  
>  /* FIXME!! we need to try to merge to left or right after zero-out  */
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index b12c5cfdf601..fcbfd2f26650 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -831,12 +831,10 @@ static int __es_insert_extent(struct inode *inode, struct extent_status *newes,
>  /*
>   * ext4_es_insert_extent() adds information to an inode's extent
>   * status tree.
> - *
> - * Return 0 on success, error code on failure.
>   */
> -int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> -			  ext4_lblk_t len, ext4_fsblk_t pblk,
> -			  unsigned int status)
> +void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> +			   ext4_lblk_t len, ext4_fsblk_t pblk,
> +			   unsigned int status)
>  {
>  	struct extent_status newes;
>  	ext4_lblk_t end = lblk + len - 1;
> @@ -847,13 +845,13 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  	struct extent_status *es2 = NULL;
>  
>  	if (EXT4_SB(inode->i_sb)->s_mount_state & EXT4_FC_REPLAY)
> -		return 0;
> +		return;
>  
>  	es_debug("add [%u/%u) %llu %x to extent status tree of inode %lu\n",
>  		 lblk, len, pblk, status, inode->i_ino);
>  
>  	if (!len)
> -		return 0;
> +		return;
>  
>  	BUG_ON(end < lblk);
>  
> @@ -905,7 +903,7 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
>  		goto retry;
>  
>  	ext4_es_print_tree(inode);
> -	return 0;
> +	return;
>  }
>  
>  /*
> diff --git a/fs/ext4/extents_status.h b/fs/ext4/extents_status.h
> index c22edb931f1b..d9847a4a25db 100644
> --- a/fs/ext4/extents_status.h
> +++ b/fs/ext4/extents_status.h
> @@ -127,9 +127,9 @@ extern int __init ext4_init_es(void);
>  extern void ext4_exit_es(void);
>  extern void ext4_es_init_tree(struct ext4_es_tree *tree);
>  
> -extern int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> -				 ext4_lblk_t len, ext4_fsblk_t pblk,
> -				 unsigned int status);
> +extern void ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
> +				  ext4_lblk_t len, ext4_fsblk_t pblk,
> +				  unsigned int status);
>  extern void ext4_es_cache_extent(struct inode *inode, ext4_lblk_t lblk,
>  				 ext4_lblk_t len, ext4_fsblk_t pblk,
>  				 unsigned int status);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 4221b2dafeb5..ffa40ce04c27 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -594,10 +594,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>  				       map->m_lblk + map->m_len - 1))
>  			status |= EXTENT_STATUS_DELAYED;
> -		ret = ext4_es_insert_extent(inode, map->m_lblk,
> -					    map->m_len, map->m_pblk, status);
> -		if (ret < 0)
> -			retval = ret;
> +		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> +				      map->m_pblk, status);
>  	}
>  	up_read((&EXT4_I(inode)->i_data_sem));
>  
> @@ -706,12 +704,8 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
>  		    ext4_es_scan_range(inode, &ext4_es_is_delayed, map->m_lblk,
>  				       map->m_lblk + map->m_len - 1))
>  			status |= EXTENT_STATUS_DELAYED;
> -		ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> -					    map->m_pblk, status);
> -		if (ret < 0) {
> -			retval = ret;
> -			goto out_sem;
> -		}
> +		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> +				      map->m_pblk, status);
>  	}
>  
>  out_sem:
> @@ -1779,7 +1773,6 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  		set_buffer_new(bh);
>  		set_buffer_delay(bh);
>  	} else if (retval > 0) {
> -		int ret;
>  		unsigned int status;
>  
>  		if (unlikely(retval != map->m_len)) {
> @@ -1792,10 +1785,8 @@ static int ext4_da_map_blocks(struct inode *inode, sector_t iblock,
>  
>  		status = map->m_flags & EXT4_MAP_UNWRITTEN ?
>  				EXTENT_STATUS_UNWRITTEN : EXTENT_STATUS_WRITTEN;
> -		ret = ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> -					    map->m_pblk, status);
> -		if (ret != 0)
> -			retval = ret;
> +		ext4_es_insert_extent(inode, map->m_lblk, map->m_len,
> +				      map->m_pblk, status);
>  	}
>  
>  out_unlock:
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

[PATCH v4 12/12] ext4: make ext4_zeroout_es() return void

[Baokun Li]

After ext4_es_insert_extent() returns void, the return value in
ext4_zeroout_es() is also unnecessary, so make it return void too.

Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
 fs/ext4/extents.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d555ed924f37..6c3080830b00 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3123,7 +3123,7 @@ void ext4_ext_release(struct super_block *sb)
 #endif
 }
 
-static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
+static void ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
 {
 	ext4_lblk_t  ee_block;
 	ext4_fsblk_t ee_pblock;
@@ -3134,11 +3134,10 @@ static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
 	ee_pblock = ext4_ext_pblock(ex);
 
 	if (ee_len == 0)
-		return 0;
+		return;
 
 	ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
 			      EXTENT_STATUS_WRITTEN);
-	return 0;
 }
 
 /* FIXME!! we need to try to merge to left or right after zero-out  */
@@ -3288,7 +3287,7 @@ static int ext4_split_extent_at(handle_t *handle,
 			err = ext4_ext_dirty(handle, inode, path + path->p_depth);
 			if (!err)
 				/* update extent status tree */
-				err = ext4_zeroout_es(inode, &zero_ex);
+				ext4_zeroout_es(inode, &zero_ex);
 			/* If we failed at this point, we don't know in which
 			 * state the extent tree exactly is so don't try to fix
 			 * length of the original extent as it may do even more
@@ -3641,9 +3640,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
 out:
 	/* If we have gotten a failure, don't zero out status tree */
 	if (!err) {
-		err = ext4_zeroout_es(inode, &zero_ex1);
-		if (!err)
-			err = ext4_zeroout_es(inode, &zero_ex2);
+		ext4_zeroout_es(inode, &zero_ex1);
+		ext4_zeroout_es(inode, &zero_ex2);
 	}
 	return err ? err : allocated;
 }
-- 
2.31.1

Re: [PATCH v4 12/12] ext4: make ext4_zeroout_es() return void

[Jan Kara]

On Mon 24-04-23 11:38:46, Baokun Li wrote:
> After ext4_es_insert_extent() returns void, the return value in
> ext4_zeroout_es() is also unnecessary, so make it return void too.
> 
> Signed-off-by: Baokun Li <libaokun1@huawei.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/ext4/extents.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index d555ed924f37..6c3080830b00 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3123,7 +3123,7 @@ void ext4_ext_release(struct super_block *sb)
>  #endif
>  }
>  
> -static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
> +static void ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
>  {
>  	ext4_lblk_t  ee_block;
>  	ext4_fsblk_t ee_pblock;
> @@ -3134,11 +3134,10 @@ static int ext4_zeroout_es(struct inode *inode, struct ext4_extent *ex)
>  	ee_pblock = ext4_ext_pblock(ex);
>  
>  	if (ee_len == 0)
> -		return 0;
> +		return;
>  
>  	ext4_es_insert_extent(inode, ee_block, ee_len, ee_pblock,
>  			      EXTENT_STATUS_WRITTEN);
> -	return 0;
>  }
>  
>  /* FIXME!! we need to try to merge to left or right after zero-out  */
> @@ -3288,7 +3287,7 @@ static int ext4_split_extent_at(handle_t *handle,
>  			err = ext4_ext_dirty(handle, inode, path + path->p_depth);
>  			if (!err)
>  				/* update extent status tree */
> -				err = ext4_zeroout_es(inode, &zero_ex);
> +				ext4_zeroout_es(inode, &zero_ex);
>  			/* If we failed at this point, we don't know in which
>  			 * state the extent tree exactly is so don't try to fix
>  			 * length of the original extent as it may do even more
> @@ -3641,9 +3640,8 @@ static int ext4_ext_convert_to_initialized(handle_t *handle,
>  out:
>  	/* If we have gotten a failure, don't zero out status tree */
>  	if (!err) {
> -		err = ext4_zeroout_es(inode, &zero_ex1);
> -		if (!err)
> -			err = ext4_zeroout_es(inode, &zero_ex2);
> +		ext4_zeroout_es(inode, &zero_ex1);
> +		ext4_zeroout_es(inode, &zero_ex2);
>  	}
>  	return err ? err : allocated;
>  }
> -- 
> 2.31.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v4 00/12] ext4: fix WARNING in ext4_da_update_reserve_space

[Baokun Li]

A gentle ping too.

On 2023/4/24 11:38, Baokun Li wrote:
> V1->V2:
>          Modify the patch 1 description and add the Fixes tag.
>          Add the patch 2 as suggested by Jan Kara.
> V2->V3:
>          Remove the redundant judgment of count in Patch [1].
>          Rename ext4_es_alloc_should_nofail to ext4_es_must_keep.
>          Split Patch [2].
>          Make some functions return void to simplify the code.
> V3->V4:
>          using nofail preallocation.
>
> This patch set consists of three parts:
> 1. Patch [1] fix WARNING in ext4_da_update_reserve_space.
> 2. Patch [2]-[8] fix extent tree inconsistencies that may be caused
>     by memory allocation failures.
> 3. Patch [9]-[12] is cleanup.
>
> Baokun Li (12):
>    ext4: only update i_reserved_data_blocks on successful block
>      allocation
>    ext4: add a new helper to check if es must be kept
>    ext4: factor out __es_alloc_extent() and __es_free_extent()
>    ext4: use pre-allocated es in __es_insert_extent()
>    ext4: use pre-allocated es in __es_remove_extent()
>    ext4: using nofail preallocation in ext4_es_remove_extent()
>    ext4: using nofail preallocation in ext4_es_insert_delayed_block()
>    ext4: using nofail preallocation in ext4_es_insert_extent()
>    ext4: make ext4_es_remove_extent() return void
>    ext4: make ext4_es_insert_delayed_block() return void
>    ext4: make ext4_es_insert_extent() return void
>    ext4: make ext4_zeroout_es() return void
>
>   fs/ext4/extents.c        |  49 +++------
>   fs/ext4/extents_status.c | 207 ++++++++++++++++++++++++---------------
>   fs/ext4/extents_status.h |  14 +--
>   fs/ext4/indirect.c       |   8 ++
>   fs/ext4/inline.c         |  12 +--
>   fs/ext4/inode.c          |  49 ++-------
>   6 files changed, 169 insertions(+), 170 deletions(-)
>

Re: [PATCH v4 00/12] ext4: fix WARNING in ext4_da_update_reserve_space

[Theodore Ts'o]

On Mon, 24 Apr 2023 11:38:34 +0800, Baokun Li wrote:
> V1->V2:
>         Modify the patch 1 description and add the Fixes tag.
>         Add the patch 2 as suggested by Jan Kara.
> V2->V3:
>         Remove the redundant judgment of count in Patch [1].
>         Rename ext4_es_alloc_should_nofail to ext4_es_must_keep.
>         Split Patch [2].
>         Make some functions return void to simplify the code.
> V3->V4:
>         using nofail preallocation.
> 
> [...]

Applied, thanks!

[01/12] ext4: only update i_reserved_data_blocks on successful block allocation
        commit: 58f109aeac17eb37069e63a082264683bbfaf696
[02/12] ext4: add a new helper to check if es must be kept
        commit: 353c585db33932679f6f8c00414745ce09c841c2
[03/12] ext4: factor out __es_alloc_extent() and __es_free_extent()
        commit: eee0d79e61a904b103a5604496f595a435690a82
[04/12] ext4: use pre-allocated es in __es_insert_extent()
        commit: 2e7ff3411271a1dec6b8bf58b8f81286e927727c
[05/12] ext4: use pre-allocated es in __es_remove_extent()
        commit: d89b8ec674242325c2a4dcb894fdb7ee04d63312
[06/12] ext4: using nofail preallocation in ext4_es_remove_extent()
        commit: 413ebcba8610df6db5740f9bb551ea41dc353171
[07/12] ext4: using nofail preallocation in ext4_es_insert_delayed_block()
        commit: d40d2a66f5035413531f5450ed94731bb907d852
[08/12] ext4: using nofail preallocation in ext4_es_insert_extent()
        commit: bbaaebb30b6e75c1998d7ef58a2207a01a611524
[09/12] ext4: make ext4_es_remove_extent() return void
        commit: d0dc2a074d20492b46339eb7674c3527a1a9b0a2
[10/12] ext4: make ext4_es_insert_delayed_block() return void
        commit: 6b014bd8fede82619a08e5abe497daef5ef1796c
[11/12] ext4: make ext4_es_insert_extent() return void
        commit: 4ac358b63dba50218b8e46352ef319f490945a86
[12/12] ext4: make ext4_zeroout_es() return void
        commit: 703f64cb4dc0b7d30a7125bd3f28ba785f1e4351

Best regards,
-- 
Theodore Ts'o <tytso@mit.edu>