[PATCH V2 0/4] f2fs: various optimization & bugfixing for node management

[PATCH V2 0/4] f2fs: various optimization & bugfixing for node management

[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

[PATCH 1/4] f2fs: bugfix for alloc_nid_failed()

[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

[PATCH 2/4] f2fs: code cleanup for scan_nat_page() and build_free_nids()

[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

[PATCH 3/4] f2fs: optimize scan_nat_page()

[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

[PATCH 4/4] f2fs: optimize build_free_nids()

[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

Re: [PATCH 4/4] f2fs: optimize build_free_nids()

[Jaegeuk Kim]

[-- 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 --]

Re: [PATCH 3/4] f2fs: optimize scan_nat_page()

[Jaegeuk Kim]

[-- 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 --]

Re: [PATCH 3/4] f2fs: optimize scan_nat_page()

[Haicheng Li]

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
> 

Re: [PATCH 4/4] f2fs: optimize build_free_nids()

[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.

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

Re: [PATCH 4/4] f2fs: optimize build_free_nids()

[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.

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

Re: [PATCH 4/4] f2fs: optimize build_free_nids()

[Jaegeuk Kim]

[-- 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 --]

Re: [PATCH 4/4] f2fs: optimize build_free_nids()

[Haicheng Li]

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