On 11/10, heyunlei wrote:
>
>
> >-----Original Message-----
> >From: Jaegeuk Kim [mailto:[email protected]]
> >Sent: Friday, November 10, 2017 2:05 AM
> >To: heyunlei
> >Cc: Yuchao (T); [email protected]; Wangbintian; Jianing
> >(Euler)
> >Subject: Re: [f2fs-dev][PATCH v5] f2fs: Separate nat entry mem alloc from
> >nat_tree_locks
> >
> >On 11/07, Yunlei He wrote:
> >> This path separate nat_entry mem alloc from nat_tree_lock,
> >> which will benefit in some low mem case.
> >>
> >> Signed-off-by: Yunlei He <[email protected]>
> >> ---
> >> fs/f2fs/node.c | 56
> >> +++++++++++++++++++++++++++++++++++++++-----------------
> >> 1 file changed, 39 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >> index fef5c68..c6aea33 100644
> >> --- a/fs/f2fs/node.c
> >> +++ b/fs/f2fs/node.c
> >> @@ -257,22 +257,15 @@ static struct nat_entry *grab_nat_entry(struct
> >> f2fs_nm_info *nm_i, nid_t nid,
> >>
> >> if (no_fail) {
> >> new = f2fs_kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
> >> - f2fs_radix_tree_insert(&nm_i->nat_root, nid, new);
> >> } else {
> >> new = kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
> >> if (!new)
> >> return NULL;
> >> - if (radix_tree_insert(&nm_i->nat_root, nid, new)) {
> >> - kmem_cache_free(nat_entry_slab, new);
> >> - return NULL;
> >> - }
> >> }
> >>
> >> memset(new, 0, sizeof(struct nat_entry));
> >> nat_set_nid(new, nid);
> >> nat_reset_flag(new);
> >> - list_add_tail(&new->list, &nm_i->nat_entries);
> >> - nm_i->nat_cnt++;
> >> return new;
> >> }
> >>
> >> @@ -282,17 +275,32 @@ static void cache_nat_entry(struct f2fs_sb_info
> >> *sbi, nid_t nid,
> >> struct f2fs_nm_info *nm_i = NM_I(sbi);
> >> struct nat_entry *e;
> >>
> >> +#ifdef CONFIG_F2FS_CHECK_FS
> >> + down_read(&nm_i->nat_tree_lock);
> >> e = __lookup_nat_cache(nm_i, nid);
> >> - if (!e) {
> >> - e = grab_nat_entry(nm_i, nid, false);
> >> - if (e)
> >> - node_info_from_raw_nat(&e->ni, ne);
> >> - } else {
> >> + if (unlikely(e)) {
> >> f2fs_bug_on(sbi, nat_get_ino(e) != le32_to_cpu(ne->ino) ||
> >> - nat_get_blkaddr(e) !=
> >> - le32_to_cpu(ne->block_addr) ||
> >> + nat_get_blkaddr(e) != le32_to_cpu(ne->block_addr) ||
> >> nat_get_version(e) != ne->version);
> >> + up_read(&nm_i->nat_tree_lock);
> >> + return;
> >> + }
> >> + up_read(&nm_i->nat_tree_lock);
> >> +#endif
> >
> >Please avoid #ifdef as much as possible, which is totally undesirable.
> >BTW, it'd be better to give the mallocated e to grab_nat_entry() to
> >consolidate
> >initialization stuffs?
>
> Do you mean that delete the found nat entry and use the new alloced one？
Like this?
---
fs/f2fs/node.c | 82 ++++++++++++++++++++++++++++++++++++----------------------
1 file changed, 51 insertions(+), 31 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index d2530179cc9c..856b3aa9cab1 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -250,49 +250,67 @@ bool need_inode_block_update(struct f2fs_sb_info *sbi,
nid_t ino)
return need_update;
}
-static struct nat_entry *grab_nat_entry(struct f2fs_nm_info *nm_i, nid_t nid,
- bool no_fail)
+static struct nat_entry *__alloc_nat_entry(nid_t nid, bool no_fail)
{
- struct nat_entry *new;
+ struct nat_entry *new = NULL;
- if (no_fail) {
- new = f2fs_kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
- f2fs_radix_tree_insert(&nm_i->nat_root, nid, new);
- } else {
- new = kmem_cache_alloc(nat_entry_slab, GFP_NOFS);
- if (!new)
- return NULL;
- if (radix_tree_insert(&nm_i->nat_root, nid, new)) {
- kmem_cache_free(nat_entry_slab, new);
- return NULL;
- }
+ if (no_fail)
+ new = f2fs_kmem_cache_alloc(nat_entry_slab,
+ GFP_NOFS | __GFP_ZERO);
+ else
+ new = kmem_cache_alloc(nat_entry_slab,
+ GFP_NOFS | __GFP_ZERO);
+ if (new) {
+ nat_set_nid(new, nid);
+ nat_reset_flag(new);
}
+ return new;
+}
+
+static void __free_nat_entry(struct nat_entry *e)
+{
+ kmem_cache_free(nat_entry_slab, e);
+}
- memset(new, 0, sizeof(struct nat_entry));
- nat_set_nid(new, nid);
- nat_reset_flag(new);
- list_add_tail(&new->list, &nm_i->nat_entries);
+/* must be locked by nat_tree_lock */
+static struct nat_entry *__init_nat_entry(struct f2fs_nm_info *nm_i,
+ struct nat_entry *ne, struct f2fs_nat_entry *raw_ne, bool no_fail)
+{
+ if (no_fail)
+ f2fs_radix_tree_insert(&nm_i->nat_root, nat_get_nid(ne), ne);
+ else if (radix_tree_insert(&nm_i->nat_root, nat_get_nid(ne), ne))
+ return NULL;
+
+ if (raw_ne)
+ node_info_from_raw_nat(&ne->ni, raw_ne);
+ list_add_tail(&ne->list, &nm_i->nat_entries);
nm_i->nat_cnt++;
- return new;
+ return ne;
}
+/* must be locked by nat_tree_lock */
static void cache_nat_entry(struct f2fs_sb_info *sbi, nid_t nid,
struct f2fs_nat_entry *ne)
{
struct f2fs_nm_info *nm_i = NM_I(sbi);
- struct nat_entry *e;
+ struct nat_entry *new, *e;
+
+ new = __alloc_nat_entry(nid, false);
+ if (!new)
+ return;
+ down_write(&nm_i->nat_tree_lock);
e = __lookup_nat_cache(nm_i, nid);
- if (!e) {
- e = grab_nat_entry(nm_i, nid, false);
- if (e)
- node_info_from_raw_nat(&e->ni, ne);
- } else {
+ if (!e)
+ e = __init_nat_entry(nm_i, new, ne, false);
+ else
f2fs_bug_on(sbi, nat_get_ino(e) != le32_to_cpu(ne->ino) ||
nat_get_blkaddr(e) !=
le32_to_cpu(ne->block_addr) ||
nat_get_version(e) != ne->version);
- }
+ up_write(&nm_i->nat_tree_lock);
+ if (e != new)
+ __free_nat_entry(new);
}
static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni,
@@ -300,11 +318,12 @@ static void set_node_addr(struct f2fs_sb_info *sbi,
struct node_info *ni,
{
struct f2fs_nm_info *nm_i = NM_I(sbi);
struct nat_entry *e;
+ struct nat_entry *new = __alloc_nat_entry(ni->nid, true);
down_write(&nm_i->nat_tree_lock);
e = __lookup_nat_cache(nm_i, ni->nid);
if (!e) {
- e = grab_nat_entry(nm_i, ni->nid, true);
+ e = __init_nat_entry(nm_i, new, NULL, true);
copy_node_info(&e->ni, ni);
f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR);
} else if (new_blkaddr == NEW_ADDR) {
@@ -316,6 +335,9 @@ static void set_node_addr(struct f2fs_sb_info *sbi, struct
node_info *ni,
copy_node_info(&e->ni, ni);
f2fs_bug_on(sbi, ni->blk_addr != NULL_ADDR);
}
+ /* let's free early to reduce memory consumption */
+ if (e != new)
+ __free_nat_entry(new);
/* sanity check */
f2fs_bug_on(sbi, nat_get_blkaddr(e) != ni->blk_addr);
@@ -424,9 +446,7 @@ void get_node_info(struct f2fs_sb_info *sbi, nid_t nid,
struct node_info *ni)
f2fs_put_page(page, 1);
cache:
/* cache nat entry */
- down_write(&nm_i->nat_tree_lock);
cache_nat_entry(sbi, nid, &ne);
- up_write(&nm_i->nat_tree_lock);
}
/*
@@ -2371,8 +2391,8 @@ static void remove_nats_in_journal(struct f2fs_sb_info
*sbi)
ne = __lookup_nat_cache(nm_i, nid);
if (!ne) {
- ne = grab_nat_entry(nm_i, nid, true);
- node_info_from_raw_nat(&ne->ni, &raw_ne);
+ ne = __alloc_nat_entry(nid, true);
+ __init_nat_entry(nm_i, ne, &raw_ne, true);
}
/*
--
2.14.0.rc1.383.gd1ce394fe2-goog
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
