* [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry
@ 2024-03-08 10:12 Zhiguo Niu
2024-03-08 10:12 ` [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg() Zhiguo Niu
2024-03-11 3:54 ` [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Chao Yu
0 siblings, 2 replies; 6+ messages in thread
From: Zhiguo Niu @ 2024-03-08 10:12 UTC (permalink / raw)
To: jaegeuk, chao
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, zhiguo.niu, ke.wang,
hongyu.jin
add_bio_entry should not trigger system panic when bio_add_page fail,
fix to remove it.
Fixes: 0b20fcec8651 ("f2fs: cache global IPU bio")
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
---
fs/f2fs/data.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
index d9494b5..f8ae684 100644
--- a/fs/f2fs/data.c
+++ b/fs/f2fs/data.c
@@ -759,8 +759,10 @@ static void add_bio_entry(struct f2fs_sb_info *sbi, struct bio *bio,
be->bio = bio;
bio_get(bio);
- if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
- f2fs_bug_on(sbi, 1);
+ if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
+ bio_put(bio);
+ return;
+ }
f2fs_down_write(&io->bio_list_lock);
list_add_tail(&be->list, &io->bio_list);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg()
2024-03-08 10:12 [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Zhiguo Niu
@ 2024-03-08 10:12 ` Zhiguo Niu
2024-03-11 6:09 ` Chao Yu
2024-03-11 3:54 ` [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Chao Yu
1 sibling, 1 reply; 6+ messages in thread
From: Zhiguo Niu @ 2024-03-08 10:12 UTC (permalink / raw)
To: jaegeuk, chao
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, zhiguo.niu, ke.wang,
hongyu.jin
{new,change}_curseg() may return error in some special cases,
error handling should be did in their callers, and this will also
facilitate subsequent error path expansion in {new,change}_curseg().
Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
Signed-off-by: Chao Yu <chao@kernel.org>
---
fs/f2fs/extent_cache.c | 2 +-
fs/f2fs/f2fs.h | 4 ++--
fs/f2fs/gc.c | 7 +++++--
fs/f2fs/segment.c | 57 +++++++++++++++++++++++++++++++-------------------
fs/f2fs/super.c | 4 +++-
5 files changed, 46 insertions(+), 28 deletions(-)
diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
index 48048fa..dce00cf 100644
--- a/fs/f2fs/extent_cache.c
+++ b/fs/f2fs/extent_cache.c
@@ -988,7 +988,7 @@ bool f2fs_lookup_read_extent_cache_block(struct inode *inode, pgoff_t index,
void f2fs_update_read_extent_cache(struct dnode_of_data *dn)
{
- return __update_extent_cache(dn, EX_READ);
+ __update_extent_cache(dn, EX_READ);
}
void f2fs_update_read_extent_cache_range(struct dnode_of_data *dn,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 4836e7c..7beb074 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3700,10 +3700,10 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
-void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
+int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
-void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
+int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
unsigned int start, unsigned int end);
int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, bool force);
int f2fs_allocate_pinning_section(struct f2fs_sb_info *sbi);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index ca1bf41..8852814 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -2035,8 +2035,11 @@ static int free_segment_range(struct f2fs_sb_info *sbi,
mutex_unlock(&DIRTY_I(sbi)->seglist_lock);
/* Move out cursegs from the target range */
- for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++)
- f2fs_allocate_segment_for_resize(sbi, type, start, end);
+ for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++) {
+ err = f2fs_allocate_segment_for_resize(sbi, type, start, end);
+ if (err)
+ goto out;
+ }
/* do GC to move out valid blocks in the range */
err = f2fs_gc_range(sbi, start, end, dry_run, 0);
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 4e4a51a..c1c1308 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2863,7 +2863,7 @@ bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno)
* This function always allocates a used segment(from dirty seglist) by SSR
* manner, so it should recover the existing segment information of valid blocks
*/
-static void change_curseg(struct f2fs_sb_info *sbi, int type)
+static int change_curseg(struct f2fs_sb_info *sbi, int type)
{
struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
struct curseg_info *curseg = CURSEG_I(sbi, type);
@@ -2888,21 +2888,23 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
if (IS_ERR(sum_page)) {
/* GC won't be able to use stale summary pages by cp_error */
memset(curseg->sum_blk, 0, SUM_ENTRY_SIZE);
- return;
+ return PTR_ERR(sum_page);
}
sum_node = (struct f2fs_summary_block *)page_address(sum_page);
memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
f2fs_put_page(sum_page, 1);
+ return 0;
}
static int get_ssr_segment(struct f2fs_sb_info *sbi, int type,
int alloc_mode, unsigned long long age);
-static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
+static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
int target_type, int alloc_mode,
unsigned long long age)
{
struct curseg_info *curseg = CURSEG_I(sbi, type);
+ int ret = 0;
curseg->seg_type = target_type;
@@ -2910,38 +2912,41 @@ static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
struct seg_entry *se = get_seg_entry(sbi, curseg->next_segno);
curseg->seg_type = se->type;
- change_curseg(sbi, type);
+ ret = change_curseg(sbi, type);
} else {
/* allocate cold segment by default */
curseg->seg_type = CURSEG_COLD_DATA;
- new_curseg(sbi, type, true);
+ ret = new_curseg(sbi, type, true);
}
stat_inc_seg_type(sbi, curseg);
+ return ret;
}
-static void __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
+static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
{
struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
+ int ret = 0;
if (!sbi->am.atgc_enabled)
- return;
+ return 0;
f2fs_down_read(&SM_I(sbi)->curseg_lock);
mutex_lock(&curseg->curseg_mutex);
down_write(&SIT_I(sbi)->sentry_lock);
- get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC, CURSEG_COLD_DATA, SSR, 0);
+ ret = get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC,
+ CURSEG_COLD_DATA, SSR, 0);
up_write(&SIT_I(sbi)->sentry_lock);
mutex_unlock(&curseg->curseg_mutex);
f2fs_up_read(&SM_I(sbi)->curseg_lock);
-
+ return ret;
}
-void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
+int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
{
- __f2fs_init_atgc_curseg(sbi);
+ return __f2fs_init_atgc_curseg(sbi);
}
static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
@@ -3069,11 +3074,12 @@ static bool need_new_seg(struct f2fs_sb_info *sbi, int type)
return false;
}
-void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
+int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
unsigned int start, unsigned int end)
{
struct curseg_info *curseg = CURSEG_I(sbi, type);
unsigned int segno;
+ int ret = 0;
f2fs_down_read(&SM_I(sbi)->curseg_lock);
mutex_lock(&curseg->curseg_mutex);
@@ -3084,9 +3090,9 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
goto unlock;
if (f2fs_need_SSR(sbi) && get_ssr_segment(sbi, type, SSR, 0))
- change_curseg(sbi, type);
+ ret = change_curseg(sbi, type);
else
- new_curseg(sbi, type, true);
+ ret = new_curseg(sbi, type, true);
stat_inc_seg_type(sbi, curseg);
@@ -3100,6 +3106,7 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
mutex_unlock(&curseg->curseg_mutex);
f2fs_up_read(&SM_I(sbi)->curseg_lock);
+ return ret;
}
static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
@@ -3486,14 +3493,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
bool from_gc = (type == CURSEG_ALL_DATA_ATGC);
struct seg_entry *se = NULL;
bool segment_full = false;
+ int ret = 0;
f2fs_down_read(&SM_I(sbi)->curseg_lock);
mutex_lock(&curseg->curseg_mutex);
down_write(&sit_i->sentry_lock);
- if (curseg->segno == NULL_SEGNO)
+ if (curseg->segno == NULL_SEGNO) {
+ ret = -ENOSPC;
goto out_err;
+ }
if (from_gc) {
f2fs_bug_on(sbi, GET_SEGNO(sbi, old_blkaddr) == NULL_SEGNO);
@@ -3546,17 +3556,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
}
if (from_gc) {
- get_atssr_segment(sbi, type, se->type,
+ ret = get_atssr_segment(sbi, type, se->type,
AT_SSR, se->mtime);
} else {
if (need_new_seg(sbi, type))
- new_curseg(sbi, type, false);
+ ret = new_curseg(sbi, type, false);
else
- change_curseg(sbi, type);
+ ret = change_curseg(sbi, type);
stat_inc_seg_type(sbi, curseg);
}
- if (curseg->segno == NULL_SEGNO)
+ if (ret)
goto out_err;
}
@@ -3599,7 +3609,7 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
up_write(&sit_i->sentry_lock);
mutex_unlock(&curseg->curseg_mutex);
f2fs_up_read(&SM_I(sbi)->curseg_lock);
- return -ENOSPC;
+ return ret;
}
@@ -3829,7 +3839,8 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
/* change the current segment */
if (segno != curseg->segno) {
curseg->next_segno = segno;
- change_curseg(sbi, type);
+ if (change_curseg(sbi, type))
+ goto out_unlock;
}
curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, new_blkaddr);
@@ -3855,12 +3866,14 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
if (recover_curseg) {
if (old_cursegno != curseg->segno) {
curseg->next_segno = old_cursegno;
- change_curseg(sbi, type);
+ if (change_curseg(sbi, type))
+ goto out_unlock;
}
curseg->next_blkoff = old_blkoff;
curseg->alloc_type = old_alloc_type;
}
+out_unlock:
up_write(&sit_i->sentry_lock);
mutex_unlock(&curseg->curseg_mutex);
f2fs_up_write(&SM_I(sbi)->curseg_lock);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 78a7658..f2b6d3f 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4685,7 +4685,9 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
if (err)
goto free_meta;
- f2fs_init_inmem_curseg(sbi);
+ err = f2fs_init_inmem_curseg(sbi);
+ if (err)
+ goto sync_free_meta;
/* f2fs_recover_fsync_data() cleared this already */
clear_sbi_flag(sbi, SBI_POR_DOING);
--
1.9.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry
2024-03-08 10:12 [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Zhiguo Niu
2024-03-08 10:12 ` [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg() Zhiguo Niu
@ 2024-03-11 3:54 ` Chao Yu
2024-03-11 7:26 ` Zhiguo Niu
1 sibling, 1 reply; 6+ messages in thread
From: Chao Yu @ 2024-03-11 3:54 UTC (permalink / raw)
To: Zhiguo Niu, jaegeuk
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang, hongyu.jin
On 2024/3/8 18:12, Zhiguo Niu wrote:
> add_bio_entry should not trigger system panic when bio_add_page fail,
> fix to remove it.
>
> Fixes: 0b20fcec8651 ("f2fs: cache global IPU bio")
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> ---
> fs/f2fs/data.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index d9494b5..f8ae684 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -759,8 +759,10 @@ static void add_bio_entry(struct f2fs_sb_info *sbi, struct bio *bio,
> be->bio = bio;
> bio_get(bio);
>
> - if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
> - f2fs_bug_on(sbi, 1);
> + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> + bio_put(bio);
I didn't get it, why new created bio has no space to store one page?
Thanks,
> + return;
> + }
>
> f2fs_down_write(&io->bio_list_lock);
> list_add_tail(&be->list, &io->bio_list);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg()
2024-03-08 10:12 ` [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg() Zhiguo Niu
@ 2024-03-11 6:09 ` Chao Yu
2024-03-11 7:19 ` Zhiguo Niu
0 siblings, 1 reply; 6+ messages in thread
From: Chao Yu @ 2024-03-11 6:09 UTC (permalink / raw)
To: Zhiguo Niu, jaegeuk
Cc: linux-f2fs-devel, linux-kernel, niuzhiguo84, ke.wang, hongyu.jin
On 2024/3/8 18:12, Zhiguo Niu wrote:
> {new,change}_curseg() may return error in some special cases,
> error handling should be did in their callers, and this will also
> facilitate subsequent error path expansion in {new,change}_curseg().
>
> Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/f2fs/extent_cache.c | 2 +-
> fs/f2fs/f2fs.h | 4 ++--
> fs/f2fs/gc.c | 7 +++++--
> fs/f2fs/segment.c | 57 +++++++++++++++++++++++++++++++-------------------
> fs/f2fs/super.c | 4 +++-
> 5 files changed, 46 insertions(+), 28 deletions(-)
>
> diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> index 48048fa..dce00cf 100644
> --- a/fs/f2fs/extent_cache.c
> +++ b/fs/f2fs/extent_cache.c
> @@ -988,7 +988,7 @@ bool f2fs_lookup_read_extent_cache_block(struct inode *inode, pgoff_t index,
>
> void f2fs_update_read_extent_cache(struct dnode_of_data *dn)
> {
> - return __update_extent_cache(dn, EX_READ);
> + __update_extent_cache(dn, EX_READ);
Above change is not related to this patch?
Otherwise, it looks good to me.
Thanks,
> }
>
> void f2fs_update_read_extent_cache_range(struct dnode_of_data *dn,
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 4836e7c..7beb074 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3700,10 +3700,10 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
> void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
> int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
> bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
> -void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> +int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
> void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
> -void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> +int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> unsigned int start, unsigned int end);
> int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, bool force);
> int f2fs_allocate_pinning_section(struct f2fs_sb_info *sbi);
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index ca1bf41..8852814 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -2035,8 +2035,11 @@ static int free_segment_range(struct f2fs_sb_info *sbi,
> mutex_unlock(&DIRTY_I(sbi)->seglist_lock);
>
> /* Move out cursegs from the target range */
> - for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++)
> - f2fs_allocate_segment_for_resize(sbi, type, start, end);
> + for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++) {
> + err = f2fs_allocate_segment_for_resize(sbi, type, start, end);
> + if (err)
> + goto out;
> + }
>
> /* do GC to move out valid blocks in the range */
> err = f2fs_gc_range(sbi, start, end, dry_run, 0);
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 4e4a51a..c1c1308 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2863,7 +2863,7 @@ bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno)
> * This function always allocates a used segment(from dirty seglist) by SSR
> * manner, so it should recover the existing segment information of valid blocks
> */
> -static void change_curseg(struct f2fs_sb_info *sbi, int type)
> +static int change_curseg(struct f2fs_sb_info *sbi, int type)
> {
> struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> struct curseg_info *curseg = CURSEG_I(sbi, type);
> @@ -2888,21 +2888,23 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> if (IS_ERR(sum_page)) {
> /* GC won't be able to use stale summary pages by cp_error */
> memset(curseg->sum_blk, 0, SUM_ENTRY_SIZE);
> - return;
> + return PTR_ERR(sum_page);
> }
> sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> f2fs_put_page(sum_page, 1);
> + return 0;
> }
>
> static int get_ssr_segment(struct f2fs_sb_info *sbi, int type,
> int alloc_mode, unsigned long long age);
>
> -static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> +static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> int target_type, int alloc_mode,
> unsigned long long age)
> {
> struct curseg_info *curseg = CURSEG_I(sbi, type);
> + int ret = 0;
>
> curseg->seg_type = target_type;
>
> @@ -2910,38 +2912,41 @@ static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> struct seg_entry *se = get_seg_entry(sbi, curseg->next_segno);
>
> curseg->seg_type = se->type;
> - change_curseg(sbi, type);
> + ret = change_curseg(sbi, type);
> } else {
> /* allocate cold segment by default */
> curseg->seg_type = CURSEG_COLD_DATA;
> - new_curseg(sbi, type, true);
> + ret = new_curseg(sbi, type, true);
> }
> stat_inc_seg_type(sbi, curseg);
> + return ret;
> }
>
> -static void __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> +static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> {
> struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
> + int ret = 0;
>
> if (!sbi->am.atgc_enabled)
> - return;
> + return 0;
>
> f2fs_down_read(&SM_I(sbi)->curseg_lock);
>
> mutex_lock(&curseg->curseg_mutex);
> down_write(&SIT_I(sbi)->sentry_lock);
>
> - get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC, CURSEG_COLD_DATA, SSR, 0);
> + ret = get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC,
> + CURSEG_COLD_DATA, SSR, 0);
>
> up_write(&SIT_I(sbi)->sentry_lock);
> mutex_unlock(&curseg->curseg_mutex);
>
> f2fs_up_read(&SM_I(sbi)->curseg_lock);
> -
> + return ret;
> }
> -void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> +int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> {
> - __f2fs_init_atgc_curseg(sbi);
> + return __f2fs_init_atgc_curseg(sbi);
> }
>
> static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> @@ -3069,11 +3074,12 @@ static bool need_new_seg(struct f2fs_sb_info *sbi, int type)
> return false;
> }
>
> -void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> +int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> unsigned int start, unsigned int end)
> {
> struct curseg_info *curseg = CURSEG_I(sbi, type);
> unsigned int segno;
> + int ret = 0;
>
> f2fs_down_read(&SM_I(sbi)->curseg_lock);
> mutex_lock(&curseg->curseg_mutex);
> @@ -3084,9 +3090,9 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> goto unlock;
>
> if (f2fs_need_SSR(sbi) && get_ssr_segment(sbi, type, SSR, 0))
> - change_curseg(sbi, type);
> + ret = change_curseg(sbi, type);
> else
> - new_curseg(sbi, type, true);
> + ret = new_curseg(sbi, type, true);
>
> stat_inc_seg_type(sbi, curseg);
>
> @@ -3100,6 +3106,7 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
>
> mutex_unlock(&curseg->curseg_mutex);
> f2fs_up_read(&SM_I(sbi)->curseg_lock);
> + return ret;
> }
>
> static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
> @@ -3486,14 +3493,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> bool from_gc = (type == CURSEG_ALL_DATA_ATGC);
> struct seg_entry *se = NULL;
> bool segment_full = false;
> + int ret = 0;
>
> f2fs_down_read(&SM_I(sbi)->curseg_lock);
>
> mutex_lock(&curseg->curseg_mutex);
> down_write(&sit_i->sentry_lock);
>
> - if (curseg->segno == NULL_SEGNO)
> + if (curseg->segno == NULL_SEGNO) {
> + ret = -ENOSPC;
> goto out_err;
> + }
>
> if (from_gc) {
> f2fs_bug_on(sbi, GET_SEGNO(sbi, old_blkaddr) == NULL_SEGNO);
> @@ -3546,17 +3556,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> }
>
> if (from_gc) {
> - get_atssr_segment(sbi, type, se->type,
> + ret = get_atssr_segment(sbi, type, se->type,
> AT_SSR, se->mtime);
> } else {
> if (need_new_seg(sbi, type))
> - new_curseg(sbi, type, false);
> + ret = new_curseg(sbi, type, false);
> else
> - change_curseg(sbi, type);
> + ret = change_curseg(sbi, type);
> stat_inc_seg_type(sbi, curseg);
> }
>
> - if (curseg->segno == NULL_SEGNO)
> + if (ret)
> goto out_err;
> }
>
> @@ -3599,7 +3609,7 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> up_write(&sit_i->sentry_lock);
> mutex_unlock(&curseg->curseg_mutex);
> f2fs_up_read(&SM_I(sbi)->curseg_lock);
> - return -ENOSPC;
> + return ret;
>
> }
>
> @@ -3829,7 +3839,8 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> /* change the current segment */
> if (segno != curseg->segno) {
> curseg->next_segno = segno;
> - change_curseg(sbi, type);
> + if (change_curseg(sbi, type))
> + goto out_unlock;
> }
>
> curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, new_blkaddr);
> @@ -3855,12 +3866,14 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> if (recover_curseg) {
> if (old_cursegno != curseg->segno) {
> curseg->next_segno = old_cursegno;
> - change_curseg(sbi, type);
> + if (change_curseg(sbi, type))
> + goto out_unlock;
> }
> curseg->next_blkoff = old_blkoff;
> curseg->alloc_type = old_alloc_type;
> }
>
> +out_unlock:
> up_write(&sit_i->sentry_lock);
> mutex_unlock(&curseg->curseg_mutex);
> f2fs_up_write(&SM_I(sbi)->curseg_lock);
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 78a7658..f2b6d3f 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4685,7 +4685,9 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> if (err)
> goto free_meta;
>
> - f2fs_init_inmem_curseg(sbi);
> + err = f2fs_init_inmem_curseg(sbi);
> + if (err)
> + goto sync_free_meta;
>
> /* f2fs_recover_fsync_data() cleared this already */
> clear_sbi_flag(sbi, SBI_POR_DOING);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg()
2024-03-11 6:09 ` Chao Yu
@ 2024-03-11 7:19 ` Zhiguo Niu
0 siblings, 0 replies; 6+ messages in thread
From: Zhiguo Niu @ 2024-03-11 7:19 UTC (permalink / raw)
To: Chao Yu
Cc: Zhiguo Niu, jaegeuk, linux-f2fs-devel, linux-kernel, ke.wang, hongyu.jin
On Mon, Mar 11, 2024 at 2:09 PM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/3/8 18:12, Zhiguo Niu wrote:
> > {new,change}_curseg() may return error in some special cases,
> > error handling should be did in their callers, and this will also
> > facilitate subsequent error path expansion in {new,change}_curseg().
> >
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> > Signed-off-by: Chao Yu <chao@kernel.org>
> > ---
> > fs/f2fs/extent_cache.c | 2 +-
> > fs/f2fs/f2fs.h | 4 ++--
> > fs/f2fs/gc.c | 7 +++++--
> > fs/f2fs/segment.c | 57 +++++++++++++++++++++++++++++++-------------------
> > fs/f2fs/super.c | 4 +++-
> > 5 files changed, 46 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/f2fs/extent_cache.c b/fs/f2fs/extent_cache.c
> > index 48048fa..dce00cf 100644
> > --- a/fs/f2fs/extent_cache.c
> > +++ b/fs/f2fs/extent_cache.c
> > @@ -988,7 +988,7 @@ bool f2fs_lookup_read_extent_cache_block(struct inode *inode, pgoff_t index,
> >
> > void f2fs_update_read_extent_cache(struct dnode_of_data *dn)
> > {
> > - return __update_extent_cache(dn, EX_READ);
> > + __update_extent_cache(dn, EX_READ);
>
> Above change is not related to this patch?
>
> Otherwise, it looks good to me.
>
> Thanks,
Dear Chao,
Okay, I see that both functions here are void type, so there is no
need to use return^^.
I will remove this part and update.
By the way, I did a stability test on this patch and the result passed
thanks!
>
> > }
> >
> > void f2fs_update_read_extent_cache_range(struct dnode_of_data *dn,
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 4836e7c..7beb074 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3700,10 +3700,10 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
> > void f2fs_release_discard_addrs(struct f2fs_sb_info *sbi);
> > int f2fs_npages_for_summary_flush(struct f2fs_sb_info *sbi, bool for_ra);
> > bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno);
> > -void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> > +int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi);
> > void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi);
> > void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi);
> > -void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > +int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > unsigned int start, unsigned int end);
> > int f2fs_allocate_new_section(struct f2fs_sb_info *sbi, int type, bool force);
> > int f2fs_allocate_pinning_section(struct f2fs_sb_info *sbi);
> > diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> > index ca1bf41..8852814 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -2035,8 +2035,11 @@ static int free_segment_range(struct f2fs_sb_info *sbi,
> > mutex_unlock(&DIRTY_I(sbi)->seglist_lock);
> >
> > /* Move out cursegs from the target range */
> > - for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++)
> > - f2fs_allocate_segment_for_resize(sbi, type, start, end);
> > + for (type = CURSEG_HOT_DATA; type < NR_CURSEG_PERSIST_TYPE; type++) {
> > + err = f2fs_allocate_segment_for_resize(sbi, type, start, end);
> > + if (err)
> > + goto out;
> > + }
> >
> > /* do GC to move out valid blocks in the range */
> > err = f2fs_gc_range(sbi, start, end, dry_run, 0);
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 4e4a51a..c1c1308 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2863,7 +2863,7 @@ bool f2fs_segment_has_free_slot(struct f2fs_sb_info *sbi, int segno)
> > * This function always allocates a used segment(from dirty seglist) by SSR
> > * manner, so it should recover the existing segment information of valid blocks
> > */
> > -static void change_curseg(struct f2fs_sb_info *sbi, int type)
> > +static int change_curseg(struct f2fs_sb_info *sbi, int type)
> > {
> > struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
> > struct curseg_info *curseg = CURSEG_I(sbi, type);
> > @@ -2888,21 +2888,23 @@ static void change_curseg(struct f2fs_sb_info *sbi, int type)
> > if (IS_ERR(sum_page)) {
> > /* GC won't be able to use stale summary pages by cp_error */
> > memset(curseg->sum_blk, 0, SUM_ENTRY_SIZE);
> > - return;
> > + return PTR_ERR(sum_page);
> > }
> > sum_node = (struct f2fs_summary_block *)page_address(sum_page);
> > memcpy(curseg->sum_blk, sum_node, SUM_ENTRY_SIZE);
> > f2fs_put_page(sum_page, 1);
> > + return 0;
> > }
> >
> > static int get_ssr_segment(struct f2fs_sb_info *sbi, int type,
> > int alloc_mode, unsigned long long age);
> >
> > -static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> > +static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> > int target_type, int alloc_mode,
> > unsigned long long age)
> > {
> > struct curseg_info *curseg = CURSEG_I(sbi, type);
> > + int ret = 0;
> >
> > curseg->seg_type = target_type;
> >
> > @@ -2910,38 +2912,41 @@ static void get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> > struct seg_entry *se = get_seg_entry(sbi, curseg->next_segno);
> >
> > curseg->seg_type = se->type;
> > - change_curseg(sbi, type);
> > + ret = change_curseg(sbi, type);
> > } else {
> > /* allocate cold segment by default */
> > curseg->seg_type = CURSEG_COLD_DATA;
> > - new_curseg(sbi, type, true);
> > + ret = new_curseg(sbi, type, true);
> > }
> > stat_inc_seg_type(sbi, curseg);
> > + return ret;
> > }
> >
> > -static void __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > +static int __f2fs_init_atgc_curseg(struct f2fs_sb_info *sbi)
> > {
> > struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC);
> > + int ret = 0;
> >
> > if (!sbi->am.atgc_enabled)
> > - return;
> > + return 0;
> >
> > f2fs_down_read(&SM_I(sbi)->curseg_lock);
> >
> > mutex_lock(&curseg->curseg_mutex);
> > down_write(&SIT_I(sbi)->sentry_lock);
> >
> > - get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC, CURSEG_COLD_DATA, SSR, 0);
> > + ret = get_atssr_segment(sbi, CURSEG_ALL_DATA_ATGC,
> > + CURSEG_COLD_DATA, SSR, 0);
> >
> > up_write(&SIT_I(sbi)->sentry_lock);
> > mutex_unlock(&curseg->curseg_mutex);
> >
> > f2fs_up_read(&SM_I(sbi)->curseg_lock);
> > -
> > + return ret;
> > }
> > -void f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> > +int f2fs_init_inmem_curseg(struct f2fs_sb_info *sbi)
> > {
> > - __f2fs_init_atgc_curseg(sbi);
> > + return __f2fs_init_atgc_curseg(sbi);
> > }
> >
> > static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> > @@ -3069,11 +3074,12 @@ static bool need_new_seg(struct f2fs_sb_info *sbi, int type)
> > return false;
> > }
> >
> > -void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > +int f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > unsigned int start, unsigned int end)
> > {
> > struct curseg_info *curseg = CURSEG_I(sbi, type);
> > unsigned int segno;
> > + int ret = 0;
> >
> > f2fs_down_read(&SM_I(sbi)->curseg_lock);
> > mutex_lock(&curseg->curseg_mutex);
> > @@ -3084,9 +3090,9 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > goto unlock;
> >
> > if (f2fs_need_SSR(sbi) && get_ssr_segment(sbi, type, SSR, 0))
> > - change_curseg(sbi, type);
> > + ret = change_curseg(sbi, type);
> > else
> > - new_curseg(sbi, type, true);
> > + ret = new_curseg(sbi, type, true);
> >
> > stat_inc_seg_type(sbi, curseg);
> >
> > @@ -3100,6 +3106,7 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> >
> > mutex_unlock(&curseg->curseg_mutex);
> > f2fs_up_read(&SM_I(sbi)->curseg_lock);
> > + return ret;
> > }
> >
> > static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
> > @@ -3486,14 +3493,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> > bool from_gc = (type == CURSEG_ALL_DATA_ATGC);
> > struct seg_entry *se = NULL;
> > bool segment_full = false;
> > + int ret = 0;
> >
> > f2fs_down_read(&SM_I(sbi)->curseg_lock);
> >
> > mutex_lock(&curseg->curseg_mutex);
> > down_write(&sit_i->sentry_lock);
> >
> > - if (curseg->segno == NULL_SEGNO)
> > + if (curseg->segno == NULL_SEGNO) {
> > + ret = -ENOSPC;
> > goto out_err;
> > + }
> >
> > if (from_gc) {
> > f2fs_bug_on(sbi, GET_SEGNO(sbi, old_blkaddr) == NULL_SEGNO);
> > @@ -3546,17 +3556,17 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> > }
> >
> > if (from_gc) {
> > - get_atssr_segment(sbi, type, se->type,
> > + ret = get_atssr_segment(sbi, type, se->type,
> > AT_SSR, se->mtime);
> > } else {
> > if (need_new_seg(sbi, type))
> > - new_curseg(sbi, type, false);
> > + ret = new_curseg(sbi, type, false);
> > else
> > - change_curseg(sbi, type);
> > + ret = change_curseg(sbi, type);
> > stat_inc_seg_type(sbi, curseg);
> > }
> >
> > - if (curseg->segno == NULL_SEGNO)
> > + if (ret)
> > goto out_err;
> > }
> >
> > @@ -3599,7 +3609,7 @@ int f2fs_allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> > up_write(&sit_i->sentry_lock);
> > mutex_unlock(&curseg->curseg_mutex);
> > f2fs_up_read(&SM_I(sbi)->curseg_lock);
> > - return -ENOSPC;
> > + return ret;
> >
> > }
> >
> > @@ -3829,7 +3839,8 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > /* change the current segment */
> > if (segno != curseg->segno) {
> > curseg->next_segno = segno;
> > - change_curseg(sbi, type);
> > + if (change_curseg(sbi, type))
> > + goto out_unlock;
> > }
> >
> > curseg->next_blkoff = GET_BLKOFF_FROM_SEG0(sbi, new_blkaddr);
> > @@ -3855,12 +3866,14 @@ void f2fs_do_replace_block(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
> > if (recover_curseg) {
> > if (old_cursegno != curseg->segno) {
> > curseg->next_segno = old_cursegno;
> > - change_curseg(sbi, type);
> > + if (change_curseg(sbi, type))
> > + goto out_unlock;
> > }
> > curseg->next_blkoff = old_blkoff;
> > curseg->alloc_type = old_alloc_type;
> > }
> >
> > +out_unlock:
> > up_write(&sit_i->sentry_lock);
> > mutex_unlock(&curseg->curseg_mutex);
> > f2fs_up_write(&SM_I(sbi)->curseg_lock);
> > diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> > index 78a7658..f2b6d3f 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -4685,7 +4685,9 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
> > if (err)
> > goto free_meta;
> >
> > - f2fs_init_inmem_curseg(sbi);
> > + err = f2fs_init_inmem_curseg(sbi);
> > + if (err)
> > + goto sync_free_meta;
> >
> > /* f2fs_recover_fsync_data() cleared this already */
> > clear_sbi_flag(sbi, SBI_POR_DOING);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry
2024-03-11 3:54 ` [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Chao Yu
@ 2024-03-11 7:26 ` Zhiguo Niu
0 siblings, 0 replies; 6+ messages in thread
From: Zhiguo Niu @ 2024-03-11 7:26 UTC (permalink / raw)
To: Chao Yu
Cc: Zhiguo Niu, jaegeuk, linux-f2fs-devel, linux-kernel, ke.wang, hongyu.jin
On Mon, Mar 11, 2024 at 11:54 AM Chao Yu <chao@kernel.org> wrote:
>
> On 2024/3/8 18:12, Zhiguo Niu wrote:
> > add_bio_entry should not trigger system panic when bio_add_page fail,
> > fix to remove it.
> >
> > Fixes: 0b20fcec8651 ("f2fs: cache global IPU bio")
> > Signed-off-by: Zhiguo Niu <zhiguo.niu@unisoc.com>
> > ---
> > fs/f2fs/data.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> > index d9494b5..f8ae684 100644
> > --- a/fs/f2fs/data.c
> > +++ b/fs/f2fs/data.c
> > @@ -759,8 +759,10 @@ static void add_bio_entry(struct f2fs_sb_info *sbi, struct bio *bio,
> > be->bio = bio;
> > bio_get(bio);
> >
> > - if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE)
> > - f2fs_bug_on(sbi, 1);
> > + if (bio_add_page(bio, page, PAGE_SIZE, 0) != PAGE_SIZE) {
> > + bio_put(bio);
>
> I didn't get it, why new created bio has no space to store one page?
>
> Thanks,
Dear Chao,
I got what you mean.
We are doing bio merge optimization in bio_add_page.
After looking at all the locations where bio_add_page is called,
and think it is unreasonable to panic the system if bio_add_page fails.
but it is not impossible to panic in the current flow about bio_add_page.
so keeping it as is is a good choice.
thanks!
> > + return;
> > + }
> >
> > f2fs_down_write(&io->bio_list_lock);
> > list_add_tail(&be->list, &io->bio_list);
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-11 7:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-08 10:12 [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Zhiguo Niu
2024-03-08 10:12 ` [PATCH 2/2] f2fs: fix to handle error paths of {new,change}_curseg() Zhiguo Niu
2024-03-11 6:09 ` Chao Yu
2024-03-11 7:19 ` Zhiguo Niu
2024-03-11 3:54 ` [PATCH 1/2] f2fs: fix to remove f2fs_bug_on in add_bio_entry Chao Yu
2024-03-11 7:26 ` Zhiguo Niu
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).