Re: [PATCH] f2fs: fix to check result of new_curseg in f2fs_allocate_segment_for_resize

[Chao Yu]

On 2024/3/4 15:52, Zhiguo Niu wrote:
> On Mon, Mar 4, 2024 at 3:17 PM Chao Yu <chao@xxxxxxxxxx> wrote:
> > On 2024/3/4 11:33, Zhiguo Niu wrote:
> > > On Mon, Mar 4, 2024 at 11:19 AM Chao Yu <chao@xxxxxxxxxx> wrote:
> > > > On 2024/3/1 19:36, Zhiguo Niu wrote:
> > > > > new_curseg may return error if get_new_segment fail, so its result
> > > > > should be check in its caller f2fs_allocate_segment_for_resize,
> > > > > alos pass this results to free_segment_range.
> > > >
> > > > Zhiguo,
> > > >
> > > > What about handling all error paths of new_curseg() and change_curseg()
> > > > in one patch?
> > > Dear Chao,
> > >
> > > Do you mean to merge it with the previous patch “f2fs: fix to check
> > > return value of f2fs_gc_range”?
> > > Because in addition to new_curseg/change_curseg error handling, there
> > > are some other changes in the previous patch.
> > > besides, I searched for new related codes, and there should be the
> > > only place left without error handling about new_curseg/
> > > change_curseg .
> >
> > Zhiguo, I meant something like this?
> >
> > Subject: [PATCH] f2fs: fix to handle error paths of {new,change}_curseg()
> Dear Chao,
> I got your meaning and I think this patch looks good.
> Please ignore my patch and use your version:）.

Zhiguo,

This is a raw patch, can you please write commit message for it, and
resend it once you've tested it.

Thanks,

> thanks!
> > ---
> >    fs/f2fs/f2fs.h    |  4 +--
> >    fs/f2fs/gc.c      |  7 +++--
> >    fs/f2fs/segment.c | 67 +++++++++++++++++++++++++++++++----------------
> >    fs/f2fs/super.c   |  4 ++-
> >    4 files changed, 54 insertions(+), 28 deletions(-)
> >
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 80789255bf68..03927f1b2ea1 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -3702,10 +3702,10 @@ int f2fs_disable_cp_again(struct f2fs_sb_info *sbi, block_t unusable);
> >    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 f8314765246a..854ad0a3f6ea 100644
> > --- a/fs/f2fs/gc.c
> > +++ b/fs/f2fs/gc.c
> > @@ -2033,8 +2033,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 22241aba6564..2bcf01fde143 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,24 @@ 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;
> >
> >          curseg->seg_type = target_type;
> >
> > @@ -2910,38 +2913,45 @@ 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;
> >
> >          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 +3079,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 +3095,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 +3111,8 @@ 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,
> > @@ -3107,6 +3120,7 @@ static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
> >    {
> >          struct curseg_info *curseg = CURSEG_I(sbi, type);
> >          unsigned int old_segno;
> > +       int ret;
> >
> >          if (type == CURSEG_COLD_DATA_PINNED && !curseg->inited)
> >                  goto allocate;
> > @@ -3119,8 +3133,9 @@ static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,
> >
> >    allocate:
> >          old_segno = curseg->segno;
> > -       if (new_curseg(sbi, type, true))
> > -               return -EAGAIN;
> > +       ret = new_curseg(sbi, type, true);
> > +       if (ret)
> > +               return ret;
> >          stat_inc_seg_type(sbi, curseg);
> >          locate_dirty_segment(sbi, old_segno);
> >          return 0;
> > @@ -3480,14 +3495,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);
> > @@ -3541,17 +3559,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;
> >          }
> >
> > @@ -3594,7 +3612,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;
> >
> >    }
> >
> > @@ -3824,7 +3842,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);
> > @@ -3850,12 +3869,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 0676c2dcbbf7..8c69257db8cc 100644
> > --- a/fs/f2fs/super.c
> > +++ b/fs/f2fs/super.c
> > @@ -4691,7 +4691,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);
> > --
> > 2.40.1
> >
> >
> >
> > > thanks!
> > > > Thanks,
> > > >
> > > > > Signed-off-by: Zhiguo Niu <zhiguo.niu@xxxxxxxxxx>
> > > > > ---
> > > > >     fs/f2fs/f2fs.h    | 2 +-
> > > > >     fs/f2fs/gc.c      | 7 +++++--
> > > > >     fs/f2fs/segment.c | 9 +++++++--
> > > > >     3 files changed, 13 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > > > > index 4331012..39dda7d 100644
> > > > > --- a/fs/f2fs/f2fs.h
> > > > > +++ b/fs/f2fs/f2fs.h
> > > > > @@ -3701,7 +3701,7 @@ void f2fs_clear_prefree_segments(struct f2fs_sb_info *sbi,
> > > > >     void 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 c60b747..7a458fa 100644
> > > > > --- a/fs/f2fs/gc.c
> > > > > +++ b/fs/f2fs/gc.c
> > > > > @@ -2037,8 +2037,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 1bb3019..2a07b9d 100644
> > > > > --- a/fs/f2fs/segment.c
> > > > > +++ b/fs/f2fs/segment.c
> > > > > @@ -3071,11 +3071,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 err = 0;
> > > > >
> > > > >         f2fs_down_read(&SM_I(sbi)->curseg_lock);
> > > > >         mutex_lock(&curseg->curseg_mutex);
> > > > > @@ -3089,7 +3090,10 @@ void f2fs_allocate_segment_for_resize(struct f2fs_sb_info *sbi, int type,
> > > > >                 change_curseg(sbi, type);
> > > > >         else
> > > > >                 new_curseg(sbi, type, true);
> > > > > -
> > > > > +     if (curseg->segno == NULL_SEGNO) {
> > > > > +             err = -ENOSPC;
> > > > > +             goto unlock;
> > > > > +     }
> > > > >         stat_inc_seg_type(sbi, curseg);
> > > > >
> > > > >         locate_dirty_segment(sbi, segno);
> > > > > @@ -3102,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 err;
> > > > >     }
> > > > >
> > > > >     static int __allocate_new_segment(struct f2fs_sb_info *sbi, int type,