Re: [PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Chao Yu]

On 2017/11/10 1:51, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 11/07, Chao Yu wrote:
>> We will keep __add_ino_entry success all the time, for ENOMEM failure
>> case, we have already handled it by using  __GFP_NOFAIL flag, so we
>> don't have to use additional opened loop codes here, remove them.
>>
>> Signed-off-by: Chao Yu 
>> ---
>> v2:
>> As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
>> memory allocation including access memory reserves, so it's better to
>> use it rather than opened loop code, update this patch for it.
>>  fs/f2fs/checkpoint.c | 19 +++
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 98777c1ae70c..80b90146cff2 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
>> nid_t ino,
>>  {
>>  struct inode_management *im = >im[type];
>>  struct ino_entry *e, *tmp;
>> +bool preloaded;
>> +int err;
>>  
>>  tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
>> -retry:
>> -radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +
>> +preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +f2fs_bug_on(sbi, !preloaded);
> 
> I don't think we need this by adding new useless variable.

Yup, let me update it.

> 
>>  
>>  spin_lock(>ino_lock);
>>  e = radix_tree_lookup(>ino_root, ino);
>>  if (!e) {
>>  e = tmp;
>> -if (radix_tree_insert(>ino_root, ino, e)) {
> 
> How about just adding this?
> 
>   if (unlikely(radix_tree_insert(>ino_root, ino, e)))
>   f2fs_bug_on(sbi, 1);

Okay.

Thanks,

> 
>> -spin_unlock(>ino_lock);
>> -radix_tree_preload_end();
>> -goto retry;
>> -}
>> +err = radix_tree_insert(>ino_root, ino, e);
>> +f2fs_bug_on(sbi, err);
>> +
>>  memset(e, 0, sizeof(struct ino_entry));
>>  e->ino = ino;
>>  
>> @@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
>> nid_t ino,
>>  f2fs_set_bit(devidx, (char *)>dirty_device);
>>  
>>  spin_unlock(>ino_lock);
>> -radix_tree_preload_end();
>> +
>> +if (preloaded)
>> +radix_tree_preload_end();
>>  
>>  if (e != tmp)
>>  kmem_cache_free(ino_entry_slab, tmp);
>> -- 
>> 2.15.0.55.gc2ece9dc4de6
> 
> .
> 

Re: [PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Chao Yu]

On 2017/11/10 1:51, Jaegeuk Kim wrote:
> Hi Chao,
> 
> On 11/07, Chao Yu wrote:
>> We will keep __add_ino_entry success all the time, for ENOMEM failure
>> case, we have already handled it by using  __GFP_NOFAIL flag, so we
>> don't have to use additional opened loop codes here, remove them.
>>
>> Signed-off-by: Chao Yu 
>> ---
>> v2:
>> As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
>> memory allocation including access memory reserves, so it's better to
>> use it rather than opened loop code, update this patch for it.
>>  fs/f2fs/checkpoint.c | 19 +++
>>  1 file changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 98777c1ae70c..80b90146cff2 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
>> nid_t ino,
>>  {
>>  struct inode_management *im = >im[type];
>>  struct ino_entry *e, *tmp;
>> +bool preloaded;
>> +int err;
>>  
>>  tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
>> -retry:
>> -radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +
>> +preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +f2fs_bug_on(sbi, !preloaded);
> 
> I don't think we need this by adding new useless variable.

Yup, let me update it.

> 
>>  
>>  spin_lock(>ino_lock);
>>  e = radix_tree_lookup(>ino_root, ino);
>>  if (!e) {
>>  e = tmp;
>> -if (radix_tree_insert(>ino_root, ino, e)) {
> 
> How about just adding this?
> 
>   if (unlikely(radix_tree_insert(>ino_root, ino, e)))
>   f2fs_bug_on(sbi, 1);

Okay.

Thanks,

> 
>> -spin_unlock(>ino_lock);
>> -radix_tree_preload_end();
>> -goto retry;
>> -}
>> +err = radix_tree_insert(>ino_root, ino, e);
>> +f2fs_bug_on(sbi, err);
>> +
>>  memset(e, 0, sizeof(struct ino_entry));
>>  e->ino = ino;
>>  
>> @@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
>> nid_t ino,
>>  f2fs_set_bit(devidx, (char *)>dirty_device);
>>  
>>  spin_unlock(>ino_lock);
>> -radix_tree_preload_end();
>> +
>> +if (preloaded)
>> +radix_tree_preload_end();
>>  
>>  if (e != tmp)
>>  kmem_cache_free(ino_entry_slab, tmp);
>> -- 
>> 2.15.0.55.gc2ece9dc4de6
> 
> .
> 

Re: [PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Jaegeuk Kim]

Hi Chao,

On 11/07, Chao Yu wrote:
> We will keep __add_ino_entry success all the time, for ENOMEM failure
> case, we have already handled it by using  __GFP_NOFAIL flag, so we
> don't have to use additional opened loop codes here, remove them.
> 
> Signed-off-by: Chao Yu 
> ---
> v2:
> As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
> memory allocation including access memory reserves, so it's better to
> use it rather than opened loop code, update this patch for it.
>  fs/f2fs/checkpoint.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 98777c1ae70c..80b90146cff2 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
> nid_t ino,
>  {
>   struct inode_management *im = >im[type];
>   struct ino_entry *e, *tmp;
> + bool preloaded;
> + int err;
>  
>   tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
> -retry:
> - radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
> +
> + preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
> + f2fs_bug_on(sbi, !preloaded);

I don't think we need this by adding new useless variable.

>  
>   spin_lock(>ino_lock);
>   e = radix_tree_lookup(>ino_root, ino);
>   if (!e) {
>   e = tmp;
> - if (radix_tree_insert(>ino_root, ino, e)) {

How about just adding this?

if (unlikely(radix_tree_insert(>ino_root, ino, e)))
f2fs_bug_on(sbi, 1);

> - spin_unlock(>ino_lock);
> - radix_tree_preload_end();
> - goto retry;
> - }
> + err = radix_tree_insert(>ino_root, ino, e);
> + f2fs_bug_on(sbi, err);
> +
>   memset(e, 0, sizeof(struct ino_entry));
>   e->ino = ino;
>  
> @@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
> nid_t ino,
>   f2fs_set_bit(devidx, (char *)>dirty_device);
>  
>   spin_unlock(>ino_lock);
> - radix_tree_preload_end();
> +
> + if (preloaded)
> + radix_tree_preload_end();
>  
>   if (e != tmp)
>   kmem_cache_free(ino_entry_slab, tmp);
> -- 
> 2.15.0.55.gc2ece9dc4de6

Re: [PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Jaegeuk Kim]

Hi Chao,

On 11/07, Chao Yu wrote:
> We will keep __add_ino_entry success all the time, for ENOMEM failure
> case, we have already handled it by using  __GFP_NOFAIL flag, so we
> don't have to use additional opened loop codes here, remove them.
> 
> Signed-off-by: Chao Yu 
> ---
> v2:
> As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
> memory allocation including access memory reserves, so it's better to
> use it rather than opened loop code, update this patch for it.
>  fs/f2fs/checkpoint.c | 19 +++
>  1 file changed, 11 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 98777c1ae70c..80b90146cff2 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
> nid_t ino,
>  {
>   struct inode_management *im = >im[type];
>   struct ino_entry *e, *tmp;
> + bool preloaded;
> + int err;
>  
>   tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
> -retry:
> - radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
> +
> + preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
> + f2fs_bug_on(sbi, !preloaded);

I don't think we need this by adding new useless variable.

>  
>   spin_lock(>ino_lock);
>   e = radix_tree_lookup(>ino_root, ino);
>   if (!e) {
>   e = tmp;
> - if (radix_tree_insert(>ino_root, ino, e)) {

How about just adding this?

if (unlikely(radix_tree_insert(>ino_root, ino, e)))
f2fs_bug_on(sbi, 1);

> - spin_unlock(>ino_lock);
> - radix_tree_preload_end();
> - goto retry;
> - }
> + err = radix_tree_insert(>ino_root, ino, e);
> + f2fs_bug_on(sbi, err);
> +
>   memset(e, 0, sizeof(struct ino_entry));
>   e->ino = ino;
>  
> @@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
> nid_t ino,
>   f2fs_set_bit(devidx, (char *)>dirty_device);
>  
>   spin_unlock(>ino_lock);
> - radix_tree_preload_end();
> +
> + if (preloaded)
> + radix_tree_preload_end();
>  
>   if (e != tmp)
>   kmem_cache_free(ino_entry_slab, tmp);
> -- 
> 2.15.0.55.gc2ece9dc4de6

[PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Chao Yu]

We will keep __add_ino_entry success all the time, for ENOMEM failure
case, we have already handled it by using  __GFP_NOFAIL flag, so we
don't have to use additional opened loop codes here, remove them.

Signed-off-by: Chao Yu 
---
v2:
As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
memory allocation including access memory reserves, so it's better to
use it rather than opened loop code, update this patch for it.
 fs/f2fs/checkpoint.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 98777c1ae70c..80b90146cff2 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
nid_t ino,
 {
struct inode_management *im = >im[type];
struct ino_entry *e, *tmp;
+   bool preloaded;
+   int err;
 
tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
-retry:
-   radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
+
+   preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
+   f2fs_bug_on(sbi, !preloaded);
 
spin_lock(>ino_lock);
e = radix_tree_lookup(>ino_root, ino);
if (!e) {
e = tmp;
-   if (radix_tree_insert(>ino_root, ino, e)) {
-   spin_unlock(>ino_lock);
-   radix_tree_preload_end();
-   goto retry;
-   }
+   err = radix_tree_insert(>ino_root, ino, e);
+   f2fs_bug_on(sbi, err);
+
memset(e, 0, sizeof(struct ino_entry));
e->ino = ino;
 
@@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t 
ino,
f2fs_set_bit(devidx, (char *)>dirty_device);
 
spin_unlock(>ino_lock);
-   radix_tree_preload_end();
+
+   if (preloaded)
+   radix_tree_preload_end();
 
if (e != tmp)
kmem_cache_free(ino_entry_slab, tmp);
-- 
2.15.0.55.gc2ece9dc4de6

[PATCH v2 1/3] f2fs: avoid opened loop codes in __add_ino_entry

[Chao Yu]

We will keep __add_ino_entry success all the time, for ENOMEM failure
case, we have already handled it by using  __GFP_NOFAIL flag, so we
don't have to use additional opened loop codes here, remove them.

Signed-off-by: Chao Yu 
---
v2:
As Michal Hocko suggested, with __GFP_NOFAIL, MM will do all it can for
memory allocation including access memory reserves, so it's better to
use it rather than opened loop code, update this patch for it.
 fs/f2fs/checkpoint.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 98777c1ae70c..80b90146cff2 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -405,20 +405,21 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, 
nid_t ino,
 {
struct inode_management *im = >im[type];
struct ino_entry *e, *tmp;
+   bool preloaded;
+   int err;
 
tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
-retry:
-   radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
+
+   preloaded = !radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
+   f2fs_bug_on(sbi, !preloaded);
 
spin_lock(>ino_lock);
e = radix_tree_lookup(>ino_root, ino);
if (!e) {
e = tmp;
-   if (radix_tree_insert(>ino_root, ino, e)) {
-   spin_unlock(>ino_lock);
-   radix_tree_preload_end();
-   goto retry;
-   }
+   err = radix_tree_insert(>ino_root, ino, e);
+   f2fs_bug_on(sbi, err);
+
memset(e, 0, sizeof(struct ino_entry));
e->ino = ino;
 
@@ -431,7 +432,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t 
ino,
f2fs_set_bit(devidx, (char *)>dirty_device);
 
spin_unlock(>ino_lock);
-   radix_tree_preload_end();
+
+   if (preloaded)
+   radix_tree_preload_end();
 
if (e != tmp)
kmem_cache_free(ino_entry_slab, tmp);
-- 
2.15.0.55.gc2ece9dc4de6