On 2017/11/11 8:14, Jaegeuk Kim wrote: > 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?
From my aspect, more neat. :) Reviewed-by: Chao Yu <[email protected]> Trivial comments as below: > > --- > 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; No need to initialize new? > > - 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); Should also replace kmem_cache_free in __del_from_nat_cache? Thanks, > +} > > - 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); > } > > /* > ------------------------------------------------------------------------------ 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
