[PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

We will keep __add_ino_entry success all the time, for ENOMEM failure
case, we have already handled it with an opened loop code, so we don't
have to use redundant __GFP_NOFAIL in radix_tree_preload, remove it.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/checkpoint.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 98777c1ae70c..43ee9d97fd8f 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -405,10 +405,11 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
 {
 	struct inode_management *im = &sbi->im[type];
 	struct ino_entry *e, *tmp;
+	bool preloaded;
 
 	tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
 retry:
-	radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
+	preloaded = !radix_tree_preload(GFP_NOFS);
 
 	spin_lock(&im->ino_lock);
 	e = radix_tree_lookup(&im->ino_root, ino);
@@ -416,7 +417,8 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
 		e = tmp;
 		if (radix_tree_insert(&im->ino_root, ino, e)) {
 			spin_unlock(&im->ino_lock);
-			radix_tree_preload_end();
+			if (preloaded)
+				radix_tree_preload_end();
 			goto retry;
 		}
 		memset(e, 0, sizeof(struct ino_entry));
@@ -431,7 +433,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
 		f2fs_set_bit(devidx, (char *)&e->dirty_device);
 
 	spin_unlock(&im->ino_lock);
-	radix_tree_preload_end();
+
+	if (preloaded)
+		radix_tree_preload_end();
 
 	if (e != tmp)
 		kmem_cache_free(ino_entry_slab, tmp);
-- 
2.14.1.145.gb3622a4ee

[PATCH 2/3] f2fs: trace checkpoint reason in fsync()

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

This patch slightly changes need_do_checkpoint to return the detail
info that indicates why we need do checkpoint, then caller could print
it with trace message.

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/file.c              | 34 ++++++++++++++++++----------------
 include/trace/events/f2fs.h | 12 ++++++------
 2 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cee0f36ba9a1..d9d8b1c372f8 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -141,27 +141,29 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
 	return 1;
 }
 
-static inline bool need_do_checkpoint(struct inode *inode)
+static inline int need_do_checkpoint(struct inode *inode)
 {
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
-	bool need_cp = false;
+	int cp_reason = 0;
 
-	if (!S_ISREG(inode->i_mode) || inode->i_nlink != 1)
-		need_cp = true;
+	if (!S_ISREG(inode->i_mode))
+		cp_reason = 1;
+	else if (inode->i_nlink != 1)
+		cp_reason = 2;
 	else if (is_sbi_flag_set(sbi, SBI_NEED_CP))
-		need_cp = true;
+		cp_reason = 3;
 	else if (file_wrong_pino(inode))
-		need_cp = true;
+		cp_reason = 4;
 	else if (!space_for_roll_forward(sbi))
-		need_cp = true;
+		cp_reason = 5;
 	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
-		need_cp = true;
+		cp_reason = 6;
 	else if (test_opt(sbi, FASTBOOT))
-		need_cp = true;
+		cp_reason = 7;
 	else if (sbi->active_logs == 2)
-		need_cp = true;
+		cp_reason = 8;
 
-	return need_cp;
+	return cp_reason;
 }
 
 static bool need_inode_page_update(struct f2fs_sb_info *sbi, nid_t ino)
@@ -196,7 +198,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
 	nid_t ino = inode->i_ino;
 	int ret = 0;
-	bool need_cp = false;
+	bool cp_reason = 0;
 	struct writeback_control wbc = {
 		.sync_mode = WB_SYNC_ALL,
 		.nr_to_write = LONG_MAX,
@@ -215,7 +217,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	clear_inode_flag(inode, FI_NEED_IPU);
 
 	if (ret) {
-		trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
+		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
 		return ret;
 	}
 
@@ -246,10 +248,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	 * sudden-power-off.
 	 */
 	down_read(&F2FS_I(inode)->i_sem);
-	need_cp = need_do_checkpoint(inode);
+	cp_reason = need_do_checkpoint(inode);
 	up_read(&F2FS_I(inode)->i_sem);
 
-	if (need_cp) {
+	if (cp_reason) {
 		/* all the dirty node pages should be flushed for POR */
 		ret = f2fs_sync_fs(inode->i_sb, 1);
 
@@ -306,7 +308,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	}
 	f2fs_update_time(sbi, REQ_TIME);
 out:
-	trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
+	trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
 	f2fs_trace_ios(NULL, 1);
 	return ret;
 }
diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index 009745564214..f1a317e6db79 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -210,14 +210,14 @@ DEFINE_EVENT(f2fs__inode, f2fs_sync_file_enter,
 
 TRACE_EVENT(f2fs_sync_file_exit,
 
-	TP_PROTO(struct inode *inode, int need_cp, int datasync, int ret),
+	TP_PROTO(struct inode *inode, int cp_reason, int datasync, int ret),
 
-	TP_ARGS(inode, need_cp, datasync, ret),
+	TP_ARGS(inode, cp_reason, datasync, ret),
 
 	TP_STRUCT__entry(
 		__field(dev_t,	dev)
 		__field(ino_t,	ino)
-		__field(int,	need_cp)
+		__field(int,	cp_reason)
 		__field(int,	datasync)
 		__field(int,	ret)
 	),
@@ -225,15 +225,15 @@ TRACE_EVENT(f2fs_sync_file_exit,
 	TP_fast_assign(
 		__entry->dev		= inode->i_sb->s_dev;
 		__entry->ino		= inode->i_ino;
-		__entry->need_cp	= need_cp;
+		__entry->cp_reason	= cp_reason;
 		__entry->datasync	= datasync;
 		__entry->ret		= ret;
 	),
 
-	TP_printk("dev = (%d,%d), ino = %lu, checkpoint is %s, "
+	TP_printk("dev = (%d,%d), ino = %lu, cp_reason:%d, "
 		"datasync = %d, ret = %d",
 		show_dev_ino(__entry),
-		__entry->need_cp ? "needed" : "not needed",
+		__entry->cp_reason,
 		__entry->datasync,
 		__entry->ret)
 );
-- 
2.14.1.145.gb3622a4ee

[PATCH 3/3] f2fs: keep isize once block is reserved cross EOF

[Chao Yu]

From: Chao Yu <yuchao0@huawei.com>

Without FADVISE_KEEP_SIZE_BIT, we will try to recover file size
according to last non-hole block, so in fallocate(), we must set
FADVISE_KEEP_SIZE_BIT flag once we have preallocated block cross
EOF, instead of when all preallocation is success. Otherwise, file
size will be incorrect due to lack of this flag.

Simple testcase to reproduce this:

1. echo 2 > /sys/fs/f2fs/<device>/inject_type
2. echo 10 > /sys/fs/f2fs/<device>/inject_rate
3. run tests/generic/392
4. disable fault injection
5. do remount

Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
 fs/f2fs/file.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d9d8b1c372f8..e67f03546391 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1472,8 +1472,12 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
 		new_size = ((loff_t)pg_end << PAGE_SHIFT) + off_end;
 	}
 
-	if (!(mode & FALLOC_FL_KEEP_SIZE) && i_size_read(inode) < new_size)
-		f2fs_i_size_write(inode, new_size);
+	if (new_size > i_size_read(inode)) {
+		if (mode & FALLOC_FL_KEEP_SIZE)
+			file_set_keep_isize(inode);
+		else
+			f2fs_i_size_write(inode, new_size);
+	}
 
 	return err;
 }
@@ -1520,8 +1524,6 @@ static long f2fs_fallocate(struct file *file, int mode,
 	if (!ret) {
 		inode->i_mtime = inode->i_ctime = current_time(inode);
 		f2fs_mark_inode_dirty_sync(inode, false);
-		if (mode & FALLOC_FL_KEEP_SIZE)
-			file_set_keep_isize(inode);
 		f2fs_update_time(F2FS_I_SB(inode), REQ_TIME);
 	}
 
-- 
2.14.1.145.gb3622a4ee

Re: [PATCH 2/3] f2fs: trace checkpoint reason in fsync()

[Jaegeuk Kim]

On 11/05, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
> 
> This patch slightly changes need_do_checkpoint to return the detail
> info that indicates why we need do checkpoint, then caller could print
> it with trace message.
> 
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/file.c              | 34 ++++++++++++++++++----------------
>  include/trace/events/f2fs.h | 12 ++++++------
>  2 files changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index cee0f36ba9a1..d9d8b1c372f8 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -141,27 +141,29 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>  	return 1;
>  }
>  
> -static inline bool need_do_checkpoint(struct inode *inode)
> +static inline int need_do_checkpoint(struct inode *inode)
>  {
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> -	bool need_cp = false;
> +	int cp_reason = 0;
>  
> -	if (!S_ISREG(inode->i_mode) || inode->i_nlink != 1)
> -		need_cp = true;
> +	if (!S_ISREG(inode->i_mode))
> +		cp_reason = 1;
> +	else if (inode->i_nlink != 1)
> +		cp_reason = 2;
>  	else if (is_sbi_flag_set(sbi, SBI_NEED_CP))
> -		need_cp = true;
> +		cp_reason = 3;
>  	else if (file_wrong_pino(inode))
> -		need_cp = true;
> +		cp_reason = 4;
>  	else if (!space_for_roll_forward(sbi))
> -		need_cp = true;
> +		cp_reason = 5;
>  	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
> -		need_cp = true;
> +		cp_reason = 6;
>  	else if (test_opt(sbi, FASTBOOT))
> -		need_cp = true;
> +		cp_reason = 7;
>  	else if (sbi->active_logs == 2)
> -		need_cp = true;
> +		cp_reason = 8;

We need to make the numbers by enum so that we could interpret them in tracepoint
below.

Thanks,

>  
> -	return need_cp;
> +	return cp_reason;
>  }
>  
>  static bool need_inode_page_update(struct f2fs_sb_info *sbi, nid_t ino)
> @@ -196,7 +198,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>  	nid_t ino = inode->i_ino;
>  	int ret = 0;
> -	bool need_cp = false;
> +	bool cp_reason = 0;
>  	struct writeback_control wbc = {
>  		.sync_mode = WB_SYNC_ALL,
>  		.nr_to_write = LONG_MAX,
> @@ -215,7 +217,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  	clear_inode_flag(inode, FI_NEED_IPU);
>  
>  	if (ret) {
> -		trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
> +		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
>  		return ret;
>  	}
>  
> @@ -246,10 +248,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  	 * sudden-power-off.
>  	 */
>  	down_read(&F2FS_I(inode)->i_sem);
> -	need_cp = need_do_checkpoint(inode);
> +	cp_reason = need_do_checkpoint(inode);
>  	up_read(&F2FS_I(inode)->i_sem);
>  
> -	if (need_cp) {
> +	if (cp_reason) {
>  		/* all the dirty node pages should be flushed for POR */
>  		ret = f2fs_sync_fs(inode->i_sb, 1);
>  
> @@ -306,7 +308,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>  	}
>  	f2fs_update_time(sbi, REQ_TIME);
>  out:
> -	trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
> +	trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
>  	f2fs_trace_ios(NULL, 1);
>  	return ret;
>  }
> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
> index 009745564214..f1a317e6db79 100644
> --- a/include/trace/events/f2fs.h
> +++ b/include/trace/events/f2fs.h
> @@ -210,14 +210,14 @@ DEFINE_EVENT(f2fs__inode, f2fs_sync_file_enter,
>  
>  TRACE_EVENT(f2fs_sync_file_exit,
>  
> -	TP_PROTO(struct inode *inode, int need_cp, int datasync, int ret),
> +	TP_PROTO(struct inode *inode, int cp_reason, int datasync, int ret),
>  
> -	TP_ARGS(inode, need_cp, datasync, ret),
> +	TP_ARGS(inode, cp_reason, datasync, ret),
>  
>  	TP_STRUCT__entry(
>  		__field(dev_t,	dev)
>  		__field(ino_t,	ino)
> -		__field(int,	need_cp)
> +		__field(int,	cp_reason)
>  		__field(int,	datasync)
>  		__field(int,	ret)
>  	),
> @@ -225,15 +225,15 @@ TRACE_EVENT(f2fs_sync_file_exit,
>  	TP_fast_assign(
>  		__entry->dev		= inode->i_sb->s_dev;
>  		__entry->ino		= inode->i_ino;
> -		__entry->need_cp	= need_cp;
> +		__entry->cp_reason	= cp_reason;
>  		__entry->datasync	= datasync;
>  		__entry->ret		= ret;
>  	),
>  
> -	TP_printk("dev = (%d,%d), ino = %lu, checkpoint is %s, "
> +	TP_printk("dev = (%d,%d), ino = %lu, cp_reason:%d, "
>  		"datasync = %d, ret = %d",
>  		show_dev_ino(__entry),
> -		__entry->need_cp ? "needed" : "not needed",
> +		__entry->cp_reason,
>  		__entry->datasync,
>  		__entry->ret)
>  );
> -- 
> 2.14.1.145.gb3622a4ee

Re: [PATCH 2/3] f2fs: trace checkpoint reason in fsync()

[Chao Yu]

On 2017/11/6 8:55, Jaegeuk Kim wrote:
> On 11/05, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> This patch slightly changes need_do_checkpoint to return the detail
>> info that indicates why we need do checkpoint, then caller could print
>> it with trace message.
>>
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/file.c              | 34 ++++++++++++++++++----------------
>>  include/trace/events/f2fs.h | 12 ++++++------
>>  2 files changed, 24 insertions(+), 22 deletions(-)
>>
>> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
>> index cee0f36ba9a1..d9d8b1c372f8 100644
>> --- a/fs/f2fs/file.c
>> +++ b/fs/f2fs/file.c
>> @@ -141,27 +141,29 @@ static int get_parent_ino(struct inode *inode, nid_t *pino)
>>  	return 1;
>>  }
>>  
>> -static inline bool need_do_checkpoint(struct inode *inode)
>> +static inline int need_do_checkpoint(struct inode *inode)
>>  {
>>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> -	bool need_cp = false;
>> +	int cp_reason = 0;
>>  
>> -	if (!S_ISREG(inode->i_mode) || inode->i_nlink != 1)
>> -		need_cp = true;
>> +	if (!S_ISREG(inode->i_mode))
>> +		cp_reason = 1;
>> +	else if (inode->i_nlink != 1)
>> +		cp_reason = 2;
>>  	else if (is_sbi_flag_set(sbi, SBI_NEED_CP))
>> -		need_cp = true;
>> +		cp_reason = 3;
>>  	else if (file_wrong_pino(inode))
>> -		need_cp = true;
>> +		cp_reason = 4;
>>  	else if (!space_for_roll_forward(sbi))
>> -		need_cp = true;
>> +		cp_reason = 5;
>>  	else if (!is_checkpointed_node(sbi, F2FS_I(inode)->i_pino))
>> -		need_cp = true;
>> +		cp_reason = 6;
>>  	else if (test_opt(sbi, FASTBOOT))
>> -		need_cp = true;
>> +		cp_reason = 7;
>>  	else if (sbi->active_logs == 2)
>> -		need_cp = true;
>> +		cp_reason = 8;
> 
> We need to make the numbers by enum so that we could interpret them in tracepoint
> below.

Agreed, could you check v2? :)

Thanks,

> 
> Thanks,
> 
>>  
>> -	return need_cp;
>> +	return cp_reason;
>>  }
>>  
>>  static bool need_inode_page_update(struct f2fs_sb_info *sbi, nid_t ino)
>> @@ -196,7 +198,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>  	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>  	nid_t ino = inode->i_ino;
>>  	int ret = 0;
>> -	bool need_cp = false;
>> +	bool cp_reason = 0;
>>  	struct writeback_control wbc = {
>>  		.sync_mode = WB_SYNC_ALL,
>>  		.nr_to_write = LONG_MAX,
>> @@ -215,7 +217,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>  	clear_inode_flag(inode, FI_NEED_IPU);
>>  
>>  	if (ret) {
>> -		trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
>> +		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
>>  		return ret;
>>  	}
>>  
>> @@ -246,10 +248,10 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>  	 * sudden-power-off.
>>  	 */
>>  	down_read(&F2FS_I(inode)->i_sem);
>> -	need_cp = need_do_checkpoint(inode);
>> +	cp_reason = need_do_checkpoint(inode);
>>  	up_read(&F2FS_I(inode)->i_sem);
>>  
>> -	if (need_cp) {
>> +	if (cp_reason) {
>>  		/* all the dirty node pages should be flushed for POR */
>>  		ret = f2fs_sync_fs(inode->i_sb, 1);
>>  
>> @@ -306,7 +308,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
>>  	}
>>  	f2fs_update_time(sbi, REQ_TIME);
>>  out:
>> -	trace_f2fs_sync_file_exit(inode, need_cp, datasync, ret);
>> +	trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
>>  	f2fs_trace_ios(NULL, 1);
>>  	return ret;
>>  }
>> diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
>> index 009745564214..f1a317e6db79 100644
>> --- a/include/trace/events/f2fs.h
>> +++ b/include/trace/events/f2fs.h
>> @@ -210,14 +210,14 @@ DEFINE_EVENT(f2fs__inode, f2fs_sync_file_enter,
>>  
>>  TRACE_EVENT(f2fs_sync_file_exit,
>>  
>> -	TP_PROTO(struct inode *inode, int need_cp, int datasync, int ret),
>> +	TP_PROTO(struct inode *inode, int cp_reason, int datasync, int ret),
>>  
>> -	TP_ARGS(inode, need_cp, datasync, ret),
>> +	TP_ARGS(inode, cp_reason, datasync, ret),
>>  
>>  	TP_STRUCT__entry(
>>  		__field(dev_t,	dev)
>>  		__field(ino_t,	ino)
>> -		__field(int,	need_cp)
>> +		__field(int,	cp_reason)
>>  		__field(int,	datasync)
>>  		__field(int,	ret)
>>  	),
>> @@ -225,15 +225,15 @@ TRACE_EVENT(f2fs_sync_file_exit,
>>  	TP_fast_assign(
>>  		__entry->dev		= inode->i_sb->s_dev;
>>  		__entry->ino		= inode->i_ino;
>> -		__entry->need_cp	= need_cp;
>> +		__entry->cp_reason	= cp_reason;
>>  		__entry->datasync	= datasync;
>>  		__entry->ret		= ret;
>>  	),
>>  
>> -	TP_printk("dev = (%d,%d), ino = %lu, checkpoint is %s, "
>> +	TP_printk("dev = (%d,%d), ino = %lu, cp_reason:%d, "
>>  		"datasync = %d, ret = %d",
>>  		show_dev_ino(__entry),
>> -		__entry->need_cp ? "needed" : "not needed",
>> +		__entry->cp_reason,
>>  		__entry->datasync,
>>  		__entry->ret)
>>  );
>> -- 
>> 2.14.1.145.gb3622a4ee
> 
> .
> 

Re: [PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[Michal Hocko]

On Sun 05-11-17 21:53:28, Chao Yu wrote:
> From: Chao Yu <yuchao0@huawei.com>
> 
> We will keep __add_ino_entry success all the time, for ENOMEM failure
> case, we have already handled it with an opened loop code, so we don't
> have to use redundant __GFP_NOFAIL in radix_tree_preload, remove it.

Why do you think an open coded allocation retry loop is better than
having the MM do all it can when the nofail is requested explicitly?
E.g. giving it an access to memory reserves to allow forward progress.

> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
>  fs/f2fs/checkpoint.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 98777c1ae70c..43ee9d97fd8f 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -405,10 +405,11 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>  {
>  	struct inode_management *im = &sbi->im[type];
>  	struct ino_entry *e, *tmp;
> +	bool preloaded;
>  
>  	tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
>  retry:
> -	radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
> +	preloaded = !radix_tree_preload(GFP_NOFS);
>  
>  	spin_lock(&im->ino_lock);
>  	e = radix_tree_lookup(&im->ino_root, ino);
> @@ -416,7 +417,8 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>  		e = tmp;
>  		if (radix_tree_insert(&im->ino_root, ino, e)) {
>  			spin_unlock(&im->ino_lock);
> -			radix_tree_preload_end();
> +			if (preloaded)
> +				radix_tree_preload_end();
>  			goto retry;
>  		}
>  		memset(e, 0, sizeof(struct ino_entry));
> @@ -431,7 +433,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>  		f2fs_set_bit(devidx, (char *)&e->dirty_device);
>  
>  	spin_unlock(&im->ino_lock);
> -	radix_tree_preload_end();
> +
> +	if (preloaded)
> +		radix_tree_preload_end();
>  
>  	if (e != tmp)
>  		kmem_cache_free(ino_entry_slab, tmp);
> -- 
> 2.14.1.145.gb3622a4ee

-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[Chao Yu]

Hi Michal,

On 2017/11/6 22:08, Michal Hocko wrote:
> On Sun 05-11-17 21:53:28, Chao Yu wrote:
>> From: Chao Yu <yuchao0@huawei.com>
>>
>> We will keep __add_ino_entry success all the time, for ENOMEM failure
>> case, we have already handled it with an opened loop code, so we don't
>> have to use redundant __GFP_NOFAIL in radix_tree_preload, remove it.
> 
> Why do you think an open coded allocation retry loop is better than
> having the MM do all it can when the nofail is requested explicitly?> E.g. giving it an access to memory reserves to allow forward progress.

Well, just want to remove one redundant implementation. But, as you said
MM has done lots of work to grab free memory including accessing memory
reserves, I think it's not bad for f2fs to use this flag instead of opened
loop code. :)

BTW, I notice the comments of __GFP_NOFAIL, what does this mean?
 *   Using this flag for costly allocations is _highly_ discouraged.

Thanks,

> 
>> Signed-off-by: Chao Yu <yuchao0@huawei.com>
>> ---
>>  fs/f2fs/checkpoint.c | 10 +++++++---
>>  1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>> index 98777c1ae70c..43ee9d97fd8f 100644
>> --- a/fs/f2fs/checkpoint.c
>> +++ b/fs/f2fs/checkpoint.c
>> @@ -405,10 +405,11 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  {
>>  	struct inode_management *im = &sbi->im[type];
>>  	struct ino_entry *e, *tmp;
>> +	bool preloaded;
>>  
>>  	tmp = f2fs_kmem_cache_alloc(ino_entry_slab, GFP_NOFS);
>>  retry:
>> -	radix_tree_preload(GFP_NOFS | __GFP_NOFAIL);
>> +	preloaded = !radix_tree_preload(GFP_NOFS);
>>  
>>  	spin_lock(&im->ino_lock);
>>  	e = radix_tree_lookup(&im->ino_root, ino);
>> @@ -416,7 +417,8 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  		e = tmp;
>>  		if (radix_tree_insert(&im->ino_root, ino, e)) {
>>  			spin_unlock(&im->ino_lock);
>> -			radix_tree_preload_end();
>> +			if (preloaded)
>> +				radix_tree_preload_end();
>>  			goto retry;
>>  		}
>>  		memset(e, 0, sizeof(struct ino_entry));
>> @@ -431,7 +433,9 @@ static void __add_ino_entry(struct f2fs_sb_info *sbi, nid_t ino,
>>  		f2fs_set_bit(devidx, (char *)&e->dirty_device);
>>  
>>  	spin_unlock(&im->ino_lock);
>> -	radix_tree_preload_end();
>> +
>> +	if (preloaded)
>> +		radix_tree_preload_end();
>>  
>>  	if (e != tmp)
>>  		kmem_cache_free(ino_entry_slab, tmp);
>> -- 
>> 2.14.1.145.gb3622a4ee
> 

Re: [PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[Michal Hocko]

On Tue 07-11-17 00:08:25, Chao Yu wrote:
[...]
> BTW, I notice the comments of __GFP_NOFAIL, what does this mean?
>  *   Using this flag for costly allocations is _highly_ discouraged.

This means that using __GFP_NOFAIL for high order allocations
(especially those with order > PAGE_ALLOC_COSTLY_ORDER) are highly
discouraged because we those are quite hard to get and looping inside
the allocator basically for ever is not a wise thing to do. That being
said replacing __GFP_NOFAIL by an open coded retry loop might make sense
for those e.g. to check signals or other termination conditions.
-- 
Michal Hocko
SUSE Labs

Re: [PATCH 1/3] f2fs: avoid using __GFP_NOFAIL

[Chao Yu]

On 2017/11/7 0:23, Michal Hocko wrote:
> On Tue 07-11-17 00:08:25, Chao Yu wrote:
> [...]
>> BTW, I notice the comments of __GFP_NOFAIL, what does this mean?
>>  *   Using this flag for costly allocations is _highly_ discouraged.
> 
> This means that using __GFP_NOFAIL for high order allocations
> (especially those with order > PAGE_ALLOC_COSTLY_ORDER) are highly
> discouraged because we those are quite hard to get and looping inside
> the allocator basically for ever is not a wise thing to do. That being
> said replacing __GFP_NOFAIL by an open coded retry loop might make sense
> for those e.g. to check signals or other termination conditions.

Clear to me now, thanks for your explanation and review. :)

Anyway, let me update this patch.

Thanks,

>