[PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Yunlei He]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Chao Yu]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Jaegeuk Kim]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[heyunlei]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Jaegeuk Kim]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Chao Yu]

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

Re: [PATCH v5] f2fs: Separate nat entry mem alloc from nat_tree_locks

[Jaegeuk Kim]

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