[PATCH 1/3] f2fs: guarantee node/meta inode number won't be reused

[PATCH 1/3] f2fs: guarantee node/meta inode number won't be reused

[Wanpeng Li]

Node/Meta inode numbers are also should not be reused, this patch
guarantee it.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 fs/f2fs/node.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 4687eae..1a7e925 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1421,7 +1421,7 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
 		return -1;
 
 	/* 0 nid should not be used */
-	if (unlikely(nid == 0))
+	if (unlikely(nid == 0 || nid == 1 || nid == 2))
 		return 0;
 
 	if (build) {
-- 
1.9.1

[PATCH 2/3] f2fs: fix get stale meta pages when build free nids

[Wanpeng Li]

The blocks of nat pages to be scanned will be readahead when build free 
nids. The start blkno will be adjusted to 0 when the intended range over 
max_nids, then function ra_meta_pages readahead the right blkno, however, 
function get_current_nat_page which is called in build_free_nids still 
get the meta pages w/ stale blkno, this patch fix it by adjusting the nid 
and the blkno which will be readahead in avdance in order to readahead and 
get the right meta pages.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 fs/f2fs/checkpoint.c | 3 ---
 fs/f2fs/node.c       | 9 ++++++++-
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 53bc328..aedb441 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -129,9 +129,6 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages, int type
 
 		switch (type) {
 		case META_NAT:
-			if (unlikely(blkno >=
-					NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
-				blkno = 0;
 			/* get nat block addr */
 			fio.blk_addr = current_nat_addr(sbi,
 					blkno * NAT_ENTRY_PER_BLOCK);
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 1a7e925..8751375 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1509,13 +1509,20 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
 	struct f2fs_summary_block *sum = curseg->sum_blk;
 	int i = 0;
 	nid_t nid = nm_i->next_scan_nid;
+	block_t blkno = NAT_BLOCK_OFFSET(nid);
 
 	/* Enough entries */
 	if (nm_i->fcnt > NAT_ENTRY_PER_BLOCK)
 		return;
 
+	if (unlikely(blkno >=
+			NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid))) {
+		blkno = 0;
+		nid = 0;
+	}
+
 	/* readahead nat pages to be scanned */
-	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES, META_NAT);
+	ra_meta_pages(sbi, blkno, FREE_NID_PAGES, META_NAT);
 
 	while (1) {
 		struct page *page = get_current_nat_page(sbi, nid);
-- 
1.9.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the 
conversation now. http://goparallel.sourceforge.net/

Re: [PATCH 2/3] f2fs: fix get stale meta pages when build free nids

[Jaegeuk Kim]

Hi Wanpeng,

On Mon, Mar 09, 2015 at 11:00:54AM +0800, Wanpeng Li wrote:
> The blocks of nat pages to be scanned will be readahead when build free 
> nids. The start blkno will be adjusted to 0 when the intended range over 
> max_nids, then function ra_meta_pages readahead the right blkno, however, 
> function get_current_nat_page which is called in build_free_nids still 
> get the meta pages w/ stale blkno, this patch fix it by adjusting the nid 
> and the blkno which will be readahead in avdance in order to readahead and 
> get the right meta pages.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
>  fs/f2fs/checkpoint.c | 3 ---
>  fs/f2fs/node.c       | 9 ++++++++-
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 53bc328..aedb441 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -129,9 +129,6 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages, int type
>  
>  		switch (type) {
>  		case META_NAT:
> -			if (unlikely(blkno >=
> -					NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
> -				blkno = 0;
>  			/* get nat block addr */
>  			fio.blk_addr = current_nat_addr(sbi,
>  					blkno * NAT_ENTRY_PER_BLOCK);
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 1a7e925..8751375 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1509,13 +1509,20 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
>  	struct f2fs_summary_block *sum = curseg->sum_blk;
>  	int i = 0;
>  	nid_t nid = nm_i->next_scan_nid;
> +	block_t blkno = NAT_BLOCK_OFFSET(nid);
>  
>  	/* Enough entries */
>  	if (nm_i->fcnt > NAT_ENTRY_PER_BLOCK)
>  		return;
>  
> +	if (unlikely(blkno >=
> +			NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid))) {
> +		blkno = 0;
> +		nid = 0;
> +	}

The assumption here is that the nid does not exceeed max_nid, since the
previous call already set next_scan_nid to 0, if it exceeds max_nid.
So, we don't need to do like this.

BTW, even if ra_meta_pages doesn't get the below condition,
		if (unlikely(blkno >=
				NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
			blkno = 0;
IMO, it is still worth to remain that to avoid any bugs due to further code
changes. It makes sense that unlikely was used for this too.

Thanks,

> +
>  	/* readahead nat pages to be scanned */
> -	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES, META_NAT);
> +	ra_meta_pages(sbi, blkno, FREE_NID_PAGES, META_NAT);
>  
>  	while (1) {
>  		struct page *page = get_current_nat_page(sbi, nid);
> -- 
> 1.9.1

Re: [PATCH 2/3] f2fs: fix get stale meta pages when build free nids

[Wanpeng Li]

Hi Jaegeuk,
On Sun, Mar 08, 2015 at 09:18:47PM -0700, Jaegeuk Kim wrote:
>Hi Wanpeng,
>
>On Mon, Mar 09, 2015 at 11:00:54AM +0800, Wanpeng Li wrote:
>> The blocks of nat pages to be scanned will be readahead when build free 
>> nids. The start blkno will be adjusted to 0 when the intended range over 
>> max_nids, then function ra_meta_pages readahead the right blkno, however, 
>> function get_current_nat_page which is called in build_free_nids still 
>> get the meta pages w/ stale blkno, this patch fix it by adjusting the nid 
>> and the blkno which will be readahead in avdance in order to readahead and 
>> get the right meta pages.
>> 
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>> ---
>>  fs/f2fs/checkpoint.c | 3 ---
>>  fs/f2fs/node.c       | 9 ++++++++-
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>> 
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 53bc328..aedb441 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -129,9 +129,6 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages, int type
>>  
>>  		switch (type) {
>>  		case META_NAT:
>> -			if (unlikely(blkno >=
>> -					NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
>> -				blkno = 0;
>>  			/* get nat block addr */
>>  			fio.blk_addr = current_nat_addr(sbi,
>>  					blkno * NAT_ENTRY_PER_BLOCK);
>> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
>> index 1a7e925..8751375 100644
>> --- a/fs/f2fs/node.c
>> +++ b/fs/f2fs/node.c
>> @@ -1509,13 +1509,20 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
>>  	struct f2fs_summary_block *sum = curseg->sum_blk;
>>  	int i = 0;
>>  	nid_t nid = nm_i->next_scan_nid;
>> +	block_t blkno = NAT_BLOCK_OFFSET(nid);
>>  
>>  	/* Enough entries */
>>  	if (nm_i->fcnt > NAT_ENTRY_PER_BLOCK)
>>  		return;
>>  
>> +	if (unlikely(blkno >=
>> +			NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid))) {
>> +		blkno = 0;
>> +		nid = 0;
>> +	}
>
>The assumption here is that the nid does not exceeed max_nid, since the
>previous call already set next_scan_nid to 0, if it exceeds max_nid.
>So, we don't need to do like this.

So get_current_nat_page in while(1) still get the wrong meta page for
one time, right? The nid will be set to 0 in the second round.

Regards,
Wanpeng Li 

>
>BTW, even if ra_meta_pages doesn't get the below condition,
>		if (unlikely(blkno >=
>				NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
>			blkno = 0;
>IMO, it is still worth to remain that to avoid any bugs due to further code
>changes. It makes sense that unlikely was used for this too.
>
>Thanks,
>
>> +
>>  	/* readahead nat pages to be scanned */
>> -	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES, META_NAT);
>> +	ra_meta_pages(sbi, blkno, FREE_NID_PAGES, META_NAT);
>>  
>>  	while (1) {
>>  		struct page *page = get_current_nat_page(sbi, nid);
>> -- 
>> 1.9.1

RE: [PATCH 2/3] f2fs: fix get stale meta pages when build free nids

[Chao Yu]

Hi Wanpeng,

> -----Original Message-----
> From: Wanpeng Li [mailto:wanpeng.li@linux.intel.com]
> Sent: Monday, March 09, 2015 12:25 PM
> To: Jaegeuk Kim
> Cc: Changman Lee; Chao Yu; linux-f2fs-devel@lists.sourceforge.net;
> linux-fsdevel@vger.kernel.org; linux-kernel@vger.kernel.org; Wanpeng Li
> Subject: Re: [PATCH 2/3] f2fs: fix get stale meta pages when build free nids
> 
> Hi Jaegeuk,
> On Sun, Mar 08, 2015 at 09:18:47PM -0700, Jaegeuk Kim wrote:
> >Hi Wanpeng,
> >
> >On Mon, Mar 09, 2015 at 11:00:54AM +0800, Wanpeng Li wrote:
> >> The blocks of nat pages to be scanned will be readahead when build free
> >> nids. The start blkno will be adjusted to 0 when the intended range over
> >> max_nids, then function ra_meta_pages readahead the right blkno, however,
> >> function get_current_nat_page which is called in build_free_nids still
> >> get the meta pages w/ stale blkno, this patch fix it by adjusting the nid
> >> and the blkno which will be readahead in avdance in order to readahead and
> >> get the right meta pages.
> >>
> >> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> >> ---
> >>  fs/f2fs/checkpoint.c | 3 ---
> >>  fs/f2fs/node.c       | 9 ++++++++-
> >>  2 files changed, 8 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> >> index 53bc328..aedb441 100644
> >> --- a/fs/f2fs/checkpoint.c
> >> +++ b/fs/f2fs/checkpoint.c
> >> @@ -129,9 +129,6 @@ int ra_meta_pages(struct f2fs_sb_info *sbi, block_t start, int nrpages,
> int type
> >>
> >>  		switch (type) {
> >>  		case META_NAT:
> >> -			if (unlikely(blkno >=
> >> -					NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
> >> -				blkno = 0;
> >>  			/* get nat block addr */
> >>  			fio.blk_addr = current_nat_addr(sbi,
> >>  					blkno * NAT_ENTRY_PER_BLOCK);
> >> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> >> index 1a7e925..8751375 100644
> >> --- a/fs/f2fs/node.c
> >> +++ b/fs/f2fs/node.c
> >> @@ -1509,13 +1509,20 @@ static void build_free_nids(struct f2fs_sb_info *sbi)
> >>  	struct f2fs_summary_block *sum = curseg->sum_blk;
> >>  	int i = 0;
> >>  	nid_t nid = nm_i->next_scan_nid;
> >> +	block_t blkno = NAT_BLOCK_OFFSET(nid);
> >>
> >>  	/* Enough entries */
> >>  	if (nm_i->fcnt > NAT_ENTRY_PER_BLOCK)
> >>  		return;
> >>
> >> +	if (unlikely(blkno >=
> >> +			NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid))) {
> >> +		blkno = 0;
> >> +		nid = 0;
> >> +	}
> >
> >The assumption here is that the nid does not exceeed max_nid, since the
> >previous call already set next_scan_nid to 0, if it exceeds max_nid.
> >So, we don't need to do like this.
> 
> So get_current_nat_page in while(1) still get the wrong meta page for
> one time, right? The nid will be set to 0 in the second round.

When we break while (1), our nid will not exceed max_nid because:
	while (1)
		...
		if (unlikely(nid >= nm_i->max_nid))
			nid = 0;

then next_scan_nid is assigned nid, for next round in build_free_nids
our nid which is assigned next_scan_nid will also not exceed max_nid.

So get_current_nat_page will get the right meta page.

Thanks,

> 
> Regards,
> Wanpeng Li
> 
> >
> >BTW, even if ra_meta_pages doesn't get the below condition,
> >		if (unlikely(blkno >=
> >				NAT_BLOCK_OFFSET(NM_I(sbi)->max_nid)))
> >			blkno = 0;
> >IMO, it is still worth to remain that to avoid any bugs due to further code
> >changes. It makes sense that unlikely was used for this too.
> >
> >Thanks,
> >
> >> +
> >>  	/* readahead nat pages to be scanned */
> >> -	ra_meta_pages(sbi, NAT_BLOCK_OFFSET(nid), FREE_NID_PAGES, META_NAT);
> >> +	ra_meta_pages(sbi, blkno, FREE_NID_PAGES, META_NAT);
> >>
> >>  	while (1) {
> >>  		struct page *page = get_current_nat_page(sbi, nid);
> >> --
> >> 1.9.1

[PATCH 3/3] f2fs: fix unlocked nat set cache operation

[Wanpeng Li]

nm_i->nat_tree_lock is used to sync both the operations of nat entry
cache tree and nat set cache tree, however, it isn't held when flush 
nat entries during checkpoint which lead to potential race, this patch 
fix it by holding the lock when gang lookup nat set cache and delete 
item from nat set cache.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 fs/f2fs/node.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 8751375..14e4387 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1837,6 +1837,7 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 	struct f2fs_nat_block *nat_blk;
 	struct nat_entry *ne, *cur;
 	struct page *page = NULL;
+	struct f2fs_nm_info *nm_i = NM_I(sbi);
 
 	/*
 	 * there are two steps to flush nat entries:
@@ -1890,7 +1891,9 @@ static void __flush_nat_entry_set(struct f2fs_sb_info *sbi,
 
 	f2fs_bug_on(sbi, set->entry_cnt);
 
+	down_write(&nm_i->nat_tree_lock);
 	radix_tree_delete(&NM_I(sbi)->nat_set_root, set->set);
+	up_write(&nm_i->nat_tree_lock);
 	kmem_cache_free(nat_entry_set_slab, set);
 }
 
@@ -1918,6 +1921,7 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 	if (!__has_cursum_space(sum, nm_i->dirty_nat_cnt, NAT_JOURNAL))
 		remove_nats_in_journal(sbi);
 
+	down_write(&nm_i->nat_tree_lock);
 	while ((found = __gang_lookup_nat_set(nm_i,
 					set_idx, SETVEC_SIZE, setvec))) {
 		unsigned idx;
@@ -1926,6 +1930,7 @@ void flush_nat_entries(struct f2fs_sb_info *sbi)
 			__adjust_nat_entry_set(setvec[idx], &sets,
 							MAX_NAT_JENTRIES(sum));
 	}
+	up_write(&nm_i->nat_tree_lock);
 
 	/* flush dirty nats in nat entry set */
 	list_for_each_entry_safe(set, tmp, &sets, set_list)
-- 
1.9.1


------------------------------------------------------------------------------
Dive into the World of Parallel Programming The Go Parallel Website, sponsored
by Intel and developed in partnership with Slashdot Media, is your hub for all
things parallel software development, from weekly thought leadership blogs to
news, videos, case studies, tutorials and more. Take a look and join the 
conversation now. http://goparallel.sourceforge.net/

Re: [PATCH 1/3] f2fs: guarantee node/meta inode number won't be reused

[Jaegeuk Kim]

Hi Wanpeng,

On Mon, Mar 09, 2015 at 11:00:53AM +0800, Wanpeng Li wrote:
> Node/Meta inode numbers are also should not be reused, this patch
> guarantee it.

We don't need to do this, since the mkfs.f2fs assigns block addresses as 1
for node and meta nids in their nat entries.
So, this should not happen.
Otherwise, we should fix any bugs in nat operations.

Thanks,

> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
>  fs/f2fs/node.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index 4687eae..1a7e925 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1421,7 +1421,7 @@ static int add_free_nid(struct f2fs_sb_info *sbi, nid_t nid, bool build)
>  		return -1;
>  
>  	/* 0 nid should not be used */
> -	if (unlikely(nid == 0))
> +	if (unlikely(nid == 0 || nid == 1 || nid == 2))
>  		return 0;
>  
>  	if (build) {
> -- 
> 1.9.1