* [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management @ 2013-05-06 15:15 Haicheng Li 2013-05-06 15:15 ` [PATCH 1/4] f2fs: bugfix for alloc_nid_failed() Haicheng Li ` (3 more replies) 0 siblings, 4 replies; 11+ messages in thread From: Haicheng Li @ 2013-05-06 15:15 UTC (permalink / raw) To: linux-fsdevel, linux-f2fs-devel, Jaegeuk Kim Cc: linux-kernel, Haicheng Li, Haicheng Li Found several issues during my code-review of f2fs/node.c, but my orignial patches are based on Kim's linux-f2fs master branch, and they cannot be smoothly applied to latest dev branch. So rebased and combined these patches into this patchset. V1->V2: rebase the patches against Kim's linux-f2fs dev branch. Haicheng Li (4): f2fs: bugfix for alloc_nid_failed() f2fs: code cleanup for scan_nat_page() and build_free_nids() f2fs: optimize scan_nat_page() f2fs: optimize build_free_nids() fs/f2fs/node.c | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) -- 1.7.9.5 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/4] f2fs: bugfix for alloc_nid_failed() 2013-05-06 15:15 [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management Haicheng Li @ 2013-05-06 15:15 ` Haicheng Li 2013-05-06 15:15 ` [PATCH 2/4] f2fs: code cleanup for scan_nat_page() and build_free_nids() Haicheng Li ` (2 subsequent siblings) 3 siblings, 0 replies; 11+ messages in thread From: Haicheng Li @ 2013-05-06 15:15 UTC (permalink / raw) To: linux-fsdevel, linux-f2fs-devel, Jaegeuk Kim Cc: linux-kernel, Haicheng Li, Haicheng Li Directly drop the free_nid cache when nm_i->fcnt > 2 * MAX_FREE_NIDS Since there is NOT nmi->free_nid_list_lock spinlock protection between a sequential calling of alloc_nid() and alloc_nid_failed(), some other threads may already add new free_nid to the free_nid_list during this period. We need to make sure nmi->fcnt is never > 2 * MAX_FREE_NIDS. Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> --- fs/f2fs/node.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 7209d63..532ac57 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1439,8 +1439,12 @@ void alloc_nid_failed(struct f2fs_sb_info *sbi, nid_t nid) spin_lock(&nm_i->free_nid_list_lock); i = __lookup_free_nid_list(nid, &nm_i->free_nid_list); BUG_ON(!i || i->state != NID_ALLOC); - i->state = NID_NEW; - nm_i->fcnt++; + if (nm_i->fcnt > 2 * MAX_FREE_NIDS) + __del_from_free_nid_list(i); + else { + i->state = NID_NEW; + nm_i->fcnt++; + } spin_unlock(&nm_i->free_nid_list_lock); } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/4] f2fs: code cleanup for scan_nat_page() and build_free_nids() 2013-05-06 15:15 [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management Haicheng Li 2013-05-06 15:15 ` [PATCH 1/4] f2fs: bugfix for alloc_nid_failed() Haicheng Li @ 2013-05-06 15:15 ` Haicheng Li 2013-05-06 15:15 ` [PATCH 3/4] f2fs: optimize scan_nat_page() Haicheng Li 2013-05-06 15:15 ` [PATCH 4/4] f2fs: optimize build_free_nids() Haicheng Li 3 siblings, 0 replies; 11+ messages in thread From: Haicheng Li @ 2013-05-06 15:15 UTC (permalink / raw) To: linux-fsdevel, linux-f2fs-devel, Jaegeuk Kim Cc: linux-kernel, Haicheng Li, Haicheng Li This patch does two cleanups: 1. remove unused variable "fcnt" in build_free_nids(). 2. make scan_nat_page() as void type and remove useless variable "fcnt". Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> --- fs/f2fs/node.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 532ac57..1b45dd0 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1292,12 +1292,11 @@ static void remove_free_nid(struct f2fs_nm_info *nm_i, nid_t nid) spin_unlock(&nm_i->free_nid_list_lock); } -static int scan_nat_page(struct f2fs_nm_info *nm_i, +static void scan_nat_page(struct f2fs_nm_info *nm_i, struct page *nat_page, nid_t start_nid) { struct f2fs_nat_block *nat_blk = page_address(nat_page); block_t blk_addr; - int fcnt = 0; int i; i = start_nid % NAT_ENTRY_PER_BLOCK; @@ -1308,9 +1307,8 @@ static int scan_nat_page(struct f2fs_nm_info *nm_i, blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); BUG_ON(blk_addr == NEW_ADDR); if (blk_addr == NULL_ADDR) - fcnt += add_free_nid(nm_i, start_nid); + add_free_nid(nm_i, start_nid); } - return fcnt; } static void build_free_nids(struct f2fs_sb_info *sbi) @@ -1319,7 +1317,7 @@ static void build_free_nids(struct f2fs_sb_info *sbi) struct f2fs_nm_info *nm_i = NM_I(sbi); struct curseg_info *curseg = CURSEG_I(sbi, CURSEG_HOT_DATA); struct f2fs_summary_block *sum = curseg->sum_blk; - int fcnt = 0, i = 0; + int i = 0; nid_t nid = nm_i->next_scan_nid; /* Enough entries */ @@ -1332,7 +1330,7 @@ static void build_free_nids(struct f2fs_sb_info *sbi) while (1) { struct page *page = get_current_nat_page(sbi, nid); - fcnt += scan_nat_page(nm_i, page, nid); + scan_nat_page(nm_i, page, nid); f2fs_put_page(page, 1); nid += (NAT_ENTRY_PER_BLOCK - (nid % NAT_ENTRY_PER_BLOCK)); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 3/4] f2fs: optimize scan_nat_page() 2013-05-06 15:15 [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management Haicheng Li 2013-05-06 15:15 ` [PATCH 1/4] f2fs: bugfix for alloc_nid_failed() Haicheng Li 2013-05-06 15:15 ` [PATCH 2/4] f2fs: code cleanup for scan_nat_page() and build_free_nids() Haicheng Li @ 2013-05-06 15:15 ` Haicheng Li 2013-05-07 10:36 ` Jaegeuk Kim 2013-05-06 15:15 ` [PATCH 4/4] f2fs: optimize build_free_nids() Haicheng Li 3 siblings, 1 reply; 11+ messages in thread From: Haicheng Li @ 2013-05-06 15:15 UTC (permalink / raw) To: linux-fsdevel, linux-f2fs-devel, Jaegeuk Kim Cc: linux-kernel, Haicheng Li, Haicheng Li When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT entries. Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> --- fs/f2fs/node.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1b45dd0..1fe3fe2 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1254,7 +1254,7 @@ static int add_free_nid(struct f2fs_nm_info *nm_i, nid_t nid) struct free_nid *i; if (nm_i->fcnt > 2 * MAX_FREE_NIDS) - return 0; + return -1; /* 0 nid should not be used */ if (nid == 0) @@ -1302,12 +1302,17 @@ static void scan_nat_page(struct f2fs_nm_info *nm_i, i = start_nid % NAT_ENTRY_PER_BLOCK; for (; i < NAT_ENTRY_PER_BLOCK; i++, start_nid++) { + int cnt; + if (start_nid >= nm_i->max_nid) break; - blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); + blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); BUG_ON(blk_addr == NEW_ADDR); - if (blk_addr == NULL_ADDR) - add_free_nid(nm_i, start_nid); + if (blk_addr == NULL_ADDR) { + cnt = add_free_nid(nm_i, start_nid); + if (cnt < 0) + break; + } } } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] f2fs: optimize scan_nat_page() 2013-05-06 15:15 ` [PATCH 3/4] f2fs: optimize scan_nat_page() Haicheng Li @ 2013-05-07 10:36 ` Jaegeuk Kim 2013-05-08 5:31 ` Haicheng Li 0 siblings, 1 reply; 11+ messages in thread From: Jaegeuk Kim @ 2013-05-07 10:36 UTC (permalink / raw) To: Haicheng Li; +Cc: linux-fsdevel, linux-f2fs-devel, linux-kernel, Haicheng Li [-- Attachment #1: Type: text/plain, Size: 3368 bytes --] Hi, 2013-05-06 (월), 23:15 +0800, Haicheng Li: > When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT entries. > > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> > --- > fs/f2fs/node.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 1b45dd0..1fe3fe2 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1254,7 +1254,7 @@ static int add_free_nid(struct f2fs_nm_info *nm_i, nid_t nid) > struct free_nid *i; > > if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > - return 0; > + return -1; We should check all the handler of add_free_nid(). So, plz see the below modified patch. > > /* 0 nid should not be used */ > if (nid == 0) > @@ -1302,12 +1302,17 @@ static void scan_nat_page(struct f2fs_nm_info *nm_i, > i = start_nid % NAT_ENTRY_PER_BLOCK; > > for (; i < NAT_ENTRY_PER_BLOCK; i++, start_nid++) { > + int cnt; > + > if (start_nid >= nm_i->max_nid) > break; > - blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); > + blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); > BUG_ON(blk_addr == NEW_ADDR); > - if (blk_addr == NULL_ADDR) > - add_free_nid(nm_i, start_nid); > + if (blk_addr == NULL_ADDR) { > + cnt = add_free_nid(nm_i, start_nid); > + if (cnt < 0) > + break; > + } Here we need to eliminate cnt. > } > } > ----- From e2da02f0ba045f792d166ac8215d04474c06f319 Mon Sep 17 00:00:00 2001 From: Haicheng Li <haicheng.li@linux.intel.com> Date: Mon, 6 May 2013 23:15:43 +0800 Subject: [PATCH] f2fs: optimize scan_nat_page() Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT entries. Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> [Jaegeuk Kim: fix handling the return value of add_free_nid()] Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com> --- fs/f2fs/node.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 122200e..e42934e 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1254,7 +1254,7 @@ static int add_free_nid(struct f2fs_nm_info *nm_i, nid_t nid) struct free_nid *i; if (nm_i->fcnt > 2 * MAX_FREE_NIDS) - return 0; + return -1; /* 0 nid should not be used */ if (nid == 0) @@ -1302,12 +1302,16 @@ static void scan_nat_page(struct f2fs_nm_info *nm_i, i = start_nid % NAT_ENTRY_PER_BLOCK; for (; i < NAT_ENTRY_PER_BLOCK; i++, start_nid++) { + if (start_nid >= nm_i->max_nid) break; - blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); + + blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); BUG_ON(blk_addr == NEW_ADDR); - if (blk_addr == NULL_ADDR) - add_free_nid(nm_i, start_nid); + if (blk_addr == NULL_ADDR) { + if (add_free_nid(nm_i, start_nid) < 0) + break; + } } } @@ -1655,7 +1659,7 @@ flush_now: } if (nat_get_blkaddr(ne) == NULL_ADDR && - !add_free_nid(NM_I(sbi), nid)) { + add_free_nid(NM_I(sbi), nid) <= 0) { write_lock(&nm_i->nat_tree_lock); __del_from_nat_cache(nm_i, ne); write_unlock(&nm_i->nat_tree_lock); -- 1.8.1.3.566.gaa39828 -- Jaegeuk Kim Samsung [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 3/4] f2fs: optimize scan_nat_page() 2013-05-07 10:36 ` Jaegeuk Kim @ 2013-05-08 5:31 ` Haicheng Li 0 siblings, 0 replies; 11+ messages in thread From: Haicheng Li @ 2013-05-08 5:31 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, Haicheng Li, linux-fsdevel, linux-f2fs-devel On Tue, May 07, 2013 at 07:36:28PM +0900, Jaegeuk Kim wrote: > Hi, > > 2013-05-06 (월), 23:15 +0800, Haicheng Li: > > if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > > - return 0; > > + return -1; > > We should check all the handler of add_free_nid(). yes, sorry that I missed double checking it while rebasing this patch from master to dev branch, since there is only one handler on the master branch. > So, plz see the below modified patch. > > + if (blk_addr == NULL_ADDR) { > > + cnt = add_free_nid(nm_i, start_nid); > > + if (cnt < 0) > > + break; > > + } > > Here we need to eliminate cnt. sure, harmless & can reduce the code line. and the below patch looks good to me. Thanks. > ----- > From e2da02f0ba045f792d166ac8215d04474c06f319 Mon Sep 17 00:00:00 2001 > From: Haicheng Li <haicheng.li@linux.intel.com> > Date: Mon, 6 May 2013 23:15:43 +0800 > Subject: [PATCH] f2fs: optimize scan_nat_page() > Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, > linux-f2fs-devel@lists.sourceforge.net > > When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT entries. > > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> > [Jaegeuk Kim: fix handling the return value of add_free_nid()] > Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com> > --- > fs/f2fs/node.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 122200e..e42934e 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1254,7 +1254,7 @@ static int add_free_nid(struct f2fs_nm_info *nm_i, > nid_t nid) > struct free_nid *i; > > if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > - return 0; > + return -1; > > /* 0 nid should not be used */ > if (nid == 0) > @@ -1302,12 +1302,16 @@ static void scan_nat_page(struct f2fs_nm_info > *nm_i, > i = start_nid % NAT_ENTRY_PER_BLOCK; > > for (; i < NAT_ENTRY_PER_BLOCK; i++, start_nid++) { > + > if (start_nid >= nm_i->max_nid) > break; > - blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); > + > + blk_addr = le32_to_cpu(nat_blk->entries[i].block_addr); > BUG_ON(blk_addr == NEW_ADDR); > - if (blk_addr == NULL_ADDR) > - add_free_nid(nm_i, start_nid); > + if (blk_addr == NULL_ADDR) { > + if (add_free_nid(nm_i, start_nid) < 0) > + break; > + } > } > } > > @@ -1655,7 +1659,7 @@ flush_now: > } > > if (nat_get_blkaddr(ne) == NULL_ADDR && > - !add_free_nid(NM_I(sbi), nid)) { > + add_free_nid(NM_I(sbi), nid) <= 0) { > write_lock(&nm_i->nat_tree_lock); > __del_from_nat_cache(nm_i, ne); > write_unlock(&nm_i->nat_tree_lock); > -- > 1.8.1.3.566.gaa39828 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] f2fs: optimize build_free_nids() 2013-05-06 15:15 [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management Haicheng Li ` (2 preceding siblings ...) 2013-05-06 15:15 ` [PATCH 3/4] f2fs: optimize scan_nat_page() Haicheng Li @ 2013-05-06 15:15 ` Haicheng Li 2013-05-07 10:33 ` Jaegeuk Kim 3 siblings, 1 reply; 11+ messages in thread From: Haicheng Li @ 2013-05-06 15:15 UTC (permalink / raw) To: linux-fsdevel, linux-f2fs-devel, Jaegeuk Kim Cc: linux-kernel, Haicheng Li, Haicheng Li When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT pages. Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> --- fs/f2fs/node.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 1fe3fe2..3136224 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -1342,6 +1342,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi) if (nid >= nm_i->max_nid) nid = 0; + if (nm_i->fcnt > 2 * MAX_FREE_NIDS) + break; if (i++ == FREE_NID_PAGES) break; } -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] f2fs: optimize build_free_nids() 2013-05-06 15:15 ` [PATCH 4/4] f2fs: optimize build_free_nids() Haicheng Li @ 2013-05-07 10:33 ` Jaegeuk Kim 2013-05-08 6:24 ` Haicheng Li 0 siblings, 1 reply; 11+ messages in thread From: Jaegeuk Kim @ 2013-05-07 10:33 UTC (permalink / raw) To: Haicheng Li; +Cc: linux-fsdevel, linux-f2fs-devel, linux-kernel, Haicheng Li [-- Attachment #1: Type: text/plain, Size: 861 bytes --] Hi, 2013-05-06 (월), 23:15 +0800, Haicheng Li: > When nm_i->fcnt > 2 * MAX_FREE_NIDS, stop scanning other NAT pages. > > Signed-off-by: Haicheng Li <haicheng.li@linux.intel.com> > --- > fs/f2fs/node.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > index 1fe3fe2..3136224 100644 > --- a/fs/f2fs/node.c > +++ b/fs/f2fs/node.c > @@ -1342,6 +1342,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi) > if (nid >= nm_i->max_nid) > nid = 0; > > + if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > + break; Could you explain when this can happen? IMO, this is an unnecessary condition check, since the below condition that includes FREE_NID_PAGES already limits the number of free nids. Thanks, > if (i++ == FREE_NID_PAGES) > break; > } -- Jaegeuk Kim Samsung [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] f2fs: optimize build_free_nids() 2013-05-07 10:33 ` Jaegeuk Kim @ 2013-05-08 6:24 ` Haicheng Li 2013-05-08 9:50 ` Jaegeuk Kim 0 siblings, 1 reply; 11+ messages in thread From: Haicheng Li @ 2013-05-08 6:24 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, Haicheng Li, linux-fsdevel, linux-f2fs-devel On Tue, May 07, 2013 at 07:33:59PM +0900, Jaegeuk Kim wrote: > 2013-05-06 (월), 23:15 +0800, Haicheng Li: > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > > index 1fe3fe2..3136224 100644 > > --- a/fs/f2fs/node.c > > +++ b/fs/f2fs/node.c > > @@ -1342,6 +1342,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi) > > if (nid >= nm_i->max_nid) > > nid = 0; > > > > + if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > > + break; > > Could you explain when this can happen? I'm thinking of this possible scenario: as we don't hold any spinlock to protect the context, add_free_nid() could be called by other thread anytime, e.g. by the gc_thread_func() in background. then nm_i->fcnt could be increased as 2 * MAX_FREE_NIDS while i < FREE_NID_PAGES. Anything I misconsidered? > IMO, this is an unnecessary condition check, since the below condition > that includes FREE_NID_PAGES already limits the number of free nids. > Thanks, hmm, the pros is that this check may possibly avoid some (< 4) unnecessary while-loop, the cons is that too many checks of (nm_i->fcnt > 2 * MAX_FREE_NIDS) would make the code looking messy and fragmentary... > > if (i++ == FREE_NID_PAGES) > > break; > > } > > -- > Jaegeuk Kim > Samsung -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] f2fs: optimize build_free_nids() 2013-05-08 6:24 ` Haicheng Li @ 2013-05-08 9:50 ` Jaegeuk Kim 2013-05-08 11:50 ` Haicheng Li 0 siblings, 1 reply; 11+ messages in thread From: Jaegeuk Kim @ 2013-05-08 9:50 UTC (permalink / raw) To: Haicheng Li; +Cc: linux-kernel, Haicheng Li, linux-fsdevel, linux-f2fs-devel [-- Attachment #1: Type: text/plain, Size: 2148 bytes --] 2013-05-08 (수), 14:24 +0800, Haicheng Li: > On Tue, May 07, 2013 at 07:33:59PM +0900, Jaegeuk Kim wrote: > > 2013-05-06 (월), 23:15 +0800, Haicheng Li: > > > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c > > > index 1fe3fe2..3136224 100644 > > > --- a/fs/f2fs/node.c > > > +++ b/fs/f2fs/node.c > > > @@ -1342,6 +1342,8 @@ static void build_free_nids(struct f2fs_sb_info *sbi) > > > if (nid >= nm_i->max_nid) > > > nid = 0; > > > > > > + if (nm_i->fcnt > 2 * MAX_FREE_NIDS) > > > + break; > > > > Could you explain when this can happen? > > I'm thinking of this possible scenario: > > as we don't hold any spinlock to protect the context, add_free_nid() could be > called by other thread anytime, e.g. by the gc_thread_func() in background. The gc_thread_func() is not a proper example here though, the buid_free_nids() is covered by nm_i->build_lock, so build_free_nids is entered only one at a time. In addtion, build_free_nids starts with checking if (nm_i->fcnt > NAT_ENTRY_PER_BLOCK) in order not to be conducted repeatedely. > > then nm_i->fcnt could be increased as 2 * MAX_FREE_NIDS while i < FREE_NID_PAGES. > Anything I misconsidered? Apart from the correctness of this behavior, I'm not sure why we should strictly manage this threshold value. Should we really need to do this? > > > IMO, this is an unnecessary condition check, since the below condition > > that includes FREE_NID_PAGES already limits the number of free nids. > > Thanks, > > hmm, the pros is that this check may possibly avoid some (< 4) unnecessary while-loop, > the cons is that too many checks of (nm_i->fcnt > 2 * MAX_FREE_NIDS) > would make the code looking messy and fragmentary... > > > > if (i++ == FREE_NID_PAGES) > > > break; > > > } > > > > -- > > Jaegeuk Kim > > Samsung > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- Jaegeuk Kim Samsung [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 836 bytes --] ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 4/4] f2fs: optimize build_free_nids() 2013-05-08 9:50 ` Jaegeuk Kim @ 2013-05-08 11:50 ` Haicheng Li 0 siblings, 0 replies; 11+ messages in thread From: Haicheng Li @ 2013-05-08 11:50 UTC (permalink / raw) To: Jaegeuk Kim; +Cc: linux-kernel, Haicheng Li, linux-fsdevel, linux-f2fs-devel On Wed, May 08, 2013 at 06:50:04PM +0900, Jaegeuk Kim wrote: > > Could you explain when this can happen? > > > > I'm thinking of this possible scenario: > > > > as we don't hold any spinlock to protect the context, add_free_nid() could be > > called by other thread anytime, e.g. by the gc_thread_func() in background. > > The gc_thread_func() is not a proper example here though, the > buid_free_nids() is covered by nm_i->build_lock, so build_free_nids is > entered only one at a time. > In addtion, build_free_nids starts with checking if (nm_i->fcnt > > NAT_ENTRY_PER_BLOCK) in order not to be conducted repeatedely. surely build_free_nids() itself is under well protection. but this scenario would happen when gc_thread_func() is running in background: f2fs_gc() write_checkpoint() flush_nat_entries() add_free_nid() > > > > then nm_i->fcnt could be increased as 2 * MAX_FREE_NIDS while i < FREE_NID_PAGES. > > Anything I misconsidered? > > Apart from the correctness of this behavior, I'm not sure why we should > strictly manage this threshold value. > Should we really need to do this? This threshold value itself should have already be well managed in current code. This patch is just to avoid unecessary while-loop that tries scan_nat_page() even when this threshold has already been reached. But as I mentioned previously, it just possibly avoids "< 4" unecessary tries. So this patch now becomes a very very trivial optimization because scan_nat_page() itself can detect out the condition. In such case, You can *ignore* this patch:). Thanks for the patch review, Jaegeuk! > > > > hmm, the pros is that this check may possibly avoid some (< 4) unnecessary while-loop, > > the cons is that too many checks of (nm_i->fcnt > 2 * MAX_FREE_NIDS) > > would make the code looking messy and fragmentary... > > > > > > if (i++ == FREE_NID_PAGES) > > > > break; > > > > } -haicheng ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-05-08 11:50 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-06 15:15 [PATCH V2 0/4] f2fs: various optimization & bugfixing for node management Haicheng Li 2013-05-06 15:15 ` [PATCH 1/4] f2fs: bugfix for alloc_nid_failed() Haicheng Li 2013-05-06 15:15 ` [PATCH 2/4] f2fs: code cleanup for scan_nat_page() and build_free_nids() Haicheng Li 2013-05-06 15:15 ` [PATCH 3/4] f2fs: optimize scan_nat_page() Haicheng Li 2013-05-07 10:36 ` Jaegeuk Kim 2013-05-08 5:31 ` Haicheng Li 2013-05-06 15:15 ` [PATCH 4/4] f2fs: optimize build_free_nids() Haicheng Li 2013-05-07 10:33 ` Jaegeuk Kim 2013-05-08 6:24 ` Haicheng Li 2013-05-08 9:50 ` Jaegeuk Kim 2013-05-08 11:50 ` Haicheng Li
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).