* [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
@ 2024-02-05 3:30 Zhiguo Niu
2024-02-05 3:47 ` Chao Yu
0 siblings, 1 reply; 13+ messages in thread
From: Zhiguo Niu @ 2024-02-05 3:30 UTC (permalink / raw)
To: jaegeuk, chao
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, zhiguo.niu, ke.wang,
hongyu.jin
There are some cases of f2fs_is_valid_blkaddr not handled as
ERROR_INVALID_BLKADDR,so unify the error handling about all of
f2fs_is_valid_blkaddr.
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
changes of v7: update patch according to sync with Chao
-restore some code to original
-modify err handle of __is_bitmap_valid for covering all cases
changes of v6: improve patch according to Chao's suggestions
-restore dump_stack to original position
-adjuest code sequence of __is_bitmap_check_valid
changes of v5: improve patch according to Jaegeuk's suggestiongs
-restore return value of some f2fs_is_valid_blkaddr error case to original
-move cp_err checking to outermost for unified processing
-return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
__is_bitmap_valid to avoid meaningless flow
-rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
and handling its return value in the caller uniformly, also cooperate
switch checking true to false for error case of
f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
for more readable
changes of v4: update according to the latest code
changes of v3:
-rebase patch to dev-test
-correct return value for some f2fs_is_valid_blkaddr error case
changes of v2: improve patch according Chao's suggestions.
---
---
fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
fs/f2fs/data.c | 22 +++-------------------
fs/f2fs/extent_cache.c | 5 +----
fs/f2fs/file.c | 16 +++-------------
fs/f2fs/gc.c | 2 --
fs/f2fs/recovery.c | 4 ----
fs/f2fs/segment.c | 2 --
7 files changed, 25 insertions(+), 59 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index b85820e..3335619 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
if (unlikely(f2fs_cp_error(sbi)))
return exist;
- if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
- f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
- blkaddr, exist);
- set_sbi_flag(sbi, SBI_NEED_FSCK);
- return exist;
- }
-
- if (!exist && type == DATA_GENERIC_ENHANCE) {
+ if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
+ (!exist && type == DATA_GENERIC_ENHANCE)) {
f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
blkaddr, exist);
set_sbi_flag(sbi, SBI_NEED_FSCK);
dump_stack();
}
+
return exist;
}
static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type)
{
+ bool valid = false;
+
switch (type) {
case META_NAT:
break;
case META_SIT:
if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
- return false;
+ goto err;
break;
case META_SSA:
if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
blkaddr < SM_I(sbi)->ssa_blkaddr))
- return false;
+ goto err;
break;
case META_CP:
if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
blkaddr < __start_cp_addr(sbi)))
- return false;
+ goto err;
break;
case META_POR:
if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
blkaddr < MAIN_BLKADDR(sbi)))
- return false;
+ goto err;
break;
case DATA_GENERIC:
case DATA_GENERIC_ENHANCE:
@@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
blkaddr);
set_sbi_flag(sbi, SBI_NEED_FSCK);
dump_stack();
- return false;
+ goto err;
} else {
- return __is_bitmap_valid(sbi, blkaddr, type);
+ valid = __is_bitmap_valid(sbi, blkaddr, type);
+ if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
+ (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
+ goto err;
}
break;
case META_GENERIC:
if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
blkaddr >= MAIN_BLKADDR(sbi)))
- return false;
+ goto err;
break;
default:
BUG();
}
return true;
+err:
+ f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
+ return valid;
}
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index 05158f8..300f9ae 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
fio->is_por ? META_POR : (__is_meta_io(fio) ?
- META_GENERIC : DATA_GENERIC_ENHANCE))) {
- f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
+ META_GENERIC : DATA_GENERIC_ENHANCE)))
return -EFSCORRUPTED;
- }
trace_f2fs_submit_page_bio(page, fio);
@@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
fio->encrypted_page : fio->page;
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
- __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
- f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
+ __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
return -EFSCORRUPTED;
- }
trace_f2fs_submit_page_bio(page, fio);
@@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
DATA_GENERIC_ENHANCE_READ)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(F2FS_I_SB(inode),
- ERROR_INVALID_BLKADDR);
goto put_err;
}
goto got_it;
@@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
dn.data_blkaddr,
DATA_GENERIC_ENHANCE)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(F2FS_I_SB(inode),
- ERROR_INVALID_BLKADDR);
goto put_err;
}
got_it:
@@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
if (!is_hole &&
!f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto sync_out;
}
@@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
DATA_GENERIC_ENHANCE_READ)) {
ret = -EFSCORRUPTED;
- f2fs_handle_error(F2FS_I_SB(inode),
- ERROR_INVALID_BLKADDR);
goto out;
}
} else {
@@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
f2fs_lookup_read_extent_cache_block(inode, page->index,
&fio->old_blkaddr)) {
if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
- DATA_GENERIC_ENHANCE)) {
- f2fs_handle_error(fio->sbi,
- ERROR_INVALID_BLKADDR);
+ DATA_GENERIC_ENHANCE))
return -EFSCORRUPTED;
- }
ipu_force = true;
fio->need_lock = LOCK_DONE;
@@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
DATA_GENERIC_ENHANCE)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
goto out_writepage;
}
@@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
DATA_GENERIC_ENHANCE_READ)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto fail;
}
err = f2fs_submit_page_read(use_cow ?
diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index ad8dfac7..48048fa 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
!f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
DATA_GENERIC_ENHANCE)) {
- set_sbi_flag(sbi, SBI_NEED_FSCK);
f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
__func__, inode->i_ino,
ei->blk, ei->fofs, ei->len);
@@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
goto out;
if (__is_valid_data_blkaddr(blkaddr) &&
- !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
- f2fs_bug_on(sbi, 1);
+ !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
return -EINVAL;
- }
out:
/*
* init block age with zero, this can happen when the block age extent
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 25b119cf..23cd6a1 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
continue;
if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
- DATA_GENERIC_ENHANCE)) {
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
+ DATA_GENERIC_ENHANCE))
continue;
- }
if (compressed_cluster)
valid_blocks++;
}
@@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
!f2fs_is_valid_blkaddr(sbi, *blkaddr,
DATA_GENERIC_ENHANCE)) {
f2fs_put_dnode(&dn);
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
return -EFSCORRUPTED;
}
@@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
DATA_GENERIC_ENHANCE)) {
ret = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
break;
}
@@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
if (!__is_valid_data_blkaddr(blkaddr))
continue;
if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
- DATA_GENERIC_ENHANCE))) {
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
+ DATA_GENERIC_ENHANCE)))
return -EFSCORRUPTED;
- }
}
while (count) {
@@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
if (!__is_valid_data_blkaddr(blkaddr))
continue;
if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
- DATA_GENERIC_ENHANCE))) {
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
+ DATA_GENERIC_ENHANCE)))
return -EFSCORRUPTED;
- }
}
while (count) {
@@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
DATA_GENERIC_ENHANCE)) {
ret = -EFSCORRUPTED;
f2fs_put_dnode(&dn);
- f2fs_handle_error(sbi,
- ERROR_INVALID_BLKADDR);
goto out;
}
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index a079eeb..30e93d8 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
DATA_GENERIC_ENHANCE_READ))) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto put_page;
}
goto got_it;
@@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
DATA_GENERIC_ENHANCE))) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto put_page;
}
got_it:
diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
index aad1d1a..289c0bf 100644
--- a/fs/f2fs/recovery.c
+++ b/fs/f2fs/recovery.c
@@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
if (__is_valid_data_blkaddr(src) &&
!f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto err;
}
if (__is_valid_data_blkaddr(dest) &&
!f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
goto err;
}
@@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
dest, inode->i_ino, dn.ofs_in_node);
err = -EFSCORRUPTED;
- f2fs_handle_error(sbi,
- ERROR_INVALID_BLKADDR);
goto err;
}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 7901ede..ad6511f 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
DATA_GENERIC_ENHANCE)) {
f2fs_put_dnode(&dn);
ret = -EFSCORRUPTED;
- f2fs_handle_error(sbi,
- ERROR_INVALID_BLKADDR);
goto out;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-05 3:30 [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr Zhiguo Niu
@ 2024-02-05 3:47 ` Chao Yu
2024-02-06 3:32 ` Jaegeuk Kim
0 siblings, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-05 3:47 UTC (permalink / raw)
To: Zhiguo Niu, jaegeuk
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang, hongyu.jin
On 2024/2/5 11:30, Zhiguo Niu wrote:
> There are some cases of f2fs_is_valid_blkaddr not handled as
> ERROR_INVALID_BLKADDR,so unify the error handling about all of
> f2fs_is_valid_blkaddr.
>
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> changes of v7: update patch according to sync with Chao
> -restore some code to original
> -modify err handle of __is_bitmap_valid for covering all cases
> changes of v6: improve patch according to Chao's suggestions
> -restore dump_stack to original position
> -adjuest code sequence of __is_bitmap_check_valid
> changes of v5: improve patch according to Jaegeuk's suggestiongs
> -restore return value of some f2fs_is_valid_blkaddr error case to original
> -move cp_err checking to outermost for unified processing
> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
> __is_bitmap_valid to avoid meaningless flow
> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
> and handling its return value in the caller uniformly, also cooperate
> switch checking true to false for error case of
> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
> for more readable
> changes of v4: update according to the latest code
> changes of v3:
> -rebase patch to dev-test
> -correct return value for some f2fs_is_valid_blkaddr error case
> changes of v2: improve patch according Chao's suggestions.
> ---
> ---
> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
> fs/f2fs/data.c | 22 +++-------------------
> fs/f2fs/extent_cache.c | 5 +----
> fs/f2fs/file.c | 16 +++-------------
> fs/f2fs/gc.c | 2 --
> fs/f2fs/recovery.c | 4 ----
> fs/f2fs/segment.c | 2 --
> 7 files changed, 25 insertions(+), 59 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index b85820e..3335619 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> if (unlikely(f2fs_cp_error(sbi)))
> return exist;
>
> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> - blkaddr, exist);
> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> - return exist;
> - }
> -
> - if (!exist && type == DATA_GENERIC_ENHANCE) {
> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> + (!exist && type == DATA_GENERIC_ENHANCE)) {
> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> blkaddr, exist);
> set_sbi_flag(sbi, SBI_NEED_FSCK);
> dump_stack();
> }
> +
No need to add one blank line.
Otherwise, it looks good to me.
Reviewed-by: Chao Yu <chao@kernel.org>
Thanks,
> return exist;
> }
>
> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> block_t blkaddr, int type)
> {
> + bool valid = false;
> +
> switch (type) {
> case META_NAT:
> break;
> case META_SIT:
> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
> - return false;
> + goto err;
> break;
> case META_SSA:
> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
> blkaddr < SM_I(sbi)->ssa_blkaddr))
> - return false;
> + goto err;
> break;
> case META_CP:
> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
> blkaddr < __start_cp_addr(sbi)))
> - return false;
> + goto err;
> break;
> case META_POR:
> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> blkaddr < MAIN_BLKADDR(sbi)))
> - return false;
> + goto err;
> break;
> case DATA_GENERIC:
> case DATA_GENERIC_ENHANCE:
> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> blkaddr);
> set_sbi_flag(sbi, SBI_NEED_FSCK);
> dump_stack();
> - return false;
> + goto err;
> } else {
> - return __is_bitmap_valid(sbi, blkaddr, type);
> + valid = __is_bitmap_valid(sbi, blkaddr, type);
> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> + goto err;
> }
> break;
> case META_GENERIC:
> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
> blkaddr >= MAIN_BLKADDR(sbi)))
> - return false;
> + goto err;
> break;
> default:
> BUG();
> }
>
> return true;
> +err:
> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> + return valid;
> }
>
> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 05158f8..300f9ae 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>
> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> fio->is_por ? META_POR : (__is_meta_io(fio) ?
> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> + META_GENERIC : DATA_GENERIC_ENHANCE)))
> return -EFSCORRUPTED;
> - }
>
> trace_f2fs_submit_page_bio(page, fio);
>
> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> fio->encrypted_page : fio->page;
>
> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> return -EFSCORRUPTED;
> - }
>
> trace_f2fs_submit_page_bio(page, fio);
>
> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
> DATA_GENERIC_ENHANCE_READ)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(F2FS_I_SB(inode),
> - ERROR_INVALID_BLKADDR);
> goto put_err;
> }
> goto got_it;
> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> dn.data_blkaddr,
> DATA_GENERIC_ENHANCE)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(F2FS_I_SB(inode),
> - ERROR_INVALID_BLKADDR);
> goto put_err;
> }
> got_it:
> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
> if (!is_hole &&
> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto sync_out;
> }
>
> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
> DATA_GENERIC_ENHANCE_READ)) {
> ret = -EFSCORRUPTED;
> - f2fs_handle_error(F2FS_I_SB(inode),
> - ERROR_INVALID_BLKADDR);
> goto out;
> }
> } else {
> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> f2fs_lookup_read_extent_cache_block(inode, page->index,
> &fio->old_blkaddr)) {
> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> - DATA_GENERIC_ENHANCE)) {
> - f2fs_handle_error(fio->sbi,
> - ERROR_INVALID_BLKADDR);
> + DATA_GENERIC_ENHANCE))
> return -EFSCORRUPTED;
> - }
>
> ipu_force = true;
> fio->need_lock = LOCK_DONE;
> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> DATA_GENERIC_ENHANCE)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> goto out_writepage;
> }
>
> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
> DATA_GENERIC_ENHANCE_READ)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto fail;
> }
> err = f2fs_submit_page_read(use_cow ?
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index ad8dfac7..48048fa 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
> DATA_GENERIC_ENHANCE)) {
> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
> __func__, inode->i_ino,
> ei->blk, ei->fofs, ei->len);
> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
> goto out;
>
> if (__is_valid_data_blkaddr(blkaddr) &&
> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> - f2fs_bug_on(sbi, 1);
> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
> return -EINVAL;
> - }
> out:
> /*
> * init block age with zero, this can happen when the block age extent
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 25b119cf..23cd6a1 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
> continue;
> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
> - DATA_GENERIC_ENHANCE)) {
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> + DATA_GENERIC_ENHANCE))
> continue;
> - }
> if (compressed_cluster)
> valid_blocks++;
> }
> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
> DATA_GENERIC_ENHANCE)) {
> f2fs_put_dnode(&dn);
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> return -EFSCORRUPTED;
> }
>
> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
> DATA_GENERIC_ENHANCE)) {
> ret = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> break;
> }
>
> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> if (!__is_valid_data_blkaddr(blkaddr))
> continue;
> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> - DATA_GENERIC_ENHANCE))) {
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> + DATA_GENERIC_ENHANCE)))
> return -EFSCORRUPTED;
> - }
> }
>
> while (count) {
> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> if (!__is_valid_data_blkaddr(blkaddr))
> continue;
> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> - DATA_GENERIC_ENHANCE))) {
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> + DATA_GENERIC_ENHANCE)))
> return -EFSCORRUPTED;
> - }
> }
>
> while (count) {
> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> DATA_GENERIC_ENHANCE)) {
> ret = -EFSCORRUPTED;
> f2fs_put_dnode(&dn);
> - f2fs_handle_error(sbi,
> - ERROR_INVALID_BLKADDR);
> goto out;
> }
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index a079eeb..30e93d8 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> DATA_GENERIC_ENHANCE_READ))) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto put_page;
> }
> goto got_it;
> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> DATA_GENERIC_ENHANCE))) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto put_page;
> }
> got_it:
> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> index aad1d1a..289c0bf 100644
> --- a/fs/f2fs/recovery.c
> +++ b/fs/f2fs/recovery.c
> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> if (__is_valid_data_blkaddr(src) &&
> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto err;
> }
>
> if (__is_valid_data_blkaddr(dest) &&
> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> goto err;
> }
>
> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
> dest, inode->i_ino, dn.ofs_in_node);
> err = -EFSCORRUPTED;
> - f2fs_handle_error(sbi,
> - ERROR_INVALID_BLKADDR);
> goto err;
> }
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 7901ede..ad6511f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> DATA_GENERIC_ENHANCE)) {
> f2fs_put_dnode(&dn);
> ret = -EFSCORRUPTED;
> - f2fs_handle_error(sbi,
> - ERROR_INVALID_BLKADDR);
> goto out;
> }
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-05 3:47 ` Chao Yu
@ 2024-02-06 3:32 ` Jaegeuk Kim
2024-02-06 3:36 ` Chao Yu
2024-02-19 3:46 ` Chao Yu
0 siblings, 2 replies; 13+ messages in thread
From: Jaegeuk Kim @ 2024-02-06 3:32 UTC (permalink / raw)
To: Chao Yu
Cc: Zhiguo Niu, linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang,
hongyu.jin
On 02/05, Chao Yu wrote:
> On 2024/2/5 11:30, Zhiguo Niu wrote:
> > There are some cases of f2fs_is_valid_blkaddr not handled as
> > ERROR_INVALID_BLKADDR,so unify the error handling about all of
> > f2fs_is_valid_blkaddr.
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> > Signed-off-by: Chao Yu <chao@kernel.org>
> > ---
> > changes of v7: update patch according to sync with Chao
> > -restore some code to original
> > -modify err handle of __is_bitmap_valid for covering all cases
> > changes of v6: improve patch according to Chao's suggestions
> > -restore dump_stack to original position
> > -adjuest code sequence of __is_bitmap_check_valid
> > changes of v5: improve patch according to Jaegeuk's suggestiongs
> > -restore return value of some f2fs_is_valid_blkaddr error case to original
> > -move cp_err checking to outermost for unified processing
> > -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
> > __is_bitmap_valid to avoid meaningless flow
> > -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
> > and handling its return value in the caller uniformly, also cooperate
> > switch checking true to false for error case of
> > f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
> > for more readable
> > changes of v4: update according to the latest code
> > changes of v3:
> > -rebase patch to dev-test
> > -correct return value for some f2fs_is_valid_blkaddr error case
> > changes of v2: improve patch according Chao's suggestions.
> > ---
> > ---
> > fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
> > fs/f2fs/data.c | 22 +++-------------------
> > fs/f2fs/extent_cache.c | 5 +----
> > fs/f2fs/file.c | 16 +++-------------
> > fs/f2fs/gc.c | 2 --
> > fs/f2fs/recovery.c | 4 ----
> > fs/f2fs/segment.c | 2 --
> > 7 files changed, 25 insertions(+), 59 deletions(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index b85820e..3335619 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> > if (unlikely(f2fs_cp_error(sbi)))
> > return exist;
> > - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
> > - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> > - blkaddr, exist);
> > - set_sbi_flag(sbi, SBI_NEED_FSCK);
> > - return exist;
> > - }
> > -
> > - if (!exist && type == DATA_GENERIC_ENHANCE) {
> > + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> > + (!exist && type == DATA_GENERIC_ENHANCE)) {
> > f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> > blkaddr, exist);
> > set_sbi_flag(sbi, SBI_NEED_FSCK);
> > dump_stack();
> > }
> > +
>
> No need to add one blank line.
>
> Otherwise, it looks good to me.
>
> Reviewed-by: Chao Yu <chao@kernel.org>
>
> Thanks,
>
> > return exist;
> > }
> > static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > block_t blkaddr, int type)
> > {
> > + bool valid = false;
> > +
> > switch (type) {
> > case META_NAT:
> > break;
> > case META_SIT:
> > if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
> > - return false;
> > + goto err;
> > break;
> > case META_SSA:
> > if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
> > blkaddr < SM_I(sbi)->ssa_blkaddr))
> > - return false;
> > + goto err;
> > break;
> > case META_CP:
> > if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
> > blkaddr < __start_cp_addr(sbi)))
> > - return false;
> > + goto err;
> > break;
> > case META_POR:
> > if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> > blkaddr < MAIN_BLKADDR(sbi)))
> > - return false;
> > + goto err;
> > break;
> > case DATA_GENERIC:
> > case DATA_GENERIC_ENHANCE:
> > @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > blkaddr);
> > set_sbi_flag(sbi, SBI_NEED_FSCK);
> > dump_stack();
> > - return false;
> > + goto err;
> > } else {
> > - return __is_bitmap_valid(sbi, blkaddr, type);
> > + valid = __is_bitmap_valid(sbi, blkaddr, type);
> > + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> > + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> > + goto err;
Please think about how to optimize this, which is really ugly now.
> > }
> > break;
> > case META_GENERIC:
> > if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
> > blkaddr >= MAIN_BLKADDR(sbi)))
> > - return false;
> > + goto err;
> > break;
> > default:
> > BUG();
> > }
> > return true;
> > +err:
> > + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > + return valid;
> > }
> > bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index 05158f8..300f9ae 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> > fio->is_por ? META_POR : (__is_meta_io(fio) ?
> > - META_GENERIC : DATA_GENERIC_ENHANCE))) {
> > - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> > + META_GENERIC : DATA_GENERIC_ENHANCE)))
> > return -EFSCORRUPTED;
> > - }
> > trace_f2fs_submit_page_bio(page, fio);
> > @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> > fio->encrypted_page : fio->page;
> > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> > - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
> > - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> > + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> > return -EFSCORRUPTED;
> > - }
> > trace_f2fs_submit_page_bio(page, fio);
> > @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> > if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
> > DATA_GENERIC_ENHANCE_READ)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(F2FS_I_SB(inode),
> > - ERROR_INVALID_BLKADDR);
> > goto put_err;
> > }
> > goto got_it;
> > @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> > dn.data_blkaddr,
> > DATA_GENERIC_ENHANCE)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(F2FS_I_SB(inode),
> > - ERROR_INVALID_BLKADDR);
> > goto put_err;
> > }
> > got_it:
> > @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
> > if (!is_hole &&
> > !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto sync_out;
> > }
> > @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
> > if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
> > DATA_GENERIC_ENHANCE_READ)) {
> > ret = -EFSCORRUPTED;
> > - f2fs_handle_error(F2FS_I_SB(inode),
> > - ERROR_INVALID_BLKADDR);
> > goto out;
> > }
> > } else {
> > @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> > f2fs_lookup_read_extent_cache_block(inode, page->index,
> > &fio->old_blkaddr)) {
> > if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> > - DATA_GENERIC_ENHANCE)) {
> > - f2fs_handle_error(fio->sbi,
> > - ERROR_INVALID_BLKADDR);
> > + DATA_GENERIC_ENHANCE))
> > return -EFSCORRUPTED;
> > - }
> > ipu_force = true;
> > fio->need_lock = LOCK_DONE;
> > @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> > !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> > DATA_GENERIC_ENHANCE)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> > goto out_writepage;
> > }
> > @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> > if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
> > DATA_GENERIC_ENHANCE_READ)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto fail;
> > }
> > err = f2fs_submit_page_read(use_cow ?
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> > index ad8dfac7..48048fa 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
> > if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
> > !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
> > DATA_GENERIC_ENHANCE)) {
> > - set_sbi_flag(sbi, SBI_NEED_FSCK);
> > f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
> > __func__, inode->i_ino,
> > ei->blk, ei->fofs, ei->len);
> > @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
> > goto out;
> > if (__is_valid_data_blkaddr(blkaddr) &&
> > - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> > - f2fs_bug_on(sbi, 1);
> > + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
> > return -EINVAL;
> > - }
> > out:
> > /*
> > * init block age with zero, this can happen when the block age extent
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index 25b119cf..23cd6a1 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> > if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
> > continue;
> > if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
> > - DATA_GENERIC_ENHANCE)) {
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > + DATA_GENERIC_ENHANCE))
> > continue;
> > - }
> > if (compressed_cluster)
> > valid_blocks++;
> > }
> > @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
> > !f2fs_is_valid_blkaddr(sbi, *blkaddr,
> > DATA_GENERIC_ENHANCE)) {
> > f2fs_put_dnode(&dn);
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > return -EFSCORRUPTED;
> > }
> > @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
> > if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
> > DATA_GENERIC_ENHANCE)) {
> > ret = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > break;
> > }
> > @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> > if (!__is_valid_data_blkaddr(blkaddr))
> > continue;
> > if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> > - DATA_GENERIC_ENHANCE))) {
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > + DATA_GENERIC_ENHANCE)))
> > return -EFSCORRUPTED;
> > - }
> > }
> > while (count) {
> > @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> > if (!__is_valid_data_blkaddr(blkaddr))
> > continue;
> > if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> > - DATA_GENERIC_ENHANCE))) {
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > + DATA_GENERIC_ENHANCE)))
> > return -EFSCORRUPTED;
> > - }
> > }
> > while (count) {
> > @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> > DATA_GENERIC_ENHANCE)) {
> > ret = -EFSCORRUPTED;
> > f2fs_put_dnode(&dn);
> > - f2fs_handle_error(sbi,
> > - ERROR_INVALID_BLKADDR);
> > goto out;
> > }
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index a079eeb..30e93d8 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> > if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> > DATA_GENERIC_ENHANCE_READ))) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto put_page;
> > }
> > goto got_it;
> > @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> > if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> > DATA_GENERIC_ENHANCE))) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto put_page;
> > }
> > got_it:
> > diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> > index aad1d1a..289c0bf 100644
> > --- a/fs/f2fs/recovery.c
> > +++ b/fs/f2fs/recovery.c
> > @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> > if (__is_valid_data_blkaddr(src) &&
> > !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto err;
> > }
> > if (__is_valid_data_blkaddr(dest) &&
> > !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > goto err;
> > }
> > @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> > f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
> > dest, inode->i_ino, dn.ofs_in_node);
> > err = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi,
> > - ERROR_INVALID_BLKADDR);
> > goto err;
> > }
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 7901ede..ad6511f 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> > DATA_GENERIC_ENHANCE)) {
> > f2fs_put_dnode(&dn);
> > ret = -EFSCORRUPTED;
> > - f2fs_handle_error(sbi,
> > - ERROR_INVALID_BLKADDR);
> > goto out;
> > }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-06 3:32 ` Jaegeuk Kim
@ 2024-02-06 3:36 ` Chao Yu
2024-02-06 23:38 ` Zhiguo Niu
2024-02-19 3:46 ` Chao Yu
1 sibling, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-06 3:36 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Zhiguo Niu, linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang,
hongyu.jin
On 2024/2/6 11:32, Jaegeuk Kim wrote:
> On 02/05, Chao Yu wrote:
>> On 2024/2/5 11:30, Zhiguo Niu wrote:
>>> There are some cases of f2fs_is_valid_blkaddr not handled as
>>> ERROR_INVALID_BLKADDR,so unify the error handling about all of
>>> f2fs_is_valid_blkaddr.
>>>
>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>> ---
>>> changes of v7: update patch according to sync with Chao
>>> -restore some code to original
>>> -modify err handle of __is_bitmap_valid for covering all cases
>>> changes of v6: improve patch according to Chao's suggestions
>>> -restore dump_stack to original position
>>> -adjuest code sequence of __is_bitmap_check_valid
>>> changes of v5: improve patch according to Jaegeuk's suggestiongs
>>> -restore return value of some f2fs_is_valid_blkaddr error case to original
>>> -move cp_err checking to outermost for unified processing
>>> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
>>> __is_bitmap_valid to avoid meaningless flow
>>> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
>>> and handling its return value in the caller uniformly, also cooperate
>>> switch checking true to false for error case of
>>> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
>>> for more readable
>>> changes of v4: update according to the latest code
>>> changes of v3:
>>> -rebase patch to dev-test
>>> -correct return value for some f2fs_is_valid_blkaddr error case
>>> changes of v2: improve patch according Chao's suggestions.
>>> ---
>>> ---
>>> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
>>> fs/f2fs/data.c | 22 +++-------------------
>>> fs/f2fs/extent_cache.c | 5 +----
>>> fs/f2fs/file.c | 16 +++-------------
>>> fs/f2fs/gc.c | 2 --
>>> fs/f2fs/recovery.c | 4 ----
>>> fs/f2fs/segment.c | 2 --
>>> 7 files changed, 25 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index b85820e..3335619 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>>> if (unlikely(f2fs_cp_error(sbi)))
>>> return exist;
>>> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
>>> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>> - blkaddr, exist);
>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> - return exist;
>>> - }
>>> -
>>> - if (!exist && type == DATA_GENERIC_ENHANCE) {
>>> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
>>> + (!exist && type == DATA_GENERIC_ENHANCE)) {
>>> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>> blkaddr, exist);
>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> dump_stack();
>>> }
>>> +
>>
>> No need to add one blank line.
>>
>> Otherwise, it looks good to me.
>>
>> Reviewed-by: Chao Yu <chao@kernel.org>
>>
>> Thanks,
>>
>>> return exist;
>>> }
>>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> block_t blkaddr, int type)
>>> {
>>> + bool valid = false;
>>> +
>>> switch (type) {
>>> case META_NAT:
>>> break;
>>> case META_SIT:
>>> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_SSA:
>>> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
>>> blkaddr < SM_I(sbi)->ssa_blkaddr))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_CP:
>>> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
>>> blkaddr < __start_cp_addr(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_POR:
>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>> blkaddr < MAIN_BLKADDR(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case DATA_GENERIC:
>>> case DATA_GENERIC_ENHANCE:
>>> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> blkaddr);
>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> dump_stack();
>>> - return false;
>>> + goto err;
>>> } else {
>>> - return __is_bitmap_valid(sbi, blkaddr, type);
>>> + valid = __is_bitmap_valid(sbi, blkaddr, type);
>>> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
>>> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
>>> + goto err;
>
> Please think about how to optimize this, which is really ugly now.
How about calling f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR) inside
__is_bitmap_valid()? so that we may not add such logic outside
__is_bitmap_valid()...
Thanks,
>
>>> }
>>> break;
>>> case META_GENERIC:
>>> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
>>> blkaddr >= MAIN_BLKADDR(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> default:
>>> BUG();
>>> }
>>> return true;
>>> +err:
>>> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + return valid;
>>> }
>>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 05158f8..300f9ae 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>> fio->is_por ? META_POR : (__is_meta_io(fio) ?
>>> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> + META_GENERIC : DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> trace_f2fs_submit_page_bio(page, fio);
>>> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>>> fio->encrypted_page : fio->page;
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
>>> return -EFSCORRUPTED;
>>> - }
>>> trace_f2fs_submit_page_bio(page, fio);
>>> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto put_err;
>>> }
>>> goto got_it;
>>> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>> dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto put_err;
>>> }
>>> got_it:
>>> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>> if (!is_hole &&
>>> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto sync_out;
>>> }
>>> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
>>> } else {
>>> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>> f2fs_lookup_read_extent_cache_block(inode, page->index,
>>> &fio->old_blkaddr)) {
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>> - DATA_GENERIC_ENHANCE)) {
>>> - f2fs_handle_error(fio->sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE))
>>> return -EFSCORRUPTED;
>>> - }
>>> ipu_force = true;
>>> fio->need_lock = LOCK_DONE;
>>> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> goto out_writepage;
>>> }
>>> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>>> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto fail;
>>> }
>>> err = f2fs_submit_page_read(use_cow ?
>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>> index ad8dfac7..48048fa 100644
>>> --- a/fs/f2fs/extent_cache.c
>>> +++ b/fs/f2fs/extent_cache.c
>>> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
>>> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
>>> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
>>> DATA_GENERIC_ENHANCE)) {
>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
>>> __func__, inode->i_ino,
>>> ei->blk, ei->fofs, ei->len);
>>> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
>>> goto out;
>>> if (__is_valid_data_blkaddr(blkaddr) &&
>>> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>> - f2fs_bug_on(sbi, 1);
>>> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
>>> return -EINVAL;
>>> - }
>>> out:
>>> /*
>>> * init block age with zero, this can happen when the block age extent
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 25b119cf..23cd6a1 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>>> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
>>> continue;
>>> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE)) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE))
>>> continue;
>>> - }
>>> if (compressed_cluster)
>>> valid_blocks++;
>>> }
>>> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
>>> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> f2fs_put_dnode(&dn);
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> return -EFSCORRUPTED;
>>> }
>>> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
>>> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> break;
>>> }
>>> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>> if (!__is_valid_data_blkaddr(blkaddr))
>>> continue;
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> }
>>> while (count) {
>>> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>> if (!__is_valid_data_blkaddr(blkaddr))
>>> continue;
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> }
>>> while (count) {
>>> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
>>> DATA_GENERIC_ENHANCE)) {
>>> ret = -EFSCORRUPTED;
>>> f2fs_put_dnode(&dn);
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index a079eeb..30e93d8 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE_READ))) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto put_page;
>>> }
>>> goto got_it;
>>> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE))) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto put_page;
>>> }
>>> got_it:
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index aad1d1a..289c0bf 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>> if (__is_valid_data_blkaddr(src) &&
>>> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> if (__is_valid_data_blkaddr(dest) &&
>>> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
>>> dest, inode->i_ino, dn.ofs_in_node);
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 7901ede..ad6511f 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>> DATA_GENERIC_ENHANCE)) {
>>> f2fs_put_dnode(&dn);
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-06 3:36 ` Chao Yu
@ 2024-02-06 23:38 ` Zhiguo Niu
0 siblings, 0 replies; 13+ messages in thread
From: Zhiguo Niu @ 2024-02-06 23:38 UTC (permalink / raw)
To: Jaegeuk Kim
Cc: Chao Yu, Zhiguo Niu, linux-f2fs-devel, linux-kernel, ke.wang, hongyu.jin
On Tue, Feb 6, 2024 at 11:36 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/2/6 11:32, Jaegeuk Kim wrote:
> > On 02/05, Chao Yu wrote:
> >> On 2024/2/5 11:30, Zhiguo Niu wrote:
> >>> There are some cases of f2fs_is_valid_blkaddr not handled as
> >>> ERROR_INVALID_BLKADDR,so unify the error handling about all of
> >>> f2fs_is_valid_blkaddr.
> >>>
> >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> >>> Signed-off-by: Chao Yu <chao@kernel.org>
> >>> ---
> >>> changes of v7: update patch according to sync with Chao
> >>> -restore some code to original
> >>> -modify err handle of __is_bitmap_valid for covering all cases
> >>> changes of v6: improve patch according to Chao's suggestions
> >>> -restore dump_stack to original position
> >>> -adjuest code sequence of __is_bitmap_check_valid
> >>> changes of v5: improve patch according to Jaegeuk's suggestiongs
> >>> -restore return value of some f2fs_is_valid_blkaddr error case to original
> >>> -move cp_err checking to outermost for unified processing
> >>> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
> >>> __is_bitmap_valid to avoid meaningless flow
> >>> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
> >>> and handling its return value in the caller uniformly, also cooperate
> >>> switch checking true to false for error case of
> >>> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
> >>> for more readable
> >>> changes of v4: update according to the latest code
> >>> changes of v3:
> >>> -rebase patch to dev-test
> >>> -correct return value for some f2fs_is_valid_blkaddr error case
> >>> changes of v2: improve patch according Chao's suggestions.
> >>> ---
> >>> ---
> >>> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
> >>> fs/f2fs/data.c | 22 +++-------------------
> >>> fs/f2fs/extent_cache.c | 5 +----
> >>> fs/f2fs/file.c | 16 +++-------------
> >>> fs/f2fs/gc.c | 2 --
> >>> fs/f2fs/recovery.c | 4 ----
> >>> fs/f2fs/segment.c | 2 --
> >>> 7 files changed, 25 insertions(+), 59 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index b85820e..3335619 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> >>> if (unlikely(f2fs_cp_error(sbi)))
> >>> return exist;
> >>> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
> >>> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> >>> - blkaddr, exist);
> >>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> - return exist;
> >>> - }
> >>> -
> >>> - if (!exist && type == DATA_GENERIC_ENHANCE) {
> >>> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> >>> + (!exist && type == DATA_GENERIC_ENHANCE)) {
> >>> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> >>> blkaddr, exist);
> >>> set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> dump_stack();
> >>> }
> >>> +
> >>
> >> No need to add one blank line.
> >>
> >> Otherwise, it looks good to me.
> >>
> >> Reviewed-by: Chao Yu <chao@kernel.org>
> >>
> >> Thanks,
> >>
> >>> return exist;
> >>> }
> >>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> block_t blkaddr, int type)
> >>> {
> >>> + bool valid = false;
> >>> +
> >>> switch (type) {
> >>> case META_NAT:
> >>> break;
> >>> case META_SIT:
> >>> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_SSA:
> >>> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
> >>> blkaddr < SM_I(sbi)->ssa_blkaddr))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_CP:
> >>> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
> >>> blkaddr < __start_cp_addr(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_POR:
> >>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> >>> blkaddr < MAIN_BLKADDR(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case DATA_GENERIC:
> >>> case DATA_GENERIC_ENHANCE:
> >>> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> blkaddr);
> >>> set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> dump_stack();
> >>> - return false;
> >>> + goto err;
> >>> } else {
> >>> - return __is_bitmap_valid(sbi, blkaddr, type);
> >>> + valid = __is_bitmap_valid(sbi, blkaddr, type);
> >>> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> >>> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> >>> + goto err;
> >
> > Please think about how to optimize this, which is really ugly now.
>
> How about calling f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR) inside
> __is_bitmap_valid()? so that we may not add such logic outside
> __is_bitmap_valid()...
>
> Thanks,
Dear Jaegeuk,
I agree with Chao's options.
The original intention of this patch is that some failed cases of
f2fs_is_valid_blkaddr
are not processed by f2fs_handle_error, so here do unified processing.
Is it a good way to keep the original main logic of __is_bitmap_valid ?
Do you have any other suggestions?
thanks!
>
> >
> >>> }
> >>> break;
> >>> case META_GENERIC:
> >>> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
> >>> blkaddr >= MAIN_BLKADDR(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> default:
> >>> BUG();
> >>> }
> >>> return true;
> >>> +err:
> >>> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + return valid;
> >>> }
> >>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 05158f8..300f9ae 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> >>> fio->is_por ? META_POR : (__is_meta_io(fio) ?
> >>> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> + META_GENERIC : DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> trace_f2fs_submit_page_bio(page, fio);
> >>> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> >>> fio->encrypted_page : fio->page;
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> >>> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> trace_f2fs_submit_page_bio(page, fio);
> >>> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> >>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto put_err;
> >>> }
> >>> goto got_it;
> >>> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> >>> dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto put_err;
> >>> }
> >>> got_it:
> >>> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
> >>> if (!is_hole &&
> >>> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto sync_out;
> >>> }
> >>> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
> >>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
> >>> } else {
> >>> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> >>> f2fs_lookup_read_extent_cache_block(inode, page->index,
> >>> &fio->old_blkaddr)) {
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> >>> - DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_handle_error(fio->sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> ipu_force = true;
> >>> fio->need_lock = LOCK_DONE;
> >>> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> >>> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> goto out_writepage;
> >>> }
> >>> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> >>> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto fail;
> >>> }
> >>> err = f2fs_submit_page_read(use_cow ?
> >>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >>> index ad8dfac7..48048fa 100644
> >>> --- a/fs/f2fs/extent_cache.c
> >>> +++ b/fs/f2fs/extent_cache.c
> >>> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
> >>> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
> >>> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
> >>> __func__, inode->i_ino,
> >>> ei->blk, ei->fofs, ei->len);
> >>> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
> >>> goto out;
> >>> if (__is_valid_data_blkaddr(blkaddr) &&
> >>> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_bug_on(sbi, 1);
> >>> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
> >>> return -EINVAL;
> >>> - }
> >>> out:
> >>> /*
> >>> * init block age with zero, this can happen when the block age extent
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index 25b119cf..23cd6a1 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> >>> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
> >>> continue;
> >>> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE))
> >>> continue;
> >>> - }
> >>> if (compressed_cluster)
> >>> valid_blocks++;
> >>> }
> >>> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
> >>> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> f2fs_put_dnode(&dn);
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> return -EFSCORRUPTED;
> >>> }
> >>> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
> >>> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> break;
> >>> }
> >>> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> >>> if (!__is_valid_data_blkaddr(blkaddr))
> >>> continue;
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> }
> >>> while (count) {
> >>> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> >>> if (!__is_valid_data_blkaddr(blkaddr))
> >>> continue;
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> }
> >>> while (count) {
> >>> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> >>> DATA_GENERIC_ENHANCE)) {
> >>> ret = -EFSCORRUPTED;
> >>> f2fs_put_dnode(&dn);
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>> index a079eeb..30e93d8 100644
> >>> --- a/fs/f2fs/gc.c
> >>> +++ b/fs/f2fs/gc.c
> >>> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ))) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto put_page;
> >>> }
> >>> goto got_it;
> >>> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE))) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto put_page;
> >>> }
> >>> got_it:
> >>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>> index aad1d1a..289c0bf 100644
> >>> --- a/fs/f2fs/recovery.c
> >>> +++ b/fs/f2fs/recovery.c
> >>> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> >>> if (__is_valid_data_blkaddr(src) &&
> >>> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> if (__is_valid_data_blkaddr(dest) &&
> >>> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> >>> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
> >>> dest, inode->i_ino, dn.ofs_in_node);
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index 7901ede..ad6511f 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> >>> DATA_GENERIC_ENHANCE)) {
> >>> f2fs_put_dnode(&dn);
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-06 3:32 ` Jaegeuk Kim
2024-02-06 3:36 ` Chao Yu
@ 2024-02-19 3:46 ` Chao Yu
2024-02-19 11:19 ` Zhiguo Niu
1 sibling, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-19 3:46 UTC (permalink / raw)
To: Jaegeuk Kim, Zhiguo Niu
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang, hongyu.jin
On 2024/2/6 11:32, Jaegeuk Kim wrote:
> On 02/05, Chao Yu wrote:
>> On 2024/2/5 11:30, Zhiguo Niu wrote:
>>> There are some cases of f2fs_is_valid_blkaddr not handled as
>>> ERROR_INVALID_BLKADDR,so unify the error handling about all of
>>> f2fs_is_valid_blkaddr.
>>>
>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>> ---
>>> changes of v7: update patch according to sync with Chao
>>> -restore some code to original
>>> -modify err handle of __is_bitmap_valid for covering all cases
>>> changes of v6: improve patch according to Chao's suggestions
>>> -restore dump_stack to original position
>>> -adjuest code sequence of __is_bitmap_check_valid
>>> changes of v5: improve patch according to Jaegeuk's suggestiongs
>>> -restore return value of some f2fs_is_valid_blkaddr error case to original
>>> -move cp_err checking to outermost for unified processing
>>> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
>>> __is_bitmap_valid to avoid meaningless flow
>>> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
>>> and handling its return value in the caller uniformly, also cooperate
>>> switch checking true to false for error case of
>>> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
>>> for more readable
>>> changes of v4: update according to the latest code
>>> changes of v3:
>>> -rebase patch to dev-test
>>> -correct return value for some f2fs_is_valid_blkaddr error case
>>> changes of v2: improve patch according Chao's suggestions.
>>> ---
>>> ---
>>> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
>>> fs/f2fs/data.c | 22 +++-------------------
>>> fs/f2fs/extent_cache.c | 5 +----
>>> fs/f2fs/file.c | 16 +++-------------
>>> fs/f2fs/gc.c | 2 --
>>> fs/f2fs/recovery.c | 4 ----
>>> fs/f2fs/segment.c | 2 --
>>> 7 files changed, 25 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index b85820e..3335619 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>>> if (unlikely(f2fs_cp_error(sbi)))
>>> return exist;
>>> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
>>> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>> - blkaddr, exist);
>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> - return exist;
>>> - }
>>> -
>>> - if (!exist && type == DATA_GENERIC_ENHANCE) {
>>> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
>>> + (!exist && type == DATA_GENERIC_ENHANCE)) {
>>> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>> blkaddr, exist);
>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> dump_stack();
>>> }
>>> +
>>
>> No need to add one blank line.
>>
>> Otherwise, it looks good to me.
>>
>> Reviewed-by: Chao Yu <chao@kernel.org>
>>
>> Thanks,
>>
>>> return exist;
>>> }
>>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> block_t blkaddr, int type)
>>> {
>>> + bool valid = false;
>>> +
>>> switch (type) {
>>> case META_NAT:
>>> break;
>>> case META_SIT:
>>> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_SSA:
>>> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
>>> blkaddr < SM_I(sbi)->ssa_blkaddr))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_CP:
>>> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
>>> blkaddr < __start_cp_addr(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case META_POR:
>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>> blkaddr < MAIN_BLKADDR(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> case DATA_GENERIC:
>>> case DATA_GENERIC_ENHANCE:
>>> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> blkaddr);
>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> dump_stack();
>>> - return false;
>>> + goto err;
>>> } else {
>>> - return __is_bitmap_valid(sbi, blkaddr, type);
>>> + valid = __is_bitmap_valid(sbi, blkaddr, type);
>>> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
>>> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
>>> + goto err;
>
> Please think about how to optimize this, which is really ugly now.
How about this?
---
fs/f2fs/checkpoint.c | 16 +++++++++-------
1 file changed, 9 insertions(+), 7 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 246b2a063cfb..5a6ac6f26cfe 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
}
static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
- int type)
+ int type, bool *record_error)
{
struct seg_entry *se;
unsigned int segno, offset;
@@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
blkaddr, exist);
set_sbi_flag(sbi, SBI_NEED_FSCK);
dump_stack();
+ *record_error = true;
}
return exist;
@@ -168,8 +169,6 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type)
{
- bool valid = false;
-
switch (type) {
case META_NAT:
break;
@@ -209,10 +208,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
dump_stack();
goto err;
} else {
- valid = __is_bitmap_valid(sbi, blkaddr, type);
- if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
- (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
+ bool valid, record_error = false;
+
+ valid = __is_bitmap_valid(sbi, blkaddr, type,
+ &record_error);
+ if (!valid || record_error)
goto err;
+ return valid;
}
break;
case META_GENERIC:
@@ -227,7 +229,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
return true;
err:
f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
- return valid;
+ return false;
}
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
--
2.40.1
>
>>> }
>>> break;
>>> case META_GENERIC:
>>> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
>>> blkaddr >= MAIN_BLKADDR(sbi)))
>>> - return false;
>>> + goto err;
>>> break;
>>> default:
>>> BUG();
>>> }
>>> return true;
>>> +err:
>>> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + return valid;
>>> }
>>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>> index 05158f8..300f9ae 100644
>>> --- a/fs/f2fs/data.c
>>> +++ b/fs/f2fs/data.c
>>> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>> fio->is_por ? META_POR : (__is_meta_io(fio) ?
>>> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> + META_GENERIC : DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> trace_f2fs_submit_page_bio(page, fio);
>>> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>>> fio->encrypted_page : fio->page;
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
>>> return -EFSCORRUPTED;
>>> - }
>>> trace_f2fs_submit_page_bio(page, fio);
>>> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto put_err;
>>> }
>>> goto got_it;
>>> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>> dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto put_err;
>>> }
>>> got_it:
>>> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>> if (!is_hole &&
>>> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto sync_out;
>>> }
>>> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
>>> } else {
>>> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>> f2fs_lookup_read_extent_cache_block(inode, page->index,
>>> &fio->old_blkaddr)) {
>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>> - DATA_GENERIC_ENHANCE)) {
>>> - f2fs_handle_error(fio->sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE))
>>> return -EFSCORRUPTED;
>>> - }
>>> ipu_force = true;
>>> fio->need_lock = LOCK_DONE;
>>> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>> goto out_writepage;
>>> }
>>> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>>> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> DATA_GENERIC_ENHANCE_READ)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto fail;
>>> }
>>> err = f2fs_submit_page_read(use_cow ?
>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>> index ad8dfac7..48048fa 100644
>>> --- a/fs/f2fs/extent_cache.c
>>> +++ b/fs/f2fs/extent_cache.c
>>> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
>>> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
>>> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
>>> DATA_GENERIC_ENHANCE)) {
>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
>>> __func__, inode->i_ino,
>>> ei->blk, ei->fofs, ei->len);
>>> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
>>> goto out;
>>> if (__is_valid_data_blkaddr(blkaddr) &&
>>> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>> - f2fs_bug_on(sbi, 1);
>>> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
>>> return -EINVAL;
>>> - }
>>> out:
>>> /*
>>> * init block age with zero, this can happen when the block age extent
>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>> index 25b119cf..23cd6a1 100644
>>> --- a/fs/f2fs/file.c
>>> +++ b/fs/f2fs/file.c
>>> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>>> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
>>> continue;
>>> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE)) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE))
>>> continue;
>>> - }
>>> if (compressed_cluster)
>>> valid_blocks++;
>>> }
>>> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
>>> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> f2fs_put_dnode(&dn);
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> return -EFSCORRUPTED;
>>> }
>>> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
>>> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
>>> DATA_GENERIC_ENHANCE)) {
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> break;
>>> }
>>> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>> if (!__is_valid_data_blkaddr(blkaddr))
>>> continue;
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> }
>>> while (count) {
>>> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>> if (!__is_valid_data_blkaddr(blkaddr))
>>> continue;
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>> - DATA_GENERIC_ENHANCE))) {
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> + DATA_GENERIC_ENHANCE)))
>>> return -EFSCORRUPTED;
>>> - }
>>> }
>>> while (count) {
>>> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
>>> DATA_GENERIC_ENHANCE)) {
>>> ret = -EFSCORRUPTED;
>>> f2fs_put_dnode(&dn);
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index a079eeb..30e93d8 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE_READ))) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto put_page;
>>> }
>>> goto got_it;
>>> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>> DATA_GENERIC_ENHANCE))) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto put_page;
>>> }
>>> got_it:
>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>> index aad1d1a..289c0bf 100644
>>> --- a/fs/f2fs/recovery.c
>>> +++ b/fs/f2fs/recovery.c
>>> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>> if (__is_valid_data_blkaddr(src) &&
>>> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> if (__is_valid_data_blkaddr(dest) &&
>>> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
>>> dest, inode->i_ino, dn.ofs_in_node);
>>> err = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto err;
>>> }
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 7901ede..ad6511f 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>> DATA_GENERIC_ENHANCE)) {
>>> f2fs_put_dnode(&dn);
>>> ret = -EFSCORRUPTED;
>>> - f2fs_handle_error(sbi,
>>> - ERROR_INVALID_BLKADDR);
>>> goto out;
>>> }
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-19 3:46 ` Chao Yu
@ 2024-02-19 11:19 ` Zhiguo Niu
2024-02-19 14:36 ` Chao Yu
0 siblings, 1 reply; 13+ messages in thread
From: Zhiguo Niu @ 2024-02-19 11:19 UTC (permalink / raw)
To: Chao Yu
Cc: Jaegeuk Kim, Zhiguo Niu, linux-f2fs-devel, linux-kernel, ke.wang,
hongyu.jin
Dear Chao
On Mon, Feb 19, 2024 at 11:46 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/2/6 11:32, Jaegeuk Kim wrote:
> > On 02/05, Chao Yu wrote:
> >> On 2024/2/5 11:30, Zhiguo Niu wrote:
> >>> There are some cases of f2fs_is_valid_blkaddr not handled as
> >>> ERROR_INVALID_BLKADDR,so unify the error handling about all of
> >>> f2fs_is_valid_blkaddr.
> >>>
> >>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> >>> Signed-off-by: Chao Yu <chao@kernel.org>
> >>> ---
> >>> changes of v7: update patch according to sync with Chao
> >>> -restore some code to original
> >>> -modify err handle of __is_bitmap_valid for covering all cases
> >>> changes of v6: improve patch according to Chao's suggestions
> >>> -restore dump_stack to original position
> >>> -adjuest code sequence of __is_bitmap_check_valid
> >>> changes of v5: improve patch according to Jaegeuk's suggestiongs
> >>> -restore return value of some f2fs_is_valid_blkaddr error case to original
> >>> -move cp_err checking to outermost for unified processing
> >>> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
> >>> __is_bitmap_valid to avoid meaningless flow
> >>> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
> >>> and handling its return value in the caller uniformly, also cooperate
> >>> switch checking true to false for error case of
> >>> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
> >>> for more readable
> >>> changes of v4: update according to the latest code
> >>> changes of v3:
> >>> -rebase patch to dev-test
> >>> -correct return value for some f2fs_is_valid_blkaddr error case
> >>> changes of v2: improve patch according Chao's suggestions.
> >>> ---
> >>> ---
> >>> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
> >>> fs/f2fs/data.c | 22 +++-------------------
> >>> fs/f2fs/extent_cache.c | 5 +----
> >>> fs/f2fs/file.c | 16 +++-------------
> >>> fs/f2fs/gc.c | 2 --
> >>> fs/f2fs/recovery.c | 4 ----
> >>> fs/f2fs/segment.c | 2 --
> >>> 7 files changed, 25 insertions(+), 59 deletions(-)
> >>>
> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >>> index b85820e..3335619 100644
> >>> --- a/fs/f2fs/checkpoint.c
> >>> +++ b/fs/f2fs/checkpoint.c
> >>> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> >>> if (unlikely(f2fs_cp_error(sbi)))
> >>> return exist;
> >>> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
> >>> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> >>> - blkaddr, exist);
> >>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> - return exist;
> >>> - }
> >>> -
> >>> - if (!exist && type == DATA_GENERIC_ENHANCE) {
> >>> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> >>> + (!exist && type == DATA_GENERIC_ENHANCE)) {
> >>> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> >>> blkaddr, exist);
> >>> set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> dump_stack();
> >>> }
> >>> +
> >>
> >> No need to add one blank line.
> >>
> >> Otherwise, it looks good to me.
> >>
> >> Reviewed-by: Chao Yu <chao@kernel.org>
> >>
> >> Thanks,
> >>
> >>> return exist;
> >>> }
> >>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> block_t blkaddr, int type)
> >>> {
> >>> + bool valid = false;
> >>> +
> >>> switch (type) {
> >>> case META_NAT:
> >>> break;
> >>> case META_SIT:
> >>> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_SSA:
> >>> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
> >>> blkaddr < SM_I(sbi)->ssa_blkaddr))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_CP:
> >>> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
> >>> blkaddr < __start_cp_addr(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case META_POR:
> >>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
> >>> blkaddr < MAIN_BLKADDR(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> case DATA_GENERIC:
> >>> case DATA_GENERIC_ENHANCE:
> >>> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> blkaddr);
> >>> set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> dump_stack();
> >>> - return false;
> >>> + goto err;
> >>> } else {
> >>> - return __is_bitmap_valid(sbi, blkaddr, type);
> >>> + valid = __is_bitmap_valid(sbi, blkaddr, type);
> >>> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> >>> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> >>> + goto err;
> >
> > Please think about how to optimize this, which is really ugly now.
>
> How about this?
>
> ---
> fs/f2fs/checkpoint.c | 16 +++++++++-------
> 1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 246b2a063cfb..5a6ac6f26cfe 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
> }
>
> static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> - int type)
> + int type, bool *record_error)
> {
> struct seg_entry *se;
> unsigned int segno, offset;
> @@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> blkaddr, exist);
> set_sbi_flag(sbi, SBI_NEED_FSCK);
> dump_stack();
> + *record_error = true;
> }
>
> return exist;
> @@ -168,8 +169,6 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> block_t blkaddr, int type)
> {
> - bool valid = false;
> -
> switch (type) {
> case META_NAT:
> break;
> @@ -209,10 +208,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> dump_stack();
> goto err;
> } else {
> - valid = __is_bitmap_valid(sbi, blkaddr, type);
> - if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> - (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> + bool valid, record_error = false;
> +
> + valid = __is_bitmap_valid(sbi, blkaddr, type,
> + &record_error);
> + if (!valid || record_error)
> goto err;
> + return valid;
> }
> break;
> case META_GENERIC:
> @@ -227,7 +229,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> return true;
> err:
> f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> - return valid;
> + return false;
for case: (type == DATA_GENERIC_ENHANCE_UPDATE and bitmap check valid）,
true should be returned, but here return false, it is not as expected, right?
> }
>
> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> --
> 2.40.1
>
>
>
> >
> >>> }
> >>> break;
> >>> case META_GENERIC:
> >>> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
> >>> blkaddr >= MAIN_BLKADDR(sbi)))
> >>> - return false;
> >>> + goto err;
> >>> break;
> >>> default:
> >>> BUG();
> >>> }
> >>> return true;
> >>> +err:
> >>> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + return valid;
> >>> }
> >>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> >>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> >>> index 05158f8..300f9ae 100644
> >>> --- a/fs/f2fs/data.c
> >>> +++ b/fs/f2fs/data.c
> >>> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> >>> fio->is_por ? META_POR : (__is_meta_io(fio) ?
> >>> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> + META_GENERIC : DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> trace_f2fs_submit_page_bio(page, fio);
> >>> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
> >>> fio->encrypted_page : fio->page;
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
> >>> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> trace_f2fs_submit_page_bio(page, fio);
> >>> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> >>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto put_err;
> >>> }
> >>> goto got_it;
> >>> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
> >>> dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto put_err;
> >>> }
> >>> got_it:
> >>> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
> >>> if (!is_hole &&
> >>> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto sync_out;
> >>> }
> >>> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
> >>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(F2FS_I_SB(inode),
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
> >>> } else {
> >>> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> >>> f2fs_lookup_read_extent_cache_block(inode, page->index,
> >>> &fio->old_blkaddr)) {
> >>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> >>> - DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_handle_error(fio->sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> ipu_force = true;
> >>> fio->need_lock = LOCK_DONE;
> >>> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
> >>> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
> >>> goto out_writepage;
> >>> }
> >>> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
> >>> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto fail;
> >>> }
> >>> err = f2fs_submit_page_read(use_cow ?
> >>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> >>> index ad8dfac7..48048fa 100644
> >>> --- a/fs/f2fs/extent_cache.c
> >>> +++ b/fs/f2fs/extent_cache.c
> >>> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
> >>> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
> >>> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
> >>> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
> >>> __func__, inode->i_ino,
> >>> ei->blk, ei->fofs, ei->len);
> >>> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
> >>> goto out;
> >>> if (__is_valid_data_blkaddr(blkaddr) &&
> >>> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_bug_on(sbi, 1);
> >>> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
> >>> return -EINVAL;
> >>> - }
> >>> out:
> >>> /*
> >>> * init block age with zero, this can happen when the block age extent
> >>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> >>> index 25b119cf..23cd6a1 100644
> >>> --- a/fs/f2fs/file.c
> >>> +++ b/fs/f2fs/file.c
> >>> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
> >>> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
> >>> continue;
> >>> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE)) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE))
> >>> continue;
> >>> - }
> >>> if (compressed_cluster)
> >>> valid_blocks++;
> >>> }
> >>> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
> >>> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> f2fs_put_dnode(&dn);
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> return -EFSCORRUPTED;
> >>> }
> >>> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
> >>> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
> >>> DATA_GENERIC_ENHANCE)) {
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> break;
> >>> }
> >>> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> >>> if (!__is_valid_data_blkaddr(blkaddr))
> >>> continue;
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> }
> >>> while (count) {
> >>> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
> >>> if (!__is_valid_data_blkaddr(blkaddr))
> >>> continue;
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
> >>> - DATA_GENERIC_ENHANCE))) {
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> + DATA_GENERIC_ENHANCE)))
> >>> return -EFSCORRUPTED;
> >>> - }
> >>> }
> >>> while (count) {
> >>> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
> >>> DATA_GENERIC_ENHANCE)) {
> >>> ret = -EFSCORRUPTED;
> >>> f2fs_put_dnode(&dn);
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
> >>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> >>> index a079eeb..30e93d8 100644
> >>> --- a/fs/f2fs/gc.c
> >>> +++ b/fs/f2fs/gc.c
> >>> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE_READ))) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto put_page;
> >>> }
> >>> goto got_it;
> >>> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
> >>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
> >>> DATA_GENERIC_ENHANCE))) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto put_page;
> >>> }
> >>> got_it:
> >>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
> >>> index aad1d1a..289c0bf 100644
> >>> --- a/fs/f2fs/recovery.c
> >>> +++ b/fs/f2fs/recovery.c
> >>> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> >>> if (__is_valid_data_blkaddr(src) &&
> >>> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> if (__is_valid_data_blkaddr(dest) &&
> >>> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
> >>> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
> >>> dest, inode->i_ino, dn.ofs_in_node);
> >>> err = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto err;
> >>> }
> >>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >>> index 7901ede..ad6511f 100644
> >>> --- a/fs/f2fs/segment.c
> >>> +++ b/fs/f2fs/segment.c
> >>> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
> >>> DATA_GENERIC_ENHANCE)) {
> >>> f2fs_put_dnode(&dn);
> >>> ret = -EFSCORRUPTED;
> >>> - f2fs_handle_error(sbi,
> >>> - ERROR_INVALID_BLKADDR);
> >>> goto out;
> >>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-19 11:19 ` Zhiguo Niu
@ 2024-02-19 14:36 ` Chao Yu
2024-02-20 2:36 ` [f2fs-dev] " Chao Yu
0 siblings, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-19 14:36 UTC (permalink / raw)
To: Zhiguo Niu
Cc: Jaegeuk Kim, Zhiguo Niu, linux-f2fs-devel, linux-kernel, ke.wang,
hongyu.jin
On 2024/2/19 19:19, Zhiguo Niu wrote:
> Dear Chao
>
> On Mon, Feb 19, 2024 at 11:46 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/2/6 11:32, Jaegeuk Kim wrote:
>>> On 02/05, Chao Yu wrote:
>>>> On 2024/2/5 11:30, Zhiguo Niu wrote:
>>>>> There are some cases of f2fs_is_valid_blkaddr not handled as
>>>>> ERROR_INVALID_BLKADDR,so unify the error handling about all of
>>>>> f2fs_is_valid_blkaddr.
>>>>>
>>>>> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
>>>>> Signed-off-by: Chao Yu <chao@kernel.org>
>>>>> ---
>>>>> changes of v7: update patch according to sync with Chao
>>>>> -restore some code to original
>>>>> -modify err handle of __is_bitmap_valid for covering all cases
>>>>> changes of v6: improve patch according to Chao's suggestions
>>>>> -restore dump_stack to original position
>>>>> -adjuest code sequence of __is_bitmap_check_valid
>>>>> changes of v5: improve patch according to Jaegeuk's suggestiongs
>>>>> -restore return value of some f2fs_is_valid_blkaddr error case to original
>>>>> -move cp_err checking to outermost for unified processing
>>>>> -return true directly for case (type=DATA_GENERIC_ENHANCE_READ) in
>>>>> __is_bitmap_valid to avoid meaningless flow
>>>>> -rename __is_bitmap_valid to __is_bitmap_check_valid for avoiding ambiguity
>>>>> and handling its return value in the caller uniformly, also cooperate
>>>>> switch checking true to false for error case of
>>>>> f2fs_is_valid_blkaddr(type=DATA_GENERIC_ENHANCE_UPDATE) in do_recover_data
>>>>> for more readable
>>>>> changes of v4: update according to the latest code
>>>>> changes of v3:
>>>>> -rebase patch to dev-test
>>>>> -correct return value for some f2fs_is_valid_blkaddr error case
>>>>> changes of v2: improve patch according Chao's suggestions.
>>>>> ---
>>>>> ---
>>>>> fs/f2fs/checkpoint.c | 33 ++++++++++++++++++---------------
>>>>> fs/f2fs/data.c | 22 +++-------------------
>>>>> fs/f2fs/extent_cache.c | 5 +----
>>>>> fs/f2fs/file.c | 16 +++-------------
>>>>> fs/f2fs/gc.c | 2 --
>>>>> fs/f2fs/recovery.c | 4 ----
>>>>> fs/f2fs/segment.c | 2 --
>>>>> 7 files changed, 25 insertions(+), 59 deletions(-)
>>>>>
>>>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>>>> index b85820e..3335619 100644
>>>>> --- a/fs/f2fs/checkpoint.c
>>>>> +++ b/fs/f2fs/checkpoint.c
>>>>> @@ -154,46 +154,43 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>>>>> if (unlikely(f2fs_cp_error(sbi)))
>>>>> return exist;
>>>>> - if (exist && type == DATA_GENERIC_ENHANCE_UPDATE) {
>>>>> - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>>>> - blkaddr, exist);
>>>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> - return exist;
>>>>> - }
>>>>> -
>>>>> - if (!exist && type == DATA_GENERIC_ENHANCE) {
>>>>> + if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
>>>>> + (!exist && type == DATA_GENERIC_ENHANCE)) {
>>>>> f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
>>>>> blkaddr, exist);
>>>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> dump_stack();
>>>>> }
>>>>> +
>>>>
>>>> No need to add one blank line.
>>>>
>>>> Otherwise, it looks good to me.
>>>>
>>>> Reviewed-by: Chao Yu <chao@kernel.org>
>>>>
>>>> Thanks,
>>>>
>>>>> return exist;
>>>>> }
>>>>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>>>> block_t blkaddr, int type)
>>>>> {
>>>>> + bool valid = false;
>>>>> +
>>>>> switch (type) {
>>>>> case META_NAT:
>>>>> break;
>>>>> case META_SIT:
>>>>> if (unlikely(blkaddr >= SIT_BLK_CNT(sbi)))
>>>>> - return false;
>>>>> + goto err;
>>>>> break;
>>>>> case META_SSA:
>>>>> if (unlikely(blkaddr >= MAIN_BLKADDR(sbi) ||
>>>>> blkaddr < SM_I(sbi)->ssa_blkaddr))
>>>>> - return false;
>>>>> + goto err;
>>>>> break;
>>>>> case META_CP:
>>>>> if (unlikely(blkaddr >= SIT_I(sbi)->sit_base_addr ||
>>>>> blkaddr < __start_cp_addr(sbi)))
>>>>> - return false;
>>>>> + goto err;
>>>>> break;
>>>>> case META_POR:
>>>>> if (unlikely(blkaddr >= MAX_BLKADDR(sbi) ||
>>>>> blkaddr < MAIN_BLKADDR(sbi)))
>>>>> - return false;
>>>>> + goto err;
>>>>> break;
>>>>> case DATA_GENERIC:
>>>>> case DATA_GENERIC_ENHANCE:
>>>>> @@ -210,21 +207,27 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>>>> blkaddr);
>>>>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> dump_stack();
>>>>> - return false;
>>>>> + goto err;
>>>>> } else {
>>>>> - return __is_bitmap_valid(sbi, blkaddr, type);
>>>>> + valid = __is_bitmap_valid(sbi, blkaddr, type);
>>>>> + if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
>>>>> + (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
>>>>> + goto err;
>>>
>>> Please think about how to optimize this, which is really ugly now.
>>
>> How about this?
>>
>> ---
>> fs/f2fs/checkpoint.c | 16 +++++++++-------
>> 1 file changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 246b2a063cfb..5a6ac6f26cfe 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
>> }
>>
>> static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>> - int type)
>> + int type, bool *record_error)
>> {
>> struct seg_entry *se;
>> unsigned int segno, offset;
>> @@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>> blkaddr, exist);
>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>> dump_stack();
>> + *record_error = true;
>> }
>>
>> return exist;
>> @@ -168,8 +169,6 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>> static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>> block_t blkaddr, int type)
>> {
>> - bool valid = false;
>> -
>> switch (type) {
>> case META_NAT:
>> break;
>> @@ -209,10 +208,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>> dump_stack();
>> goto err;
>> } else {
>> - valid = __is_bitmap_valid(sbi, blkaddr, type);
>> - if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
>> - (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
>> + bool valid, record_error = false;
>> +
>> + valid = __is_bitmap_valid(sbi, blkaddr, type,
>> + &record_error);
>> + if (!valid || record_error)
>> goto err;
>> + return valid;
>> }
>> break;
>> case META_GENERIC:
>> @@ -227,7 +229,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>> return true;
>> err:
>> f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>> - return valid;
>> + return false;
> for case: (type == DATA_GENERIC_ENHANCE_UPDATE and bitmap check valid）,
> true should be returned, but here return false, it is not as expected, right?
Yes, let me revise soon. :)
Thanks,
>> }
>>
>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>> --
>> 2.40.1
>>
>>
>>
>>>
>>>>> }
>>>>> break;
>>>>> case META_GENERIC:
>>>>> if (unlikely(blkaddr < SEG0_BLKADDR(sbi) ||
>>>>> blkaddr >= MAIN_BLKADDR(sbi)))
>>>>> - return false;
>>>>> + goto err;
>>>>> break;
>>>>> default:
>>>>> BUG();
>>>>> }
>>>>> return true;
>>>>> +err:
>>>>> + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> + return valid;
>>>>> }
>>>>> bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>>>>> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
>>>>> index 05158f8..300f9ae 100644
>>>>> --- a/fs/f2fs/data.c
>>>>> +++ b/fs/f2fs/data.c
>>>>> @@ -738,10 +738,8 @@ int f2fs_submit_page_bio(struct f2fs_io_info *fio)
>>>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>>>> fio->is_por ? META_POR : (__is_meta_io(fio) ?
>>>>> - META_GENERIC : DATA_GENERIC_ENHANCE))) {
>>>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>>>> + META_GENERIC : DATA_GENERIC_ENHANCE)))
>>>>> return -EFSCORRUPTED;
>>>>> - }
>>>>> trace_f2fs_submit_page_bio(page, fio);
>>>>> @@ -946,10 +944,8 @@ int f2fs_merge_page_bio(struct f2fs_io_info *fio)
>>>>> fio->encrypted_page : fio->page;
>>>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->new_blkaddr,
>>>>> - __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC)) {
>>>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>>>> + __is_meta_io(fio) ? META_GENERIC : DATA_GENERIC))
>>>>> return -EFSCORRUPTED;
>>>>> - }
>>>>> trace_f2fs_submit_page_bio(page, fio);
>>>>> @@ -1286,8 +1282,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), dn.data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE_READ)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto put_err;
>>>>> }
>>>>> goto got_it;
>>>>> @@ -1313,8 +1307,6 @@ struct page *f2fs_get_read_data_page(struct inode *inode, pgoff_t index,
>>>>> dn.data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto put_err;
>>>>> }
>>>>> got_it:
>>>>> @@ -1642,7 +1634,6 @@ int f2fs_map_blocks(struct inode *inode, struct f2fs_map_blocks *map, int flag)
>>>>> if (!is_hole &&
>>>>> !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto sync_out;
>>>>> }
>>>>> @@ -2166,8 +2157,6 @@ static int f2fs_read_single_page(struct inode *inode, struct page *page,
>>>>> if (!f2fs_is_valid_blkaddr(F2FS_I_SB(inode), block_nr,
>>>>> DATA_GENERIC_ENHANCE_READ)) {
>>>>> ret = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(F2FS_I_SB(inode),
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto out;
>>>>> }
>>>>> } else {
>>>>> @@ -2707,11 +2696,8 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>>>> f2fs_lookup_read_extent_cache_block(inode, page->index,
>>>>> &fio->old_blkaddr)) {
>>>>> if (!f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>>>> - DATA_GENERIC_ENHANCE)) {
>>>>> - f2fs_handle_error(fio->sbi,
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> + DATA_GENERIC_ENHANCE))
>>>>> return -EFSCORRUPTED;
>>>>> - }
>>>>> ipu_force = true;
>>>>> fio->need_lock = LOCK_DONE;
>>>>> @@ -2739,7 +2725,6 @@ int f2fs_do_write_data_page(struct f2fs_io_info *fio)
>>>>> !f2fs_is_valid_blkaddr(fio->sbi, fio->old_blkaddr,
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(fio->sbi, ERROR_INVALID_BLKADDR);
>>>>> goto out_writepage;
>>>>> }
>>>>> @@ -3706,7 +3691,6 @@ static int f2fs_write_begin(struct file *file, struct address_space *mapping,
>>>>> if (!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>>>> DATA_GENERIC_ENHANCE_READ)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto fail;
>>>>> }
>>>>> err = f2fs_submit_page_read(use_cow ?
>>>>> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
>>>>> index ad8dfac7..48048fa 100644
>>>>> --- a/fs/f2fs/extent_cache.c
>>>>> +++ b/fs/f2fs/extent_cache.c
>>>>> @@ -43,7 +43,6 @@ bool sanity_check_extent_cache(struct inode *inode)
>>>>> if (!f2fs_is_valid_blkaddr(sbi, ei->blk, DATA_GENERIC_ENHANCE) ||
>>>>> !f2fs_is_valid_blkaddr(sbi, ei->blk + ei->len - 1,
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> - set_sbi_flag(sbi, SBI_NEED_FSCK);
>>>>> f2fs_warn(sbi, "%s: inode (ino=%lx) extent info [%u, %u, %u] is incorrect, run fsck to fix",
>>>>> __func__, inode->i_ino,
>>>>> ei->blk, ei->fofs, ei->len);
>>>>> @@ -856,10 +855,8 @@ static int __get_new_block_age(struct inode *inode, struct extent_info *ei,
>>>>> goto out;
>>>>> if (__is_valid_data_blkaddr(blkaddr) &&
>>>>> - !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE)) {
>>>>> - f2fs_bug_on(sbi, 1);
>>>>> + !f2fs_is_valid_blkaddr(sbi, blkaddr, DATA_GENERIC_ENHANCE))
>>>>> return -EINVAL;
>>>>> - }
>>>>> out:
>>>>> /*
>>>>> * init block age with zero, this can happen when the block age extent
>>>>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>>>>> index 25b119cf..23cd6a1 100644
>>>>> --- a/fs/f2fs/file.c
>>>>> +++ b/fs/f2fs/file.c
>>>>> @@ -593,10 +593,8 @@ void f2fs_truncate_data_blocks_range(struct dnode_of_data *dn, int count)
>>>>> if (time_to_inject(sbi, FAULT_BLKADDR_CONSISTENCE))
>>>>> continue;
>>>>> if (!f2fs_is_valid_blkaddr_raw(sbi, blkaddr,
>>>>> - DATA_GENERIC_ENHANCE)) {
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> + DATA_GENERIC_ENHANCE))
>>>>> continue;
>>>>> - }
>>>>> if (compressed_cluster)
>>>>> valid_blocks++;
>>>>> }
>>>>> @@ -1196,7 +1194,6 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
>>>>> !f2fs_is_valid_blkaddr(sbi, *blkaddr,
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> f2fs_put_dnode(&dn);
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> return -EFSCORRUPTED;
>>>>> }
>>>>> @@ -1482,7 +1479,6 @@ static int f2fs_do_zero_range(struct dnode_of_data *dn, pgoff_t start,
>>>>> if (!f2fs_is_valid_blkaddr(sbi, dn->data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> ret = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> break;
>>>>> }
>>>>> @@ -3442,10 +3438,8 @@ static int release_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>>>> if (!__is_valid_data_blkaddr(blkaddr))
>>>>> continue;
>>>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>>>> - DATA_GENERIC_ENHANCE))) {
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> + DATA_GENERIC_ENHANCE)))
>>>>> return -EFSCORRUPTED;
>>>>> - }
>>>>> }
>>>>> while (count) {
>>>>> @@ -3607,10 +3601,8 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count)
>>>>> if (!__is_valid_data_blkaddr(blkaddr))
>>>>> continue;
>>>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, blkaddr,
>>>>> - DATA_GENERIC_ENHANCE))) {
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> + DATA_GENERIC_ENHANCE)))
>>>>> return -EFSCORRUPTED;
>>>>> - }
>>>>> }
>>>>> while (count) {
>>>>> @@ -3894,8 +3886,6 @@ static int f2fs_sec_trim_file(struct file *filp, unsigned long arg)
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> ret = -EFSCORRUPTED;
>>>>> f2fs_put_dnode(&dn);
>>>>> - f2fs_handle_error(sbi,
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto out;
>>>>> }
>>>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>>>> index a079eeb..30e93d8 100644
>>>>> --- a/fs/f2fs/gc.c
>>>>> +++ b/fs/f2fs/gc.c
>>>>> @@ -1197,7 +1197,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE_READ))) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto put_page;
>>>>> }
>>>>> goto got_it;
>>>>> @@ -1216,7 +1215,6 @@ static int ra_data_block(struct inode *inode, pgoff_t index)
>>>>> if (unlikely(!f2fs_is_valid_blkaddr(sbi, dn.data_blkaddr,
>>>>> DATA_GENERIC_ENHANCE))) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto put_page;
>>>>> }
>>>>> got_it:
>>>>> diff --git a/fs/f2fs/recovery.c b/fs/f2fs/recovery.c
>>>>> index aad1d1a..289c0bf 100644
>>>>> --- a/fs/f2fs/recovery.c
>>>>> +++ b/fs/f2fs/recovery.c
>>>>> @@ -693,14 +693,12 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>>>> if (__is_valid_data_blkaddr(src) &&
>>>>> !f2fs_is_valid_blkaddr(sbi, src, META_POR)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto err;
>>>>> }
>>>>> if (__is_valid_data_blkaddr(dest) &&
>>>>> !f2fs_is_valid_blkaddr(sbi, dest, META_POR)) {
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
>>>>> goto err;
>>>>> }
>>>>> @@ -755,8 +753,6 @@ static int do_recover_data(struct f2fs_sb_info *sbi, struct inode *inode,
>>>>> f2fs_err(sbi, "Inconsistent dest blkaddr:%u, ino:%lu, ofs:%u",
>>>>> dest, inode->i_ino, dn.ofs_in_node);
>>>>> err = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi,
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto err;
>>>>> }
>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>> index 7901ede..ad6511f 100644
>>>>> --- a/fs/f2fs/segment.c
>>>>> +++ b/fs/f2fs/segment.c
>>>>> @@ -334,8 +334,6 @@ static int __f2fs_commit_atomic_write(struct inode *inode)
>>>>> DATA_GENERIC_ENHANCE)) {
>>>>> f2fs_put_dnode(&dn);
>>>>> ret = -EFSCORRUPTED;
>>>>> - f2fs_handle_error(sbi,
>>>>> - ERROR_INVALID_BLKADDR);
>>>>> goto out;
>>>>> }
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [f2fs-dev] [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-19 14:36 ` Chao Yu
@ 2024-02-20 2:36 ` Chao Yu
2024-02-20 5:34 ` Zhiguo Niu
0 siblings, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-20 2:36 UTC (permalink / raw)
To: Jaegeuk Kim, Zhiguo Niu
Cc: ke.wang, linux-kernel, linux-f2fs-devel, hongyu.jin, Zhiguo Niu
On 2024/2/19 22:36, Chao Yu wrote:
>>>> Please think about how to optimize this, which is really ugly now
---
fs/f2fs/checkpoint.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 87b7c988c8ca..089c26b80be3 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
}
static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
- int type)
+ int type, bool *record_error)
{
struct seg_entry *se;
unsigned int segno, offset;
@@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
blkaddr, exist);
set_sbi_flag(sbi, SBI_NEED_FSCK);
dump_stack();
+ *record_error = true;
}
return exist;
@@ -209,10 +210,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
dump_stack();
goto err;
} else {
- valid = __is_bitmap_valid(sbi, blkaddr, type);
- if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
- (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
+ bool record_error = false;
+
+ valid = __is_bitmap_valid(sbi, blkaddr, type,
+ &record_error);
+ if (!valid || record_error)
goto err;
+ return valid;
}
break;
case META_GENERIC:
--
2.40.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [f2fs-dev] [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-20 2:36 ` [f2fs-dev] " Chao Yu
@ 2024-02-20 5:34 ` Zhiguo Niu
2024-02-20 6:13 ` Chao Yu
2024-02-20 6:32 ` Chao Yu
0 siblings, 2 replies; 13+ messages in thread
From: Zhiguo Niu @ 2024-02-20 5:34 UTC (permalink / raw)
To: Chao Yu
Cc: Jaegeuk Kim, ke.wang, linux-kernel, linux-f2fs-devel, hongyu.jin,
Zhiguo Niu
On Tue, Feb 20, 2024 at 10:36 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/2/19 22:36, Chao Yu wrote:
> >>>> Please think about how to optimize this, which is really ugly now
> ---
> fs/f2fs/checkpoint.c | 12 ++++++++----
> 1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 87b7c988c8ca..089c26b80be3 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
> }
>
> static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> - int type)
> + int type, bool *record_error)
> {
> struct seg_entry *se;
> unsigned int segno, offset;
> @@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> blkaddr, exist);
> set_sbi_flag(sbi, SBI_NEED_FSCK);
> dump_stack();
> + *record_error = true;
> }
>
> return exist;
> @@ -209,10 +210,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> dump_stack();
> goto err;
> } else {
> - valid = __is_bitmap_valid(sbi, blkaddr, type);
> - if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> - (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> + bool record_error = false;
> +
> + valid = __is_bitmap_valid(sbi, blkaddr, type,
> + &record_error);
> + if (!valid || record_error)
if type == DATA_GENERIC_ENHANCE_UPDATE && bitmap check invalid, it
is a OK case, but !valid
will goto do error handling.
I think do f2fs_handle_error in __is_bitmap_valid is a good way.
> goto err;
> + return valid;
> }
> break;
> case META_GENERIC:
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [f2fs-dev] [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-20 5:34 ` Zhiguo Niu
@ 2024-02-20 6:13 ` Chao Yu
2024-02-20 6:32 ` Chao Yu
1 sibling, 0 replies; 13+ messages in thread
From: Chao Yu @ 2024-02-20 6:13 UTC (permalink / raw)
To: Zhiguo Niu
Cc: Jaegeuk Kim, ke.wang, linux-kernel, linux-f2fs-devel, hongyu.jin,
Zhiguo Niu
On 2024/2/20 13:34, Zhiguo Niu wrote:
> On Tue, Feb 20, 2024 at 10:36 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2024/2/19 22:36, Chao Yu wrote:
>>>>>> Please think about how to optimize this, which is really ugly now
>> ---
>> fs/f2fs/checkpoint.c | 12 ++++++++----
>> 1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 87b7c988c8ca..089c26b80be3 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -135,7 +135,7 @@ struct page *f2fs_get_tmp_page(struct f2fs_sb_info *sbi, pgoff_t index)
>> }
>>
>> static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>> - int type)
>> + int type, bool *record_error)
>> {
>> struct seg_entry *se;
>> unsigned int segno, offset;
>> @@ -160,6 +160,7 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
>> blkaddr, exist);
>> set_sbi_flag(sbi, SBI_NEED_FSCK);
>> dump_stack();
>> + *record_error = true;
>> }
>>
>> return exist;
>> @@ -209,10 +210,13 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
>> dump_stack();
>> goto err;
>> } else {
>> - valid = __is_bitmap_valid(sbi, blkaddr, type);
>> - if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
>> - (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
>> + bool record_error = false;
>> +
>> + valid = __is_bitmap_valid(sbi, blkaddr, type,
>> + &record_error);
>> + if (!valid || record_error)
> if type == DATA_GENERIC_ENHANCE_UPDATE && bitmap check invalid, it
> is a OK case, but !valid
> will goto do error handling.
Yes, it looks we need to remove !valid condition to avoid error handling,
and assign record_error correctly inside __is_bitmap_valid() for all the
cases.
> I think do f2fs_handle_error in __is_bitmap_valid is a good way.
Let me revise the diff patch for comments.
Thanks,
>
>> goto err;
>> + return valid;
>> }
>> break;
>> case META_GENERIC:
>> --
>> 2.40.1
>>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [f2fs-dev] [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
2024-02-20 5:34 ` Zhiguo Niu
2024-02-20 6:13 ` Chao Yu
@ 2024-02-20 6:32 ` Chao Yu
[not found] ` <CAHJ8P3J8WUAxOrG91id+sKKt7ndLnETdbzz6Zy7haoC9qfT0tg@mail.gmail.com>
1 sibling, 1 reply; 13+ messages in thread
From: Chao Yu @ 2024-02-20 6:32 UTC (permalink / raw)
To: Zhiguo Niu, Jaegeuk Kim
Cc: ke.wang, linux-kernel, linux-f2fs-devel, hongyu.jin, Zhiguo Niu
On 2024/2/20 13:34, Zhiguo Niu wrote:
> I think do f2fs_handle_error in __is_bitmap_valid is a good way.
Like this?
---
fs/f2fs/checkpoint.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 87b7c988c8ca..b8a7e83eb4e0 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -155,21 +155,24 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
return exist;
if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
- (!exist && type == DATA_GENERIC_ENHANCE)) {
- f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
- blkaddr, exist);
- set_sbi_flag(sbi, SBI_NEED_FSCK);
- dump_stack();
- }
-
+ (!exist && type == DATA_GENERIC_ENHANCE))
+ goto out_err;
+ if (!exist && type != DATA_GENERIC_ENHANCE_UPDATE)
+ goto out_handle;
+ return exist;
+out_err:
+ f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
+ blkaddr, exist);
+ set_sbi_flag(sbi, SBI_NEED_FSCK);
+ dump_stack();
+out_handle:
+ f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
return exist;
}
static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
block_t blkaddr, int type)
{
- bool valid = false;
-
switch (type) {
case META_NAT:
break;
@@ -209,10 +212,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
dump_stack();
goto err;
} else {
- valid = __is_bitmap_valid(sbi, blkaddr, type);
- if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
- (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
- goto err;
+ return __is_bitmap_valid(sbi, blkaddr, type);
}
break;
case META_GENERIC:
@@ -227,7 +227,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
return true;
err:
f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
- return valid;
+ return false;
}
bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
--
2.40.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [f2fs-dev] [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr
[not found] ` <CAHJ8P3J8WUAxOrG91id+sKKt7ndLnETdbzz6Zy7haoC9qfT0tg@mail.gmail.com>
@ 2024-02-20 7:58 ` Chao Yu
0 siblings, 0 replies; 13+ messages in thread
From: Chao Yu @ 2024-02-20 7:58 UTC (permalink / raw)
To: Zhiguo Niu
Cc: Jaegeuk Kim, ke.wang, linux-kernel, linux-f2fs-devel, hongyu.jin,
Zhiguo Niu
On 2024/2/20 15:45, Zhiguo Niu wrote:
>
>
> On Tue, Feb 20, 2024 at 2:32 PM Chao Yu <chao@kernel.org <mailto:chao@kernel.org>> wrote:
> >
> > On 2024/2/20 13:34, Zhiguo Niu wrote:
> > > I think do f2fs_handle_error in __is_bitmap_valid is a good way.
> >
> > Like this?
> >
> > ---
> > fs/f2fs/checkpoint.c | 28 ++++++++++++++--------------
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> > index 87b7c988c8ca..b8a7e83eb4e0 100644
> > --- a/fs/f2fs/checkpoint.c
> > +++ b/fs/f2fs/checkpoint.c
> > @@ -155,21 +155,24 @@ static bool __is_bitmap_valid(struct f2fs_sb_info *sbi, block_t blkaddr,
> > return exist;
> >
> > if ((exist && type == DATA_GENERIC_ENHANCE_UPDATE) ||
> > - (!exist && type == DATA_GENERIC_ENHANCE)) {
> > - f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> > - blkaddr, exist);
> > - set_sbi_flag(sbi, SBI_NEED_FSCK);
> > - dump_stack();
> > - }
> > -
> > + (!exist && type == DATA_GENERIC_ENHANCE))
> > + goto out_err;
> > + if (!exist && type != DATA_GENERIC_ENHANCE_UPDATE)
> > + goto out_handle;
> > + return exist;
> > +out_err:
> > + f2fs_err(sbi, "Inconsistent error blkaddr:%u, sit bitmap:%d",
> > + blkaddr, exist);
> > + set_sbi_flag(sbi, SBI_NEED_FSCK);
> > + dump_stack();
> > +out_handle:
> > + f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > return exist;
> > }
> >
> > static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > block_t blkaddr, int type)
> > {
> > - bool valid = false;
> > -
> > switch (type) {
> > case META_NAT:
> > break;
> > @@ -209,10 +212,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > dump_stack();
> > goto err;
> > } else {
> > - valid = __is_bitmap_valid(sbi, blkaddr, type);
> > - if ((!valid && type != DATA_GENERIC_ENHANCE_UPDATE) ||
> > - (valid && type == DATA_GENERIC_ENHANCE_UPDATE))
> > - goto err;
> > + return __is_bitmap_valid(sbi, blkaddr, type);
> > }
> > break;
> > case META_GENERIC:
> > @@ -227,7 +227,7 @@ static bool __f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > return true;
> > err:
> > f2fs_handle_error(sbi, ERROR_INVALID_BLKADDR);
> > - return valid;
> > + return false;
> I think it's OK.
> Do we need to wait for Jaegeuk Kim’s suggestion or should I update the new patch version first😀?
> thanks!
I guess we'd better to wait for Jaegeuk's comments.
Thanks,
> > }
> >
> > bool f2fs_is_valid_blkaddr(struct f2fs_sb_info *sbi,
> > --
> > 2.40.1
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2024-02-20 7:58 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-05 3:30 [PATCH v7] f2fs: unify the error handling of f2fs_is_valid_blkaddr Zhiguo Niu
2024-02-05 3:47 ` Chao Yu
2024-02-06 3:32 ` Jaegeuk Kim
2024-02-06 3:36 ` Chao Yu
2024-02-06 23:38 ` Zhiguo Niu
2024-02-19 3:46 ` Chao Yu
2024-02-19 11:19 ` Zhiguo Niu
2024-02-19 14:36 ` Chao Yu
2024-02-20 2:36 ` [f2fs-dev] " Chao Yu
2024-02-20 5:34 ` Zhiguo Niu
2024-02-20 6:13 ` Chao Yu
2024-02-20 6:32 ` Chao Yu
[not found] ` <CAHJ8P3J8WUAxOrG91id+sKKt7ndLnETdbzz6Zy7haoC9qfT0tg@mail.gmail.com>
2024-02-20 7:58 ` Chao Yu
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).