* [PATCH v3 1/8] ext4: only update i_reserved_data_blocks on successful block allocation
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 18:45 ` Jan Kara
2023-04-12 12:41 ` [PATCH v3 2/8] ext4: add a new helper to check if es must be kept Baokun Li
` (6 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
In our fault injection test, we create an ext4 file, migrate it to
non-extent based file, then punch a hole and finally trigger a WARN_ON
in the ext4_da_update_reserve_space():
EXT4-fs warning (device sda): ext4_da_update_reserve_space:369:
ino 14, used 11 with only 10 reserved data blocks
When writing back a non-extent based file, if we enable delalloc, the
number of reserved blocks will be subtracted from the number of blocks
mapped by ext4_ind_map_blocks(), and the extent status tree will be
updated. We update the extent status tree by first removing the old
extent_status and then inserting the new extent_status. If the block range
we remove happens to be in an extent, then we need to allocate another
extent_status with ext4_es_alloc_extent().
use old to remove to add new
|----------|------------|------------|
old extent_status
The problem is that the allocation of a new extent_status failed due to a
fault injection, and __es_shrink() did not get free memory, resulting in
a return of -ENOMEM. Then do_writepages() retries after receiving -ENOMEM,
we map to the same extent again, and the number of reserved blocks is again
subtracted from the number of blocks in that extent. Since the blocks in
the same extent are subtracted twice, we end up triggering WARN_ON at
ext4_da_update_reserve_space() because used > ei->i_reserved_data_blocks.
For non-extent based file, we update the number of reserved blocks after
ext4_ind_map_blocks() is executed, which causes a problem that when we call
ext4_ind_map_blocks() to create a block, it doesn't always create a block,
but we always reduce the number of reserved blocks. So we move the logic
for updating reserved blocks to ext4_ind_map_blocks() to ensure that the
number of reserved blocks is updated only after we do succeed in allocating
some new blocks.
Fixes: 5f634d064c70 ("ext4: Fix quota accounting error with fallocate")
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V1->V2:
Modify the patch description and add the Fixes tag.
V2->V3:
Remove the redundant judgment of count.
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 97eb728cb958..33ae92f0ddfb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -659,16 +659,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
*/
ext4_clear_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
}
-
- /*
- * Update reserved blocks/metadata blocks after successful
- * block allocation which had been deferred till now. We don't
- * support fallocate for non extent files. So we can update
- * reserve space here.
- */
- if ((retval > 0) &&
- (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))
- ext4_da_update_reserve_space(inode, retval, 1);
}
if (retval > 0) {
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 1/8] ext4: only update i_reserved_data_blocks on successful block allocation
2023-04-12 12:41 ` [PATCH v3 1/8] ext4: only update i_reserved_data_blocks on successful block allocation Baokun Li
@ 2023-04-12 18:45 ` Jan Kara
0 siblings, 0 replies; 19+ messages in thread
From: Jan Kara @ 2023-04-12 18:45 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Wed 12-04-23 20:41:19, Baokun Li wrote:
> In our fault injection test, we create an ext4 file, migrate it to
> non-extent based file, then punch a hole and finally trigger a WARN_ON
> in the ext4_da_update_reserve_space():
>
> EXT4-fs warning (device sda): ext4_da_update_reserve_space:369:
> ino 14, used 11 with only 10 reserved data blocks
>
> When writing back a non-extent based file, if we enable delalloc, the
> number of reserved blocks will be subtracted from the number of blocks
> mapped by ext4_ind_map_blocks(), and the extent status tree will be
> updated. We update the extent status tree by first removing the old
> extent_status and then inserting the new extent_status. If the block range
> we remove happens to be in an extent, then we need to allocate another
> extent_status with ext4_es_alloc_extent().
>
> use old to remove to add new
> |----------|------------|------------|
> old extent_status
>
> The problem is that the allocation of a new extent_status failed due to a
> fault injection, and __es_shrink() did not get free memory, resulting in
> a return of -ENOMEM. Then do_writepages() retries after receiving -ENOMEM,
> we map to the same extent again, and the number of reserved blocks is again
> subtracted from the number of blocks in that extent. Since the blocks in
> the same extent are subtracted twice, we end up triggering WARN_ON at
> ext4_da_update_reserve_space() because used > ei->i_reserved_data_blocks.
>
> For non-extent based file, we update the number of reserved blocks after
> ext4_ind_map_blocks() is executed, which causes a problem that when we call
> ext4_ind_map_blocks() to create a block, it doesn't always create a block,
> but we always reduce the number of reserved blocks. So we move the logic
> for updating reserved blocks to ext4_ind_map_blocks() to ensure that the
> number of reserved blocks is updated only after we do succeed in allocating
> some new blocks.
>
> Fixes: 5f634d064c70 ("ext4: Fix quota accounting error with fallocate")
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
Looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> V1->V2:
> Modify the patch description and add the Fixes tag.
> V2->V3:
> Remove the redundant judgment of count.
>
> 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 97eb728cb958..33ae92f0ddfb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -659,16 +659,6 @@ int ext4_map_blocks(handle_t *handle, struct inode *inode,
> */
> ext4_clear_inode_state(inode, EXT4_STATE_EXT_MIGRATE);
> }
> -
> - /*
> - * Update reserved blocks/metadata blocks after successful
> - * block allocation which had been deferred till now. We don't
> - * support fallocate for non extent files. So we can update
> - * reserve space here.
> - */
> - if ((retval > 0) &&
> - (flags & EXT4_GET_BLOCKS_DELALLOC_RESERVE))
> - ext4_da_update_reserve_space(inode, retval, 1);
> }
>
> if (retval > 0) {
> --
> 2.31.1
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
2023-04-12 12:41 ` [PATCH v3 1/8] ext4: only update i_reserved_data_blocks on successful block allocation Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 18:53 ` Jan Kara
2023-04-12 12:41 ` [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail Baokun Li
` (5 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
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>
---
V2->V3:
Rename ext4_es_alloc_should_nofail to ext4_es_must_keep.
fs/ext4/extents_status.c | 35 ++++++++++++++++++++++-------------
1 file changed, 22 insertions(+), 13 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 7bc221038c6c..f9dab2510bdc 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode)
spin_unlock(&sbi->s_es_lock);
}
+/*
+ * Returns 1 indicates that we cannot fail to allocate memory for this
+ * extent_status and cannot reclaim, clear, or free the extent until
+ * its status changes.
+ */
+static inline int ext4_es_must_keep(struct extent_status *es)
+{
+ /* filemap, bigalloc, and seek_data/hole need to use it. */
+ if (ext4_es_is_delayed(es))
+ return 1;
+
+ return 0;
+}
+
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 +474,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 +493,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 this es is not a must be kept */
+ 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 +865,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 +1718,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))
+ /* We can't reclaim a must be kept extent from status tree. */
+ if (ext4_es_must_keep(es))
goto next;
if (ext4_es_is_referenced(es)) {
ext4_es_clear_referenced(es);
@@ -1774,7 +1783,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
2023-04-12 12:41 ` [PATCH v3 2/8] ext4: add a new helper to check if es must be kept Baokun Li
@ 2023-04-12 18:53 ` Jan Kara
2023-04-13 2:00 ` Baokun Li
0 siblings, 1 reply; 19+ messages in thread
From: Jan Kara @ 2023-04-12 18:53 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Wed 12-04-23 20:41:20, Baokun Li wrote:
> 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. Just some small suggestions below...
> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
> index 7bc221038c6c..f9dab2510bdc 100644
> --- a/fs/ext4/extents_status.c
> +++ b/fs/ext4/extents_status.c
> @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode)
> spin_unlock(&sbi->s_es_lock);
> }
>
> +/*
> + * Returns 1 indicates that we cannot fail to allocate memory for this
> + * extent_status and cannot reclaim, clear, or free the extent until
> + * its status changes.
> + */
> +static inline int ext4_es_must_keep(struct extent_status *es)
Maybe we can return bool? Also I'd rephrase the comment as:
/*
* Returns true if we cannot fail to allocate memory for this extent_status
* entry and cannot reclaim it until its status changes.
*/
> +{
> + /* filemap, bigalloc, and seek_data/hole need to use it. */
> + if (ext4_es_is_delayed(es))
> + return 1;
> +
> + return 0;
> +}
> +
> 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 +474,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 +493,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 this es is not a must be kept */
Let's rephrase the comment as:
/* 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 +865,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 +1718,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))
> + /* We can't reclaim a must be kept extent from status tree. */
I guess we can just drop this comment. The function name explains enough...
> + if (ext4_es_must_keep(es))
> goto next;
> if (ext4_es_is_referenced(es)) {
> ext4_es_clear_referenced(es);
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
2023-04-12 18:53 ` Jan Kara
@ 2023-04-13 2:00 ` Baokun Li
2023-04-13 10:34 ` Jan Kara
0 siblings, 1 reply; 19+ messages in thread
From: Baokun Li @ 2023-04-13 2:00 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
yi.zhang, yangerkun, yukuai3, Baokun Li
On 2023/4/13 2:53, Jan Kara wrote:
> On Wed 12-04-23 20:41:20, Baokun Li wrote:
>> 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. Just some small suggestions below...
Thank you very much for your review!
>
>> diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
>> index 7bc221038c6c..f9dab2510bdc 100644
>> --- a/fs/ext4/extents_status.c
>> +++ b/fs/ext4/extents_status.c
>> @@ -448,6 +448,20 @@ static void ext4_es_list_del(struct inode *inode)
>> spin_unlock(&sbi->s_es_lock);
>> }
>>
>> +/*
>> + * Returns 1 indicates that we cannot fail to allocate memory for this
>> + * extent_status and cannot reclaim, clear, or free the extent until
>> + * its status changes.
>> + */
>> +static inline int ext4_es_must_keep(struct extent_status *es)
> Maybe we can return bool? Also I'd rephrase the comment as:
Totally agree! I tried to move it to fs/ext4/extents_status.h before, so I
changed the function type to int, but later I realized that it was not
necessary
to move it to the header file, but the function type was not changed back.
>
> /*
> * Returns true if we cannot fail to allocate memory for this extent_status
> * entry and cannot reclaim it until its status changes.
> */
OK, looks good, thanks!
>> +{
>> + /* filemap, bigalloc, and seek_data/hole need to use it. */
>> + if (ext4_es_is_delayed(es))
>> + return 1;
>> +
>> + return 0;
>> +}
>> +
>> 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 +474,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 +493,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 this es is not a must be kept */
> Let's rephrase the comment as:
> /* Decrease the shrink counter when we can reclaim the extent */
Okay, this is very nice!
>
>> + 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 +865,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 +1718,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))
>> + /* We can't reclaim a must be kept extent from status tree. */
> I guess we can just drop this comment. The function name explains enough...
Totally agree!
>
>> + if (ext4_es_must_keep(es))
>> goto next;
>> if (ext4_es_is_referenced(es)) {
>> ext4_es_clear_referenced(es);
> Honza
Can you please help review the remaining patches for any problems?
If you have any suggestions, I'll fix them together and post another
version of v4.
Thanks again for your review!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
2023-04-13 2:00 ` Baokun Li
@ 2023-04-13 10:34 ` Jan Kara
2023-04-13 12:26 ` Baokun Li
0 siblings, 1 reply; 19+ messages in thread
From: Jan Kara @ 2023-04-13 10:34 UTC (permalink / raw)
To: Baokun Li
Cc: Jan Kara, linux-ext4, tytso, adilger.kernel, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Thu 13-04-23 10:00:56, Baokun Li wrote:
> Can you please help review the remaining patches for any problems?
> If you have any suggestions, I'll fix them together and post another version
> of v4.
Yes, I've got a bit stuck thinking about the GFP_NOFAIL problem and how to
best solve it. It will require some changes but most of the patchset can
stay I believe.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 2/8] ext4: add a new helper to check if es must be kept
2023-04-13 10:34 ` Jan Kara
@ 2023-04-13 12:26 ` Baokun Li
0 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-13 12:26 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
yi.zhang, yangerkun, yukuai3, Baokun Li
On 2023/4/13 18:34, Jan Kara wrote:
> On Thu 13-04-23 10:00:56, Baokun Li wrote:
>> Can you please help review the remaining patches for any problems?
>> If you have any suggestions, I'll fix them together and post another version
>> of v4.
> Yes, I've got a bit stuck thinking about the GFP_NOFAIL problem and how to
> best solve it. It will require some changes but most of the patchset can
> stay I believe.
>
> Honza
OK! Thank you so much! That's a tricky one indeed.
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
2023-04-12 12:41 ` [PATCH v3 1/8] ext4: only update i_reserved_data_blocks on successful block allocation Baokun Li
2023-04-12 12:41 ` [PATCH v3 2/8] ext4: add a new helper to check if es must be kept Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-13 10:30 ` Jan Kara
2023-04-12 12:41 ` [PATCH v3 4/8] ext4: make __es_remove_extent return void Baokun Li
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
If extent status tree update fails, we have inconsistency between what is
stored in the extent status tree and what is stored on disk. And that can
cause even data corruption issues in some cases.
For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
And with the above logic, the undo operation in __es_remove_extent that
may cause inconsistency if the split extent fails is unnecessary, so we
remove it as well.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V2->V3:
Define helper as a preparatory patch and simplify the code.
fs/ext4/extents_status.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index f9dab2510bdc..a7c3200f9cbe 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -464,10 +464,15 @@ static inline int ext4_es_must_keep(struct extent_status *es)
static struct extent_status *
ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
- ext4_fsblk_t pblk)
+ ext4_fsblk_t pblk, int nofail)
{
struct extent_status *es;
- es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
+ gfp_t gfp_flags = GFP_ATOMIC;
+
+ if (nofail)
+ gfp_flags |= __GFP_NOFAIL;
+
+ es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
if (es == NULL)
return NULL;
es->es_lblk = lblk;
@@ -804,7 +809,7 @@ 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);
+ newes->es_pblk, ext4_es_must_keep(newes));
if (!es)
return -ENOMEM;
rb_link_node(&es->rb_node, parent, p);
@@ -1361,8 +1366,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
ext4_es_status(&orig_es));
err = __es_insert_extent(inode, &newes);
if (err) {
- es->es_lblk = orig_es.es_lblk;
- es->es_len = orig_es.es_len;
if ((err == -ENOMEM) &&
__es_shrink(EXT4_SB(inode->i_sb),
128, EXT4_I(inode)))
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail
2023-04-12 12:41 ` [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail Baokun Li
@ 2023-04-13 10:30 ` Jan Kara
2023-04-24 3:45 ` Baokun Li
0 siblings, 1 reply; 19+ messages in thread
From: Jan Kara @ 2023-04-13 10:30 UTC (permalink / raw)
To: Baokun Li
Cc: linux-ext4, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3
On Wed 12-04-23 20:41:21, Baokun Li wrote:
> If extent status tree update fails, we have inconsistency between what is
> stored in the extent status tree and what is stored on disk. And that can
> cause even data corruption issues in some cases.
>
> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
> And with the above logic, the undo operation in __es_remove_extent that
> may cause inconsistency if the split extent fails is unnecessary, so we
> remove it as well.
>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Baokun Li <libaokun1@huawei.com>
When I was looking through this patch, I've realized there's a problem with
my plan :-|. See below...
> static struct extent_status *
> ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
> - ext4_fsblk_t pblk)
> + ext4_fsblk_t pblk, int nofail)
> {
> struct extent_status *es;
> - es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
> + gfp_t gfp_flags = GFP_ATOMIC;
> +
> + if (nofail)
> + gfp_flags |= __GFP_NOFAIL;
> +
> + es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
> if (es == NULL)
> return NULL;
I have remembered that the combination of GFP_ATOMIC and GFP_NOFAIL is
discouraged because the kernel has no sane way of refilling reserves for
atomic allocations when in atomic context. So this combination can result
in lockups.
So what I think we'll have to do is that we'll just have to return error
from __es_insert_extent() and __es_remove_extent() and in the callers we
drop the i_es_lock, allocate needed status entries (one or two depending on
the desired operation) with GFP_KERNEL | GFP_NOFAIL, get the lock again and
pass the preallocated entries into __es_insert_extent /
__es_remove_extent(). It's a bit ugly but we can at least remove those
__es_shrink() calls which are not pretty either.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail
2023-04-13 10:30 ` Jan Kara
@ 2023-04-24 3:45 ` Baokun Li
0 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-24 3:45 UTC (permalink / raw)
To: Jan Kara
Cc: linux-ext4, tytso, adilger.kernel, ritesh.list, linux-kernel,
yi.zhang, yangerkun, yukuai3, Baokun Li
On 2023/4/13 18:30, Jan Kara wrote:
> On Wed 12-04-23 20:41:21, Baokun Li wrote:
>> If extent status tree update fails, we have inconsistency between what is
>> stored in the extent status tree and what is stored on disk. And that can
>> cause even data corruption issues in some cases.
>>
>> For extents that cannot be dropped we use __GFP_NOFAIL to allocate memory.
>> And with the above logic, the undo operation in __es_remove_extent that
>> may cause inconsistency if the split extent fails is unnecessary, so we
>> remove it as well.
>>
>> Suggested-by: Jan Kara<jack@suse.cz>
>> Signed-off-by: Baokun Li<libaokun1@huawei.com>
> When I was looking through this patch, I've realized there's a problem with
> my plan :-|. See below...
>
>> static struct extent_status *
>> ext4_es_alloc_extent(struct inode *inode, ext4_lblk_t lblk, ext4_lblk_t len,
>> - ext4_fsblk_t pblk)
>> + ext4_fsblk_t pblk, int nofail)
>> {
>> struct extent_status *es;
>> - es = kmem_cache_alloc(ext4_es_cachep, GFP_ATOMIC);
>> + gfp_t gfp_flags = GFP_ATOMIC;
>> +
>> + if (nofail)
>> + gfp_flags |= __GFP_NOFAIL;
>> +
>> + es = kmem_cache_alloc(ext4_es_cachep, gfp_flags);
>> if (es == NULL)
>> return NULL;
> I have remembered that the combination of GFP_ATOMIC and GFP_NOFAIL is
> discouraged because the kernel has no sane way of refilling reserves for
> atomic allocations when in atomic context. So this combination can result
> in lockups.
Indeed. GFP_NOFAIL is only applicable to sleepable allocations,
GFP_ATOMIC will ignore it. I didn't notice that.
> So what I think we'll have to do is that we'll just have to return error
> from __es_insert_extent() and __es_remove_extent() and in the callers we
> drop the i_es_lock, allocate needed status entries (one or two depending on
> the desired operation) with GFP_KERNEL | GFP_NOFAIL, get the lock again and
> pass the preallocated entries into __es_insert_extent /
> __es_remove_extent(). It's a bit ugly but we can at least remove those
> __es_shrink() calls which are not pretty either.
>
> Honza
Yes, there's really no better way, thank you very much for your review!
I've sent a patch for v4 as you suggested.
Thanks again!
--
With Best Regards,
Baokun Li
.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 4/8] ext4: make __es_remove_extent return void
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
` (2 preceding siblings ...)
2023-04-12 12:41 ` [PATCH v3 3/8] ext4: use __GFP_NOFAIL if allocating extents_status cannot fail Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 12:41 ` [PATCH v3 5/8] ext4: make ext4_es_remove_extent " Baokun Li
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
Now __es_remove_extent() cannot fail (it will always remove what it should,
maybe more) so just make it return void.
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V2->V3:
New added.
fs/ext4/extents_status.c | 30 +++++++++++-------------------
1 file changed, 11 insertions(+), 19 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index a7c3200f9cbe..5207d3a9c1ad 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -145,8 +145,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_remove_extent(struct inode *inode, ext4_lblk_t lblk,
- ext4_lblk_t end, int *reserved);
+static void __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);
static int __es_shrink(struct ext4_sb_info *sbi, int nr_to_scan,
struct ext4_inode_info *locked_ei);
@@ -862,9 +862,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);
- if (err != 0)
- goto error;
+ __es_remove_extent(inode, lblk, end, NULL);
retry:
err = __es_insert_extent(inode, &newes);
if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
@@ -878,7 +876,6 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
status & EXTENT_STATUS_UNWRITTEN))
__revise_pending(inode, lblk, len);
-error:
write_unlock(&EXT4_I(inode)->i_es_lock);
ext4_es_print_tree(inode);
@@ -1311,11 +1308,10 @@ static unsigned int get_rsvd(struct inode *inode, ext4_lblk_t end,
*
* If @reserved is not NULL and delayed allocation is enabled, counts
* block/cluster reservations freed by removing range and if bigalloc
- * enabled cancels pending reservations as needed. Returns 0 on success,
- * error code on failure.
+ * enabled cancels pending reservations as needed.
*/
-static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
- ext4_lblk_t end, int *reserved)
+static void __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
+ ext4_lblk_t end, int *reserved)
{
struct ext4_es_tree *tree = &EXT4_I(inode)->i_es_tree;
struct rb_node *node;
@@ -1334,9 +1330,9 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
es = __es_tree_search(&tree->root, lblk);
if (!es)
- goto out;
+ return;
if (es->es_lblk > end)
- goto out;
+ return;
/* Simply invalidate cache_es. */
tree->cache_es = NULL;
@@ -1370,7 +1366,7 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
__es_shrink(EXT4_SB(inode->i_sb),
128, EXT4_I(inode)))
goto retry;
- goto out;
+ return;
}
} else {
es->es_lblk = end + 1;
@@ -1429,8 +1425,6 @@ static int __es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
out_get_reserved:
if (count_reserved)
*reserved = get_rsvd(inode, end, es, &rc);
-out:
- return err;
}
/*
@@ -1469,7 +1463,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);
+ __es_remove_extent(inode, lblk, end, &reserved);
write_unlock(&EXT4_I(inode)->i_es_lock);
ext4_es_print_tree(inode);
ext4_da_release_space(inode, reserved);
@@ -2012,9 +2006,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);
- if (err != 0)
- goto error;
+ __es_remove_extent(inode, lblk, lblk, NULL);
retry:
err = __es_insert_extent(inode, &newes);
if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
--
2.31.1
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 5/8] ext4: make ext4_es_remove_extent return void
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
` (3 preceding siblings ...)
2023-04-12 12:41 ` [PATCH v3 4/8] ext4: make __es_remove_extent return void Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 12:41 ` [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block " Baokun Li
` (2 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
After __es_remove_extent returns void, the return value in
ext4_es_remove_extent is also unnecessary, so make it return void too.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V2->V3:
New added.
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, 17 insertions(+), 53 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 5207d3a9c1ad..1ba2dd14367c 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1435,24 +1435,23 @@ static void __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;
int reserved = 0;
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);
@@ -1467,7 +1466,6 @@ int ext4_es_remove_extent(struct inode *inode, ext4_lblk_t lblk,
write_unlock(&EXT4_I(inode)->i_es_lock);
ext4_es_print_tree(inode);
ext4_da_release_space(inode, reserved);
- return err;
}
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 1602d74b5eeb..dc88ec0e6611 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1955,16 +1955,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 33ae92f0ddfb..3bd4d5bbf0aa 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4026,12 +4026,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
` (4 preceding siblings ...)
2023-04-12 12:41 ` [PATCH v3 5/8] ext4: make ext4_es_remove_extent " Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 14:19 ` kernel test robot
2023-04-12 17:24 ` kernel test robot
2023-04-12 12:41 ` [PATCH v3 7/8] ext4: make ext4_es_insert_extent " Baokun Li
2023-04-12 12:41 ` [PATCH v3 8/8] ext4: make ext4_zeroout_es " Baokun Li
7 siblings, 2 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
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>
---
V2->V3:
New added.
fs/ext4/extents_status.c | 23 ++++++-----------------
fs/ext4/extents_status.h | 4 ++--
fs/ext4/inode.c | 7 ++-----
3 files changed, 10 insertions(+), 24 deletions(-)
diff --git a/fs/ext4/extents_status.c b/fs/ext4/extents_status.c
index 1ba2dd14367c..047ec2fc4899 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -1980,17 +1980,14 @@ 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 err = 0;
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);
@@ -2005,24 +2002,16 @@ int ext4_es_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk,
write_lock(&EXT4_I(inode)->i_es_lock);
__es_remove_extent(inode, lblk, lblk, NULL);
-retry:
- err = __es_insert_extent(inode, &newes);
- if (err == -ENOMEM && __es_shrink(EXT4_SB(inode->i_sb),
- 128, EXT4_I(inode)))
- goto retry;
- if (err != 0)
- goto error;
+
+ /* We never return an error when inserting a delayed extent. */
+ (void) __es_insert_extent(inode, &newes);
if (allocated)
__insert_pending(inode, lblk);
-
-error:
write_unlock(&EXT4_I(inode)->i_es_lock);
ext4_es_print_tree(inode);
ext4_print_pending_tree(inode);
-
- return err;
}
/*
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 3bd4d5bbf0aa..9fd3d526f591 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1635,7 +1635,7 @@ 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;
@@ -1677,10 +1677,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
2023-04-12 12:41 ` [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block " Baokun Li
@ 2023-04-12 14:19 ` kernel test robot
2023-04-13 2:36 ` Baokun Li
2023-04-12 17:24 ` kernel test robot
1 sibling, 1 reply; 19+ messages in thread
From: kernel test robot @ 2023-04-12 14:19 UTC (permalink / raw)
To: Baokun Li, linux-ext4
Cc: oe-kbuild-all, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3, libaokun1
Hi Baokun,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v6.3-rc6 next-20230412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230412124126.2286716-7-libaokun1%40huawei.com
patch subject: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230412/202304122234.3Meeshf9-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/87f992eeab9cd894894e27c3c6ff322cbd473ebf
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
git checkout 87f992eeab9cd894894e27c3c6ff322cbd473ebf
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304122234.3Meeshf9-lkp@intel.com/
All warnings (new ones prefixed by >>):
fs/ext4/inode.c: In function 'ext4_insert_delayed_block':
>> fs/ext4/inode.c:1640:14: warning: variable 'reserved' set but not used [-Wunused-but-set-variable]
1640 | bool reserved = false;
| ^~~~~~~~
vim +/reserved +1640 fs/ext4/inode.c
df22291ff0fde0 Aneesh Kumar K.V 2008-09-08 1623
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1624 /*
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1625 * ext4_insert_delayed_block - adds a delayed block to the extents status
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1626 * tree, incrementing the reserved cluster/block
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1627 * count or making a pending reservation
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1628 * where needed
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1629 *
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1630 * @inode - file containing the newly added block
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1631 * @lblk - logical block to be added
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1632 *
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1633 * Returns 0 on success, negative error code on failure.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1634 */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1635 static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1636 {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1637 struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
87f992eeab9cd8 Baokun Li 2023-04-12 1638 int ret = 0;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1639 bool allocated = false;
6fed83957f21ef Jeffle Xu 2021-08-23 @1640 bool reserved = false;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1641
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1642 /*
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1643 * If the cluster containing lblk is shared with a delayed,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1644 * written, or unwritten extent in a bigalloc file system, it's
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1645 * already been accounted for and does not need to be reserved.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1646 * A pending reservation must be made for the cluster if it's
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1647 * shared with a written or unwritten extent and doesn't already
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1648 * have one. Written and unwritten extents can be purged from the
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1649 * extents status tree if the system is under memory pressure, so
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1650 * it's necessary to examine the extent tree if a search of the
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1651 * extents status tree doesn't get a match.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1652 */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1653 if (sbi->s_cluster_ratio == 1) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1654 ret = ext4_da_reserve_space(inode);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1655 if (ret != 0) /* ENOSPC */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1656 goto errout;
6fed83957f21ef Jeffle Xu 2021-08-23 1657 reserved = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1658 } else { /* bigalloc */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1659 if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1660 if (!ext4_es_scan_clu(inode,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1661 &ext4_es_is_mapped, lblk)) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1662 ret = ext4_clu_mapped(inode,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1663 EXT4_B2C(sbi, lblk));
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1664 if (ret < 0)
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1665 goto errout;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1666 if (ret == 0) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1667 ret = ext4_da_reserve_space(inode);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1668 if (ret != 0) /* ENOSPC */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1669 goto errout;
6fed83957f21ef Jeffle Xu 2021-08-23 1670 reserved = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1671 } else {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1672 allocated = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1673 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1674 } else {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1675 allocated = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1676 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1677 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1678 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1679
87f992eeab9cd8 Baokun Li 2023-04-12 1680 ext4_es_insert_delayed_block(inode, lblk, allocated);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1681 errout:
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1682 return ret;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1683 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1684
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
2023-04-12 14:19 ` kernel test robot
@ 2023-04-13 2:36 ` Baokun Li
0 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-13 2:36 UTC (permalink / raw)
To: kernel test robot, linux-ext4
Cc: oe-kbuild-all, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3, Baokun Li
On 2023/4/12 22:19, kernel test robot wrote:
> Hi Baokun,
>
> kernel test robot noticed the following build warnings:
>
> [auto build test WARNING on tytso-ext4/dev]
> [also build test WARNING on linus/master v6.3-rc6 next-20230412]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
> base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
> patch link: https://lore.kernel.org/r/20230412124126.2286716-7-libaokun1%40huawei.com
> patch subject: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
> config: m68k-allyesconfig (https://download.01.org/0day-ci/archive/20230412/202304122234.3Meeshf9-lkp@intel.com/config)
> compiler: m68k-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/87f992eeab9cd894894e27c3c6ff322cbd473ebf
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
> git checkout 87f992eeab9cd894894e27c3c6ff322cbd473ebf
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash fs/ext4/
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202304122234.3Meeshf9-lkp@intel.com/
>
> All warnings (new ones prefixed by >>):
>
> fs/ext4/inode.c: In function 'ext4_insert_delayed_block':
>>> fs/ext4/inode.c:1640:14: warning: variable 'reserved' set but not used [-Wunused-but-set-variable]
> 1640 | bool reserved = false;
> | ^~~~~~~~
Thank you very much for testing, I will fix this issue in the next
version of the patch.
I checked with make W=2 to see if it would introduce a compile warning
before sending,
but it did not check for the problem. When I saw this email I checked
again with W=1,
and this checked the problem instead. This is strange, doesn't it seem
to me that a high
rank warning check should include a low rank warning check?
Am I misunderstanding or is this a bug?
>
> vim +/reserved +1640 fs/ext4/inode.c
>
> df22291ff0fde0 Aneesh Kumar K.V 2008-09-08 1623
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1624 /*
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1625 * ext4_insert_delayed_block - adds a delayed block to the extents status
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1626 * tree, incrementing the reserved cluster/block
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1627 * count or making a pending reservation
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1628 * where needed
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1629 *
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1630 * @inode - file containing the newly added block
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1631 * @lblk - logical block to be added
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1632 *
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1633 * Returns 0 on success, negative error code on failure.
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1634 */
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1635 static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1636 {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1637 struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> 87f992eeab9cd8 Baokun Li 2023-04-12 1638 int ret = 0;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1639 bool allocated = false;
> 6fed83957f21ef Jeffle Xu 2021-08-23 @1640 bool reserved = false;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1641
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1642 /*
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1643 * If the cluster containing lblk is shared with a delayed,
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1644 * written, or unwritten extent in a bigalloc file system, it's
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1645 * already been accounted for and does not need to be reserved.
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1646 * A pending reservation must be made for the cluster if it's
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1647 * shared with a written or unwritten extent and doesn't already
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1648 * have one. Written and unwritten extents can be purged from the
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1649 * extents status tree if the system is under memory pressure, so
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1650 * it's necessary to examine the extent tree if a search of the
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1651 * extents status tree doesn't get a match.
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1652 */
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1653 if (sbi->s_cluster_ratio == 1) {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1654 ret = ext4_da_reserve_space(inode);
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1655 if (ret != 0) /* ENOSPC */
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1656 goto errout;
> 6fed83957f21ef Jeffle Xu 2021-08-23 1657 reserved = true;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1658 } else { /* bigalloc */
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1659 if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1660 if (!ext4_es_scan_clu(inode,
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1661 &ext4_es_is_mapped, lblk)) {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1662 ret = ext4_clu_mapped(inode,
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1663 EXT4_B2C(sbi, lblk));
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1664 if (ret < 0)
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1665 goto errout;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1666 if (ret == 0) {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1667 ret = ext4_da_reserve_space(inode);
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1668 if (ret != 0) /* ENOSPC */
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1669 goto errout;
> 6fed83957f21ef Jeffle Xu 2021-08-23 1670 reserved = true;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1671 } else {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1672 allocated = true;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1673 }
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1674 } else {
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1675 allocated = true;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1676 }
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1677 }
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1678 }
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1679
> 87f992eeab9cd8 Baokun Li 2023-04-12 1680 ext4_es_insert_delayed_block(inode, lblk, allocated);
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1681 errout:
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1682 return ret;
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1683 }
> 0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1684
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
2023-04-12 12:41 ` [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block " Baokun Li
2023-04-12 14:19 ` kernel test robot
@ 2023-04-12 17:24 ` kernel test robot
1 sibling, 0 replies; 19+ messages in thread
From: kernel test robot @ 2023-04-12 17:24 UTC (permalink / raw)
To: Baokun Li, linux-ext4
Cc: llvm, oe-kbuild-all, tytso, adilger.kernel, jack, ritesh.list,
linux-kernel, yi.zhang, yangerkun, yukuai3, libaokun1
Hi Baokun,
kernel test robot noticed the following build warnings:
[auto build test WARNING on tytso-ext4/dev]
[also build test WARNING on linus/master v6.3-rc6 next-20230412]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
base: https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
patch link: https://lore.kernel.org/r/20230412124126.2286716-7-libaokun1%40huawei.com
patch subject: [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block return void
config: x86_64-randconfig-a002-20230410 (https://download.01.org/0day-ci/archive/20230413/202304130044.d3iOG59z-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/87f992eeab9cd894894e27c3c6ff322cbd473ebf
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Baokun-Li/ext4-only-update-i_reserved_data_blocks-on-successful-block-allocation/20230412-204407
git checkout 87f992eeab9cd894894e27c3c6ff322cbd473ebf
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash fs/ext4/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304130044.d3iOG59z-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> fs/ext4/inode.c:1640:7: warning: variable 'reserved' set but not used [-Wunused-but-set-variable]
bool reserved = false;
^
1 warning generated.
vim +/reserved +1640 fs/ext4/inode.c
df22291ff0fde0 Aneesh Kumar K.V 2008-09-08 1623
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1624 /*
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1625 * ext4_insert_delayed_block - adds a delayed block to the extents status
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1626 * tree, incrementing the reserved cluster/block
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1627 * count or making a pending reservation
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1628 * where needed
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1629 *
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1630 * @inode - file containing the newly added block
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1631 * @lblk - logical block to be added
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1632 *
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1633 * Returns 0 on success, negative error code on failure.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1634 */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1635 static int ext4_insert_delayed_block(struct inode *inode, ext4_lblk_t lblk)
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1636 {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1637 struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
87f992eeab9cd8 Baokun Li 2023-04-12 1638 int ret = 0;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1639 bool allocated = false;
6fed83957f21ef Jeffle Xu 2021-08-23 @1640 bool reserved = false;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1641
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1642 /*
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1643 * If the cluster containing lblk is shared with a delayed,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1644 * written, or unwritten extent in a bigalloc file system, it's
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1645 * already been accounted for and does not need to be reserved.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1646 * A pending reservation must be made for the cluster if it's
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1647 * shared with a written or unwritten extent and doesn't already
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1648 * have one. Written and unwritten extents can be purged from the
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1649 * extents status tree if the system is under memory pressure, so
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1650 * it's necessary to examine the extent tree if a search of the
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1651 * extents status tree doesn't get a match.
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1652 */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1653 if (sbi->s_cluster_ratio == 1) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1654 ret = ext4_da_reserve_space(inode);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1655 if (ret != 0) /* ENOSPC */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1656 goto errout;
6fed83957f21ef Jeffle Xu 2021-08-23 1657 reserved = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1658 } else { /* bigalloc */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1659 if (!ext4_es_scan_clu(inode, &ext4_es_is_delonly, lblk)) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1660 if (!ext4_es_scan_clu(inode,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1661 &ext4_es_is_mapped, lblk)) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1662 ret = ext4_clu_mapped(inode,
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1663 EXT4_B2C(sbi, lblk));
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1664 if (ret < 0)
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1665 goto errout;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1666 if (ret == 0) {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1667 ret = ext4_da_reserve_space(inode);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1668 if (ret != 0) /* ENOSPC */
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1669 goto errout;
6fed83957f21ef Jeffle Xu 2021-08-23 1670 reserved = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1671 } else {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1672 allocated = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1673 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1674 } else {
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1675 allocated = true;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1676 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1677 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1678 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1679
87f992eeab9cd8 Baokun Li 2023-04-12 1680 ext4_es_insert_delayed_block(inode, lblk, allocated);
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1681 errout:
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1682 return ret;
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1683 }
0b02f4c0d6d9e2 Eric Whitney 2018-10-01 1684
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 7/8] ext4: make ext4_es_insert_extent return void
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
` (5 preceding siblings ...)
2023-04-12 12:41 ` [PATCH v3 6/8] ext4: make ext4_es_insert_delayed_block " Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
2023-04-12 12:41 ` [PATCH v3 8/8] ext4: make ext4_zeroout_es " Baokun Li
7 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
In ext4_es_insert_extent, for extents that are not must be kept, the return
value is set to 0 even if the allocation returns -ENOMEM, and now inserting
an extent that must be kept never fails, hence ext4_es_insert_extent never
returns an error either, so let it return void.
Signed-off-by: Baokun Li <libaokun1@huawei.com>
---
V2->V3:
New added.
fs/ext4/extents.c | 5 +++--
fs/ext4/extents_status.c | 16 +++++-----------
fs/ext4/extents_status.h | 6 +++---
fs/ext4/inode.c | 21 ++++++---------------
4 files changed, 17 insertions(+), 31 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 047ec2fc4899..ab345523c8b1 100644
--- a/fs/ext4/extents_status.c
+++ b/fs/ext4/extents_status.c
@@ -823,12 +823,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;
@@ -836,13 +834,13 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
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);
@@ -868,8 +866,6 @@ 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_must_keep(&newes))
- err = 0;
if (sbi->s_cluster_ratio > 1 && test_opt(inode->i_sb, DELALLOC) &&
(status & EXTENT_STATUS_WRITTEN ||
@@ -879,8 +875,6 @@ int ext4_es_insert_extent(struct inode *inode, ext4_lblk_t lblk,
write_unlock(&EXT4_I(inode)->i_es_lock);
ext4_es_print_tree(inode);
-
- return err;
}
/*
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 9fd3d526f591..fc80791c239c 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:
@@ -1776,7 +1770,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)) {
@@ -1789,10 +1782,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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 8/8] ext4: make ext4_zeroout_es return void
2023-04-12 12:41 [PATCH v3 0/8] ext4: fix WARNING in ext4_da_update_reserve_space Baokun Li
` (6 preceding siblings ...)
2023-04-12 12:41 ` [PATCH v3 7/8] ext4: make ext4_es_insert_extent " Baokun Li
@ 2023-04-12 12:41 ` Baokun Li
7 siblings, 0 replies; 19+ messages in thread
From: Baokun Li @ 2023-04-12 12:41 UTC (permalink / raw)
To: linux-ext4
Cc: tytso, adilger.kernel, jack, ritesh.list, linux-kernel, yi.zhang,
yangerkun, yukuai3, libaokun1
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>
---
V2->V3:
New added.
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
^ permalink raw reply related [flat|nested] 19+ messages in thread