On 04/20, Chao Yu wrote:
> On 2018/4/20 12:04, Chao Yu wrote:
> > On 2018/4/20 11:37, Jaegeuk Kim wrote:
> >> On 04/20, Chao Yu wrote:
> >>> As most indirect node, dindirect node, and xattr node won't be updated
> >>> after they are created, but inode node and other direct node will change
> >>> more frequently, so store their nat entries mixedly in whole nat table
> >>> will suffer:
> >>> - fragment nat table soon due to different update rate
> >>> - more nat block update due to fragmented nat table
> >>>
> >>> In order to solve above issue, we're trying to separate whole nat table to
> >>> two part:
> >>> a. Hot free nid area:
> >>> - range: [nid #0, nid #x)
> >>> - store node block address for
> >>> * inode node
> >>> * other direct node
> >>> b. Cold free nid area:
> >>> - range: [nid #x, max nid)
> >>> - store node block address for
> >>> * indirect node
> >>> * dindirect node
> >>> * xattr node
> >>>
> >>> Allocation strategy example:
> >>>
> >>> Free nid: '-'
> >>> Used nid: '='
> >>>
> >>> 1. Initial status:
> >>> Free Nids:
> >>> |-----------------------------------------------------------------------|
> >>> ^ ^ ^
> >>> ^
> >>> Alloc Range: |---------------|
> >>> |---------------|
> >>> hot_start hot_end
> >>> cold_start cold_end
> >>>
> >>> 2. Free nids have ran out:
> >>> Free Nids:
> >>> |===============-----------------------------------------===============|
> >>> ^ ^ ^
> >>> ^
> >>> Alloc Range: |===============|
> >>> |===============|
> >>> hot_start hot_end
> >>> cold_start cold_end
> >>>
> >>> 3. Expand hot/cold area range:
> >>> Free Nids:
> >>> |===============-----------------------------------------===============|
> >>> ^ ^ ^
> >>> ^
> >>> Alloc Range: |===============----------------|
> >>> |----------------===============|
> >>> hot_start hot_end cold_start
> >>> cold_end
> >>>
> >>> 4. Hot free nids have ran out:
> >>> Free Nids:
> >>> |===============================-------------------------===============|
> >>> ^ ^ ^
> >>> ^
> >>> Alloc Range: |===============================|
> >>> |----------------===============|
> >>> hot_start hot_end cold_start
> >>> cold_end
> >>>
> >>> 5. Expand hot area range, hot/cold area boundary has been fixed:
> >>> Free Nids:
> >>> |===============================-------------------------===============|
> >>> ^ ^
> >>> ^
> >>> Alloc Range:
> >>> |===============================--------|----------------===============|
> >>> hot_start hot_end(cold_start)
> >>> cold_end
> >>>
> >>> Run xfstests with generic/*:
> >>>
> >>> before
> >>> node_write: 169660
> >>> cp_count: 60118
> >>> node/cp 2.82
> >>>
> >>> after:
> >>> node_write: 159145
> >>> cp_count: 84501
> >>> node/cp: 2.64
> >>
> >> Nice trial tho, I don't see much benefit on this huge patch. I guess we
> >> may be
> >> able to find an efficient way to achieve this issue rather than changing
> >> whole
> >> stable codes.
> >
> > IMO, based on this, later, we can add more allocation policy to manage free
> > nid
> > resource to get more benefit.
> >
> > If you worry about code stability, we can queue this patch in dev-test
> > branch to
> > test this longer time.
> >
> >>
> >> How about getting a free nid in the list from head or tail separately?
> >
> > I don't think this can get benefit from long time used image, since nat
> > table
> > will be fragmented anyway, then we won't know free nid in head or in tail
> > comes
> > from hot nat block or cold nat block.
> >
> > Anyway, I will have a try.
>
> A quick test result with below patch:
>
> node_write:187837, cp_count:76431
Can we gather some real numbers from android workloads?
Thanks,
>
> >From 9f88ea8a36a74f1d3ed8df57ceffb1b8ae41a161 Mon Sep 17 00:00:00 2001
> From: Chao Yu <yuch...@huawei.com>
> Date: Fri, 20 Apr 2018 16:18:26 +0800
> Subject: [PATCH] f2fs: separate hot/cold free nid simply
>
> Signed-off-by: Chao Yu <yuch...@huawei.com>
> ---
> fs/f2fs/f2fs.h | 2 +-
> fs/f2fs/namei.c | 2 +-
> fs/f2fs/node.c | 14 +++++++++-----
> fs/f2fs/xattr.c | 2 +-
> 4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7651a118faa3..adca5e8bc19a 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -2800,7 +2800,7 @@ int fsync_node_pages(struct f2fs_sb_info *sbi, struct
> inode *inode,
> int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc,
> bool do_balance, enum iostat_type io_type);
> void build_free_nids(struct f2fs_sb_info *sbi, bool sync, bool mount);
> -bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid);
> +bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid, bool hot);
> void alloc_nid_done(struct f2fs_sb_info *sbi, nid_t nid);
> void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid);
> int try_to_free_nids(struct f2fs_sb_info *sbi, int nr_shrink);
> diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
> index 3d59149590f7..bbf6edbb7298 100644
> --- a/fs/f2fs/namei.c
> +++ b/fs/f2fs/namei.c
> @@ -37,7 +37,7 @@ static struct inode *f2fs_new_inode(struct inode *dir,
> umode_t
> mode)
> return ERR_PTR(-ENOMEM);
>
> f2fs_lock_op(sbi);
> - if (!alloc_nid(sbi, &ino)) {
> + if (!alloc_nid(sbi, &ino, true)) {
> f2fs_unlock_op(sbi);
> err = -ENOSPC;
> goto fail;
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index ea231f8a0cce..bbaae2f3461e 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -631,7 +631,7 @@ int get_dnode_of_data(struct dnode_of_data *dn, pgoff_t
> index, int mode)
>
> if (!nids[i] && mode == ALLOC_NODE) {
> /* alloc new node */
> - if (!alloc_nid(sbi, &(nids[i]))) {
> + if (!alloc_nid(sbi, &(nids[i]), i == 1)) {
> err = -ENOSPC;
> goto release_pages;
> }
> @@ -2108,7 +2108,7 @@ void build_free_nids(struct f2fs_sb_info *sbi, bool
> sync,
> bool mount)
> * from second parameter of this function.
> * The returned nid could be used ino as well as nid when inode is created.
> */
> -bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> +bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid, bool hot)
> {
> struct f2fs_nm_info *nm_i = NM_I(sbi);
> struct free_nid *i = NULL;
> @@ -2129,8 +2129,12 @@ bool alloc_nid(struct f2fs_sb_info *sbi, nid_t *nid)
> /* We should not use stale free nids created by build_free_nids */
> if (nm_i->nid_cnt[FREE_NID] && !on_build_free_nids(nm_i)) {
> f2fs_bug_on(sbi, list_empty(&nm_i->free_nid_list));
> - i = list_first_entry(&nm_i->free_nid_list,
> - struct free_nid, list);
> + if (hot)
> + i = list_first_entry(&nm_i->free_nid_list,
> + struct free_nid, list);
> + else
> + i = list_last_entry(&nm_i->free_nid_list,
> + struct free_nid, list);
> *nid = i->nid;
>
> __move_free_nid(sbi, i, FREE_NID, PREALLOC_NID);
> @@ -2275,7 +2279,7 @@ int recover_xattr_data(struct inode *inode, struct page
> *page)
>
> recover_xnid:
> /* 2: update xattr nid in inode */
> - if (!alloc_nid(sbi, &new_xnid))
> + if (!alloc_nid(sbi, &new_xnid, false))
> return -ENOSPC;
>
> set_new_dnode(&dn, inode, NULL, NULL, new_xnid);
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index d847b2b11659..46c090614563 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -398,7 +398,7 @@ static inline int write_all_xattrs(struct inode *inode,
> __u32 hsize,
> int err = 0;
>
> if (hsize > inline_size && !F2FS_I(inode)->i_xattr_nid)
> - if (!alloc_nid(sbi, &new_nid))
> + if (!alloc_nid(sbi, &new_nid, false))
> return -ENOSPC;
>
> /* write to inline xattr */
> --
> 2.15.0.55.gc2ece9dc4de6
>
