* [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks @ 2017-11-07 9:47 Yunlei He 2017-11-08 8:46 ` Chao Yu 2017-11-09 18:04 ` Jaegeuk Kim 0 siblings, 2 replies; 7+ messages in thread From: Yunlei He @ 2017-11-07 9:47 UTC (permalink / raw) To: jaegeuk, yuchao0, linux-f2fs-devel; +Cc: ning.jia, heyunlei This path separate nat_entry mem alloc from nat_tree_lock, which will benefit in some low mem case. Signed-off-by: Yunlei He <heyunlei@huawei.com> --- 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 + e = grab_nat_entry(nm_i, nid, false); + down_write(&nm_i->nat_tree_lock); + if (!e) + goto out; + + if (!radix_tree_insert(&nm_i->nat_root, nid, e)) { + list_add_tail(&e->list, &nm_i->nat_entries); + nm_i->nat_cnt++; + node_info_from_raw_nat(&e->ni, ne); + } else { + kmem_cache_free(nat_entry_slab, e); } +out: + up_write(&nm_i->nat_tree_lock); } static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni, @@ -300,11 +308,21 @@ 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 = grab_nat_entry(nm_i, ni->nid, true); +retry: 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 = new; + if (radix_tree_insert(&nm_i->nat_root, ni->nid, e)) { + up_write(&nm_i->nat_tree_lock); + cond_resched(); + goto retry; + } + + list_add_tail(&e->list, &nm_i->nat_entries); + nm_i->nat_cnt++; copy_node_info(&e->ni, ni); f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR); } else if (new_blkaddr == NEW_ADDR) { @@ -315,6 +333,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); + kmem_cache_free(nat_entry_slab, new); + } else { + kmem_cache_free(nat_entry_slab, new); } /* sanity check */ @@ -424,9 +445,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); } /* @@ -2375,6 +2394,9 @@ 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); + f2fs_radix_tree_insert(&nm_i->nat_root, nid, ne); + list_add_tail(&ne->list, &nm_i->nat_entries); + nm_i->nat_cnt++; node_info_from_raw_nat(&ne->ni, &raw_ne); } -- 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-07 9:47 [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks Yunlei He @ 2017-11-08 8:46 ` Chao Yu 2017-11-09 18:04 ` Jaegeuk Kim 1 sibling, 0 replies; 7+ messages in thread From: Chao Yu @ 2017-11-08 8:46 UTC (permalink / raw) To: Yunlei He, jaegeuk, linux-f2fs-devel; +Cc: ning.jia On 2017/11/7 17:47, 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 <heyunlei@huawei.com> > --- > 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 > + e = grab_nat_entry(nm_i, nid, false); if (!e) return; Thanks, > + down_write(&nm_i->nat_tree_lock); > + if (!e) > + goto out; > + > + if (!radix_tree_insert(&nm_i->nat_root, nid, e)) { > + list_add_tail(&e->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > + node_info_from_raw_nat(&e->ni, ne); > + } else { > + kmem_cache_free(nat_entry_slab, e); > } > +out: > + up_write(&nm_i->nat_tree_lock); > } > > static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni, > @@ -300,11 +308,21 @@ 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 = grab_nat_entry(nm_i, ni->nid, true); > > +retry: > 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 = new; > + if (radix_tree_insert(&nm_i->nat_root, ni->nid, e)) { > + up_write(&nm_i->nat_tree_lock); > + cond_resched(); > + goto retry; > + } > + > + list_add_tail(&e->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > copy_node_info(&e->ni, ni); > f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR); > } else if (new_blkaddr == NEW_ADDR) { > @@ -315,6 +333,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); > + kmem_cache_free(nat_entry_slab, new); > + } else { > + kmem_cache_free(nat_entry_slab, new); > } > > /* sanity check */ > @@ -424,9 +445,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); > } > > /* > @@ -2375,6 +2394,9 @@ 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); > + f2fs_radix_tree_insert(&nm_i->nat_root, nid, ne); > + list_add_tail(&ne->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > node_info_from_raw_nat(&ne->ni, &raw_ne); > } > > ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-07 9:47 [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks Yunlei He 2017-11-08 8:46 ` Chao Yu @ 2017-11-09 18:04 ` Jaegeuk Kim 2017-11-10 3:26 ` heyunlei 1 sibling, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2017-11-09 18:04 UTC (permalink / raw) To: Yunlei He; +Cc: ning.jia, linux-f2fs-devel 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 <heyunlei@huawei.com> > --- > 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? > + e = grab_nat_entry(nm_i, nid, false); > + down_write(&nm_i->nat_tree_lock); > + if (!e) > + goto out; > + > + if (!radix_tree_insert(&nm_i->nat_root, nid, e)) { > + list_add_tail(&e->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > + node_info_from_raw_nat(&e->ni, ne); > + } else { > + kmem_cache_free(nat_entry_slab, e); > } > +out: > + up_write(&nm_i->nat_tree_lock); > } > > static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni, > @@ -300,11 +308,21 @@ 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 = grab_nat_entry(nm_i, ni->nid, true); > > +retry: > 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 = new; > + if (radix_tree_insert(&nm_i->nat_root, ni->nid, e)) { > + up_write(&nm_i->nat_tree_lock); > + cond_resched(); > + goto retry; > + } > + > + list_add_tail(&e->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > copy_node_info(&e->ni, ni); > f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR); > } else if (new_blkaddr == NEW_ADDR) { > @@ -315,6 +333,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); > + kmem_cache_free(nat_entry_slab, new); > + } else { > + kmem_cache_free(nat_entry_slab, new); > } > > /* sanity check */ > @@ -424,9 +445,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); > } > > /* > @@ -2375,6 +2394,9 @@ 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); > + f2fs_radix_tree_insert(&nm_i->nat_root, nid, ne); > + list_add_tail(&ne->list, &nm_i->nat_entries); > + nm_i->nat_cnt++; > node_info_from_raw_nat(&ne->ni, &raw_ne); > } > > -- > 1.9.1 ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-09 18:04 ` Jaegeuk Kim @ 2017-11-10 3:26 ` heyunlei 2017-11-11 0:14 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: heyunlei @ 2017-11-10 3:26 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net >-----Original Message----- >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >Sent: Friday, November 10, 2017 2:05 AM >To: heyunlei >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; 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 <heyunlei@huawei.com> >> --- >> 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？ Thanks. > >> + e = grab_nat_entry(nm_i, nid, false); >> + down_write(&nm_i->nat_tree_lock); >> + if (!e) >> + goto out; >> + >> + if (!radix_tree_insert(&nm_i->nat_root, nid, e)) { >> + list_add_tail(&e->list, &nm_i->nat_entries); >> + nm_i->nat_cnt++; >> + node_info_from_raw_nat(&e->ni, ne); >> + } else { >> + kmem_cache_free(nat_entry_slab, e); >> } >> +out: >> + up_write(&nm_i->nat_tree_lock); >> } >> >> static void set_node_addr(struct f2fs_sb_info *sbi, struct node_info *ni, >> @@ -300,11 +308,21 @@ 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 = grab_nat_entry(nm_i, ni->nid, true); >> >> +retry: >> 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 = new; >> + if (radix_tree_insert(&nm_i->nat_root, ni->nid, e)) { >> + up_write(&nm_i->nat_tree_lock); >> + cond_resched(); >> + goto retry; >> + } >> + >> + list_add_tail(&e->list, &nm_i->nat_entries); >> + nm_i->nat_cnt++; >> copy_node_info(&e->ni, ni); >> f2fs_bug_on(sbi, ni->blk_addr == NEW_ADDR); >> } else if (new_blkaddr == NEW_ADDR) { >> @@ -315,6 +333,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); >> + kmem_cache_free(nat_entry_slab, new); >> + } else { >> + kmem_cache_free(nat_entry_slab, new); >> } >> >> /* sanity check */ >> @@ -424,9 +445,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); >> } >> >> /* >> @@ -2375,6 +2394,9 @@ 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); >> + f2fs_radix_tree_insert(&nm_i->nat_root, nid, ne); >> + list_add_tail(&ne->list, &nm_i->nat_entries); >> + nm_i->nat_cnt++; >> node_info_from_raw_nat(&ne->ni, &raw_ne); >> } >> >> -- >> 1.9.1 ------------------------------------------------------------------------------ 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 Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-10 3:26 ` heyunlei @ 2017-11-11 0:14 ` Jaegeuk Kim 2017-11-11 1:16 ` Chao Yu 0 siblings, 1 reply; 7+ messages in thread From: Jaegeuk Kim @ 2017-11-11 0:14 UTC (permalink / raw) To: heyunlei; +Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net On 11/10, heyunlei wrote: > > > >-----Original Message----- > >From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > >Sent: Friday, November 10, 2017 2:05 AM > >To: heyunlei > >Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; 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 <heyunlei@huawei.com> > >> --- > >> 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 Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-11 0:14 ` Jaegeuk Kim @ 2017-11-11 1:16 ` Chao Yu 2017-11-12 2:40 ` Jaegeuk Kim 0 siblings, 1 reply; 7+ messages in thread From: Chao Yu @ 2017-11-11 1:16 UTC (permalink / raw) To: Jaegeuk Kim, heyunlei Cc: Jianing (Euler), linux-f2fs-devel@lists.sourceforge.net On 2017/11/11 8:14, Jaegeuk Kim wrote: > On 11/10, heyunlei wrote: >> >> >>> -----Original Message----- >>> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] >>> Sent: Friday, November 10, 2017 2:05 AM >>> To: heyunlei >>> Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; 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 <heyunlei@huawei.com> >>>> --- >>>> 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 <yuchao0@huawei.com> 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 Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks 2017-11-11 1:16 ` Chao Yu @ 2017-11-12 2:40 ` Jaegeuk Kim 0 siblings, 0 replies; 7+ messages in thread From: Jaegeuk Kim @ 2017-11-12 2:40 UTC (permalink / raw) To: Chao Yu; +Cc: Jianing (Euler), heyunlei, linux-f2fs-devel@lists.sourceforge.net On 11/11, Chao Yu wrote: > On 2017/11/11 8:14, Jaegeuk Kim wrote: > > On 11/10, heyunlei wrote: > >> > >> > >>> -----Original Message----- > >>> From: Jaegeuk Kim [mailto:jaegeuk@kernel.org] > >>> Sent: Friday, November 10, 2017 2:05 AM > >>> To: heyunlei > >>> Cc: Yuchao (T); linux-f2fs-devel@lists.sourceforge.net; 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 <heyunlei@huawei.com> > >>>> --- > >>>> 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 <yuchao0@huawei.com> > > Trivial comments as below: Done. From c79d88f915ed9219d3f021dcb93375e5da2b43f3 Mon Sep 17 00:00:00 2001 From: Yunlei He <heyunlei@huawei.com> Date: Fri, 10 Nov 2017 13:36:51 -0800 Subject: [PATCH] f2fs: separate nat entry mem alloc from nat_tree_lock This patch splits memory allocation part in nat_entry to avoid lock contention. Suggested-by: Yunlei He <heyunlei@huawei.com> Reviewed-by: Chao Yu <yuchao0@huawei.com> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> --- fs/f2fs/node.c | 98 +++++++++++++++++++++++++++++++++++----------------------- 1 file changed, 59 insertions(+), 39 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 9abfdbb5aae5..fe1fc662af2a 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -138,6 +138,44 @@ static struct page *get_next_nat_page(struct f2fs_sb_info *sbi, nid_t nid) return dst_page; } +static struct nat_entry *__alloc_nat_entry(nid_t nid, bool no_fail) +{ + struct nat_entry *new; + + 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); +} + +/* 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 ne; +} + static struct nat_entry *__lookup_nat_cache(struct f2fs_nm_info *nm_i, nid_t n) { return radix_tree_lookup(&nm_i->nat_root, n); @@ -154,7 +192,7 @@ static void __del_from_nat_cache(struct f2fs_nm_info *nm_i, struct nat_entry *e) list_del(&e->list); radix_tree_delete(&nm_i->nat_root, nat_get_nid(e)); nm_i->nat_cnt--; - kmem_cache_free(nat_entry_slab, e); + __free_nat_entry(e); } static void __set_nat_cache_dirty(struct f2fs_nm_info *nm_i, @@ -250,49 +288,29 @@ 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) -{ - struct nat_entry *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; - } - } - - 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; -} - +/* 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); } /* @@ -2374,8 +2394,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 Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-11-12 2:41 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-11-07 9:47 [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks Yunlei He 2017-11-08 8:46 ` Chao Yu 2017-11-09 18:04 ` Jaegeuk Kim 2017-11-10 3:26 ` heyunlei 2017-11-11 0:14 ` Jaegeuk Kim 2017-11-11 1:16 ` Chao Yu 2017-11-12 2:40 ` Jaegeuk Kim
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).