Re: [f2fs-dev] [PATCH] f2fs: allocate consective blkaddrs for defragment

[Jaegeuk Kim <jaegeuk@kernel.org>]

On 08/14, guo weichao wrote:
> Hi Jaegeuk,
> 
> IMO, defragment is important for many user scenarios. If it's still fragment after defrag, users may trigger more defrag ioctls, which hurts performance & device lifetime. So how about add an idle check & retry one more time besides adding "wb_sync_req[DATA]" count, draft liked the following:
> 
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> +#define DEFRAG_RETRY_COUNT 2
> +#define DEFRAG_WAIT_SEC 10
> 
> @@ -2508,7 +2512,7 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
>                                                 range->start + range->len - 1);
>         if (err)
>                 goto out;
> +frag_check:
>         /*
>          * lookup mapping info in extent cache, skip defragmenting if physical
>          * block addresses are continuous.
> @@ -2611,9 +2615,16 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
> 
>                 clear_inode_flag(inode, FI_SKIP_WRITES);
> 
> +               wait_secs = 0;
> +               while (!is_idle(sbi, REQ_TIME) && wait_secs++ < DEFRAG_WAIT_SEC)
> +                       msleep(1000);
> +               atomic_inc(&sbi->wb_sync_req[DATA]);
>                 err = filemap_fdatawrite(inode->i_mapping);
> +               atomic_dec(&sbi->wb_sync_req[DATA]);
>                 if (err)
>                         goto out;
> +               else if (retries++ < DEFRAG_RETRY_COUNT)
> +                       goto frag_check;

Doesn't it need to run some GCs to get a consecutive free sections?
What is the goal here? How can we quantify the fragmented level?

> ________________________________
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> To: Weichao Guo <guoweichao@oppo.com>
> CC: zhangshiming@oppo.com <zhangshiming@oppo.com>; linux-f2fs-devel@lists.sourceforge.net <linux-f2fs-devel@lists.sourceforge.net>
> Title: Re: [f2fs-dev] [PATCH] f2fs: allocate consective blkaddrs for defragment
> 
> Hi Weichao,
> 
> On 08/12, Weichao Guo wrote:
> > When we try to defrag a file, its data blocks may mess with others if there
> > are lots of concurrent writes. This causes the file is still fragmented
> > after defrag. So It's better to isolate defrag writes from others.
> 
> I really don't want to add more log like this. What about using
> wb_sync_req[DATA] to prevent async writes at least?
> 
> >
> > Signed-off-by: Weichao Guo <guoweichao@oppo.com>
> > Signed-off-by: Chao Yu <chao@kernel.org>
> > ---
> >  fs/f2fs/debug.c   | 4 ++++
> >  fs/f2fs/f2fs.h    | 2 ++
> >  fs/f2fs/file.c    | 5 +++++
> >  fs/f2fs/segment.c | 7 +++++++
> >  fs/f2fs/segment.h | 5 ++++-
> >  5 files changed, 22 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/f2fs/debug.c b/fs/f2fs/debug.c
> > index c014715..d85dc17 100644
> > --- a/fs/f2fs/debug.c
> > +++ b/fs/f2fs/debug.c
> > @@ -442,6 +442,10 @@ static int stat_show(struct seq_file *s, void *v)
> >                           si->curseg[CURSEG_ALL_DATA_ATGC],
> >                           si->cursec[CURSEG_ALL_DATA_ATGC],
> >                           si->curzone[CURSEG_ALL_DATA_ATGC]);
> > +             seq_printf(s, "  - DEFRAG   data: %8d %8d %8d\n",
> > +                        si->curseg[CURSEG_COLD_DATA_DEFRAG],
> > +                        si->cursec[CURSEG_COLD_DATA_DEFRAG],
> > +                        si->curzone[CURSEG_COLD_DATA_DEFRAG]);
> >                seq_printf(s, "\n  - Valid: %d\n  - Dirty: %d\n",
> >                           si->main_area_segs - si->dirty_count -
> >                           si->prefree_count - si->free_segs,
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 3c7cdb7..5f9a185 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -760,6 +760,7 @@ enum {
> >        FI_COMPRESS_RELEASED,   /* compressed blocks were released */
> >        FI_ALIGNED_WRITE,       /* enable aligned write */
> >        FI_COW_FILE,            /* indicate COW file */
> > +     FI_DEFRAG_WRITE,        /* indicate defragment writes need consective blkaddrs*/
> >        FI_MAX,                 /* max flag, never be used */
> >  };
> >
> > @@ -1017,6 +1018,7 @@ enum {
> >        CURSEG_COLD_DATA_PINNED = NR_PERSISTENT_LOG,
> >                                /* pinned file that needs consecutive block address */
> >        CURSEG_ALL_DATA_ATGC,   /* SSR alloctor in hot/warm/cold data area */
> > +     CURSEG_COLD_DATA_DEFRAG,/* used for defragment which needs consective blkaddrs */
> >        NO_CHECK_TYPE,          /* number of persistent & inmem log */
> >  };
> >
> > diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> > index ce4905a0..f104e2e 100644
> > --- a/fs/f2fs/file.c
> > +++ b/fs/f2fs/file.c
> > @@ -2626,7 +2626,12 @@ static int f2fs_defragment_range(struct f2fs_sb_info *sbi,
> >
> >                clear_inode_flag(inode, FI_SKIP_WRITES);
> >
> > +             set_inode_flag(inode, FI_DEFRAG_WRITE);
> > +             f2fs_lock_op(sbi);
> > +             f2fs_allocate_new_section(sbi, CURSEG_COLD_DATA_DEFRAG, false);
> > +             f2fs_unlock_op(sbi);
> >                err = filemap_fdatawrite(inode->i_mapping);
> > +             clear_inode_flag(inode, FI_DEFRAG_WRITE);
> >                if (err)
> >                        goto out;
> >        }
> > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> > index 0de21f8..17e7d14 100644
> > --- a/fs/f2fs/segment.c
> > +++ b/fs/f2fs/segment.c
> > @@ -2749,6 +2749,7 @@ static void __f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> >  void f2fs_save_inmem_curseg(struct f2fs_sb_info *sbi)
> >  {
> >        __f2fs_save_inmem_curseg(sbi, CURSEG_COLD_DATA_PINNED);
> > +     __f2fs_save_inmem_curseg(sbi, CURSEG_COLD_DATA_DEFRAG);
> >
> >        if (sbi->am.atgc_enabled)
> >                __f2fs_save_inmem_curseg(sbi, CURSEG_ALL_DATA_ATGC);
> > @@ -2774,6 +2775,7 @@ static void __f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi, int type)
> >  void f2fs_restore_inmem_curseg(struct f2fs_sb_info *sbi)
> >  {
> >        __f2fs_restore_inmem_curseg(sbi, CURSEG_COLD_DATA_PINNED);
> > +     __f2fs_restore_inmem_curseg(sbi, CURSEG_COLD_DATA_DEFRAG);
> >
> >        if (sbi->am.atgc_enabled)
> >                __f2fs_restore_inmem_curseg(sbi, CURSEG_ALL_DATA_ATGC);
> > @@ -3161,6 +3163,9 @@ static int __get_segment_type_6(struct f2fs_io_info *fio)
> >                if (is_inode_flag_set(inode, FI_ALIGNED_WRITE))
> >                        return CURSEG_COLD_DATA_PINNED;
> >
> > +             if (is_inode_flag_set(inode, FI_DEFRAG_WRITE))
> > +                     return CURSEG_COLD_DATA_DEFRAG;
> > +
> >                if (page_private_gcing(fio->page)) {
> >                        if (fio->sbi->am.atgc_enabled &&
> >                                (fio->io_type == FS_DATA_IO) &&
> > @@ -4332,6 +4337,8 @@ static int build_curseg(struct f2fs_sb_info *sbi)
> >                        array[i].seg_type = CURSEG_COLD_DATA;
> >                else if (i == CURSEG_ALL_DATA_ATGC)
> >                        array[i].seg_type = CURSEG_COLD_DATA;
> > +             else if (i == CURSEG_COLD_DATA_DEFRAG)
> > +                     array[i].seg_type = CURSEG_COLD_DATA;
> >                array[i].segno = NULL_SEGNO;
> >                array[i].next_blkoff = 0;
> >                array[i].inited = false;
> > diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> > index d1d6376..75e2aa8 100644
> > --- a/fs/f2fs/segment.h
> > +++ b/fs/f2fs/segment.h
> > @@ -44,7 +44,8 @@ static inline void sanity_check_seg_type(struct f2fs_sb_info *sbi,
> >         ((seg) == CURSEG_I(sbi, CURSEG_WARM_NODE)->segno) ||    \
> >         ((seg) == CURSEG_I(sbi, CURSEG_COLD_NODE)->segno) ||    \
> >         ((seg) == CURSEG_I(sbi, CURSEG_COLD_DATA_PINNED)->segno) ||     \
> > -      ((seg) == CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC)->segno))
> > +      ((seg) == CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC)->segno) ||        \
> > +      ((seg) == CURSEG_I(sbi, CURSEG_COLD_DATA_DEFRAG)->segno))
> >
> >  #define IS_CURSEC(sbi, secno)                                                \
> >        (((secno) == CURSEG_I(sbi, CURSEG_HOT_DATA)->segno /            \
> > @@ -62,6 +63,8 @@ static inline void sanity_check_seg_type(struct f2fs_sb_info *sbi,
> >         ((secno) == CURSEG_I(sbi, CURSEG_COLD_DATA_PINNED)->segno /     \
> >          (sbi)->segs_per_sec) ||        \
> >         ((secno) == CURSEG_I(sbi, CURSEG_ALL_DATA_ATGC)->segno /        \
> > +       (sbi)->segs_per_sec) ||        \
> > +      ((secno) == CURSEG_I(sbi, CURSEG_COLD_DATA_DEFRAG)->segno /     \
> >          (sbi)->segs_per_sec))
> >
> >  #define MAIN_BLKADDR(sbi)                                            \
> > --
> > 2.7.4
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel