Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to handle error paths of {new, change}_curseg()
On Mon, Mar 11, 2024 at 2:09 PM Chao Yu 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
> > Signed-off-by: Chao Yu
> > ---
> > 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(_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,
> >
Re: [f2fs-dev] [PATCH 2/2] f2fs: fix to handle error paths of {new, change}_curseg()
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
Signed-off-by: Chao Yu
---
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(_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 */
[f2fs-dev] [PATCH 2/2] f2fs: fix to handle error paths of {new, change}_curseg()
{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
Signed-off-by: Chao Yu
---
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(_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,
