Skip to contentSkip to conversation

f2fs: fix to commit bio cache after flushing node pages

PATCH In review Chao Yu 3 participants 9 messages Sep 26, 2016 – Sep 29, 2016 F2FS file system

lore
Share & tools
lore

Chao Yu

author maintainer
#0 lore raw

PATCH 1/2 f2fs: fix to commit bio cache after flushing node pages

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +9 -2
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
 
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2
author maintainer
#1 lore raw

PATCH 2/2 f2fs: remove redundant io plug

From: Chao Yu <[email protected]>

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/checkpoint.c | 3 ---
 1 file changed, 3 deletions(-)
fs/f2fs/checkpoint.c +0 -3
index a655d75..de8693c 100644
@@ -267,7 +267,6 @@ static int f2fs_write_meta_pages(struct address_space *mapping,
 				struct writeback_control *wbc)
 {
 	struct f2fs_sb_info *sbi = F2FS_M_SB(mapping);
-	struct blk_plug plug;
 	long diff, written;
 
 	/* collect a number of dirty meta pages and write together */

@@ -280,9 +279,7 @@ static int f2fs_write_meta_pages(struct address_space *mapping,
 	/* if mounting is failed, skip writing node pages */
 	mutex_lock(&sbi->cp_mutex);
 	diff = nr_pages_to_write(sbi, META, wbc);
-	blk_start_plug(&plug);
 	written = sync_meta_pages(sbi, META, wbc->nr_to_write);
-	blk_finish_plug(&plug);
 	mutex_unlock(&sbi->cp_mutex);
 	wbc->nr_to_write = max((long)0, wbc->nr_to_write - written - diff);
 	return 0;
Signature · 1 line
2.7.2
maintainer
#2 lore raw
In reply to Chao Yu

Hi Chao,

Quoted 47 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
 
 			if (--wbc->nr_to_write == 0)
 				break;

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 7 lines
fs/f2fs/node.c +3 -1
@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2
#3 lore raw

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

Thanks,

Quoted 12 lines

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2

.

maintainer
#4 lore raw
Quoted 64 lines · Chao Yu

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Quoted 4 lines

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

The do_writepages() is okay, which will call f2fs_write_node_pages(). The __writeback_single_inode() won't do filemap_fdatawait() with WB_SYNC_ALL. We don't need to take care of truncation as well.

Any missing one?

Quoted 15 lines

Thanks,

Quoted 12 lines

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2

.

------------------------------------------------------------------------------

#5 lore raw
Quoted 70 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Quoted 4 lines

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

Quoted 5 lines

The do_writepages() is okay, which will call f2fs_write_node_pages(). The __writeback_single_inode() won't do filemap_fdatawait() with WB_SYNC_ALL. We don't need to take care of truncation as well.

Any missing one?

Another is: while testing with first version of checkpoint error injection, I encounter below dump stack:

"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mount D ffff8801c1bf7960 0 97685 97397 0x00080000
ffff8801c1bf7960 ffff8801c1bf7930 ffff880175900000 ffff8801c1bf7980 ffff8801c1bf8000 0000000000000000 7fffffffffffffff ffff88021f7be340 ffffffff817c8880 ffff8801c1bf7978 ffffffff817c80a5 ffff880214f58fc0

Call Trace:
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c80a5>] schedule+0x35/0x80
 [<ffffffff817cb152>] schedule_timeout+0x292/0x3d0
 [<ffffffff81022ab5>] ? xen_clocksource_get_cycles+0x15/0x20
 [<ffffffff810eeb5c>] ? ktime_get+0x3c/0xb0
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c7906>] io_schedule_timeout+0xa6/0x110
 [<ffffffff817c889b>] bit_wait_io+0x1b/0x60
 [<ffffffff817c84e4>] __wait_on_bit+0x64/0x90
 [<ffffffff8117dcd4>] wait_on_page_bit+0xc4/0xd0
 [<ffffffff810bc4d0>] ? autoremove_wake_function+0x40/0x40
 [<ffffffff81190a29>] truncate_inode_pages_range+0x409/0x840
 [<ffffffff811a406d>] ? pcpu_free_area+0x13d/0x1a0
 [<ffffffff810bc025>] ? wake_up_bit+0x25/0x30
 [<ffffffff81190ecc>] truncate_inode_pages_final+0x4c/0x60
 [<ffffffffa025e9e8>] f2fs_evict_inode+0x48/0x390 [f2fs]
 [<ffffffff812212f7>] evict+0xc7/0x1a0
 [<ffffffff81221f77>] iput+0x197/0x200
 [<ffffffffa0268242>] f2fs_fill_super+0xab2/0x1130 [f2fs]
 [<ffffffff81209454>] mount_bdev+0x184/0x1c0
 [<ffffffffa0267790>] ? f2fs_commit_super+0x100/0x100 [f2fs]
 [<ffffffffa02646a5>] f2fs_mount+0x15/0x20 [f2fs]
 [<ffffffff81209e19>] mount_fs+0x39/0x160
 [<ffffffff81225e47>] vfs_kern_mount+0x67/0x110
 [<ffffffff812283bb>] do_mount+0x1bb/0xc80
 [<ffffffff81229163>] SyS_mount+0x83/0xd0
 [<ffffffff8100391e>] do_syscall_64+0x6e/0x170
 [<ffffffff817cc325>] entry_SYSCALL64_slow_path+0x25/0x25

Any thoughts?

Quoted 19 lines
Quoted 16 lines

Thanks,

Quoted 12 lines

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2

.

.

maintainer
#6 lore raw
Quoted 115 lines · Chao Yu
Quoted 70 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Quoted 4 lines

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

Quoted 5 lines

The do_writepages() is okay, which will call f2fs_write_node_pages(). The __writeback_single_inode() won't do filemap_fdatawait() with WB_SYNC_ALL. We don't need to take care of truncation as well.

Any missing one?

Another is: while testing with first version of checkpoint error injection, I encounter below dump stack:

"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mount D ffff8801c1bf7960 0 97685 97397 0x00080000
ffff8801c1bf7960 ffff8801c1bf7930 ffff880175900000 ffff8801c1bf7980 ffff8801c1bf8000 0000000000000000 7fffffffffffffff ffff88021f7be340 ffffffff817c8880 ffff8801c1bf7978 ffffffff817c80a5 ffff880214f58fc0

Call Trace:
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c80a5>] schedule+0x35/0x80
 [<ffffffff817cb152>] schedule_timeout+0x292/0x3d0
 [<ffffffff81022ab5>] ? xen_clocksource_get_cycles+0x15/0x20
 [<ffffffff810eeb5c>] ? ktime_get+0x3c/0xb0
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c7906>] io_schedule_timeout+0xa6/0x110
 [<ffffffff817c889b>] bit_wait_io+0x1b/0x60
 [<ffffffff817c84e4>] __wait_on_bit+0x64/0x90
 [<ffffffff8117dcd4>] wait_on_page_bit+0xc4/0xd0
 [<ffffffff810bc4d0>] ? autoremove_wake_function+0x40/0x40
 [<ffffffff81190a29>] truncate_inode_pages_range+0x409/0x840
 [<ffffffff811a406d>] ? pcpu_free_area+0x13d/0x1a0
 [<ffffffff810bc025>] ? wake_up_bit+0x25/0x30
 [<ffffffff81190ecc>] truncate_inode_pages_final+0x4c/0x60
 [<ffffffffa025e9e8>] f2fs_evict_inode+0x48/0x390 [f2fs]
 [<ffffffff812212f7>] evict+0xc7/0x1a0
 [<ffffffff81221f77>] iput+0x197/0x200
 [<ffffffffa0268242>] f2fs_fill_super+0xab2/0x1130 [f2fs]
 [<ffffffff81209454>] mount_bdev+0x184/0x1c0
 [<ffffffffa0267790>] ? f2fs_commit_super+0x100/0x100 [f2fs]
 [<ffffffffa02646a5>] f2fs_mount+0x15/0x20 [f2fs]
 [<ffffffff81209e19>] mount_fs+0x39/0x160
 [<ffffffff81225e47>] vfs_kern_mount+0x67/0x110
 [<ffffffff812283bb>] do_mount+0x1bb/0xc80
 [<ffffffff81229163>] SyS_mount+0x83/0xd0
 [<ffffffff8100391e>] do_syscall_64+0x6e/0x170
 [<ffffffff817cc325>] entry_SYSCALL64_slow_path+0x25/0x25
Quoted 1 line

Any thoughts?

I think this should not happen normally, since f2fs_stop_checkpoint() calls f2fs_flush_merged_bios().

Thanks,

Quoted 18 lines
Quoted 18 lines
Quoted 17 lines

Thanks,

Quoted 12 lines

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2

.

Quoted 2 lines
Quoted 2 lines

.

------------------------------------------------------------------------------

#7 lore raw
Quoted 116 lines · Jaegeuk Kim
Quoted 115 lines · Chao Yu
Quoted 70 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Quoted 4 lines

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

Quoted 5 lines

The do_writepages() is okay, which will call f2fs_write_node_pages(). The __writeback_single_inode() won't do filemap_fdatawait() with WB_SYNC_ALL. We don't need to take care of truncation as well.

Any missing one?

Another is: while testing with first version of checkpoint error injection, I encounter below dump stack:

"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mount D ffff8801c1bf7960 0 97685 97397 0x00080000
ffff8801c1bf7960 ffff8801c1bf7930 ffff880175900000 ffff8801c1bf7980 ffff8801c1bf8000 0000000000000000 7fffffffffffffff ffff88021f7be340 ffffffff817c8880 ffff8801c1bf7978 ffffffff817c80a5 ffff880214f58fc0

Call Trace:
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c80a5>] schedule+0x35/0x80
 [<ffffffff817cb152>] schedule_timeout+0x292/0x3d0
 [<ffffffff81022ab5>] ? xen_clocksource_get_cycles+0x15/0x20
 [<ffffffff810eeb5c>] ? ktime_get+0x3c/0xb0
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c7906>] io_schedule_timeout+0xa6/0x110
 [<ffffffff817c889b>] bit_wait_io+0x1b/0x60
 [<ffffffff817c84e4>] __wait_on_bit+0x64/0x90
 [<ffffffff8117dcd4>] wait_on_page_bit+0xc4/0xd0
 [<ffffffff810bc4d0>] ? autoremove_wake_function+0x40/0x40
 [<ffffffff81190a29>] truncate_inode_pages_range+0x409/0x840
 [<ffffffff811a406d>] ? pcpu_free_area+0x13d/0x1a0
 [<ffffffff810bc025>] ? wake_up_bit+0x25/0x30
 [<ffffffff81190ecc>] truncate_inode_pages_final+0x4c/0x60
 [<ffffffffa025e9e8>] f2fs_evict_inode+0x48/0x390 [f2fs]
 [<ffffffff812212f7>] evict+0xc7/0x1a0
 [<ffffffff81221f77>] iput+0x197/0x200
 [<ffffffffa0268242>] f2fs_fill_super+0xab2/0x1130 [f2fs]
 [<ffffffff81209454>] mount_bdev+0x184/0x1c0
 [<ffffffffa0267790>] ? f2fs_commit_super+0x100/0x100 [f2fs]
 [<ffffffffa02646a5>] f2fs_mount+0x15/0x20 [f2fs]
 [<ffffffff81209e19>] mount_fs+0x39/0x160
 [<ffffffff81225e47>] vfs_kern_mount+0x67/0x110
 [<ffffffff812283bb>] do_mount+0x1bb/0xc80
 [<ffffffff81229163>] SyS_mount+0x83/0xd0
 [<ffffffff8100391e>] do_syscall_64+0x6e/0x170
 [<ffffffff817cc325>] entry_SYSCALL64_slow_path+0x25/0x25
Quoted 4 lines
Quoted 1 line

Any thoughts?

I think this should not happen normally, since f2fs_stop_checkpoint() calls f2fs_flush_merged_bios().

In write_end_io, f2fs_stop_checkpoint will not call f2fs_flush_merged_bios.

One other problem here is it can cause latency during waiting writeback:

In fsync()
1. fsync_node_pages a/b/c pages is submitted, and d/e/f pages is still in bio cache as we didn't commit the bio in the end.
2. wait_on_node_pages_writeback will wait a/b/c pages for writebacking, then submit bio with d/e/f pages and the wait until they have writebacked to device.

Here we need to submit cached bios at the end of fsync_node_pages/sync_node_pages to let them being merged in block layer as much as possible, and also to avoid suffering more delay time due to double submit&wait.

Thanks,

Quoted 21 lines

Thanks,

Quoted 18 lines
Quoted 18 lines
Quoted 17 lines

Thanks,

Quoted 12 lines

Thanks,

Quoted 6 lines
fs/f2fs/node.c +1 -0
+	return ret;
 }
 
 int wait_on_node_pages_writeback(struct f2fs_sb_info *sbi, nid_t ino)
Signature · 1 line
2.7.2

.

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

.

.

------------------------------------------------------------------------------

maintainer
#8 lore raw
Quoted 117 lines · Chao Yu
Quoted 116 lines · Jaegeuk Kim
Quoted 115 lines · Chao Yu
Quoted 70 lines · Jaegeuk Kim
Quoted 64 lines · Chao Yu

Hi Jaegeuk,

Quoted 47 lines · Jaegeuk Kim

Hi Chao,

Quoted 44 lines · Chao Yu

From: Chao Yu <[email protected]>

In sync_node_pages, we won't check and commit last merged pages in private bio cache of f2fs, as these pages were taged as writeback, someone who is waiting for writebacking of the page will be blocked until the cache was committed by someone else.

We need to commit node type bio cache to avoid potential deadlock or long delay of waiting writeback.

Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/node.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)
fs/f2fs/node.c +5 -1
index 9faddcd..f73f774 100644
@@ -1416,6 +1416,7 @@ int sync_node_pages(struct f2fs_sb_info *sbi, struct writeback_control *wbc)
 	struct pagevec pvec;
 	int step = 0;
 	int nwritten = 0;
+	int ret = 0;
 
 	pagevec_init(&pvec, 0);
 

@@ -1436,7 +1437,8 @@ next_step:
 
 			if (unlikely(f2fs_cp_error(sbi))) {
 				pagevec_release(&pvec);
-				return -EIO;
+				ret = -EIO;
+				goto out;
 			}
 
 			/*

@@ -1487,6 +1489,8 @@ continue_unlock:
 
 			if (NODE_MAPPING(sbi)->a_ops->writepage(page, wbc))
 				unlock_page(page);
+			else
+				nwritten++;
Quoted 13 lines

fs/f2fs/node.c continue_unlock: · line 1513

Quoted 10 lines
fs/f2fs/node.c +3 -1
 			if (--wbc->nr_to_write == 0)
 				break;

@@ -1504,7 +1508,10 @@ continue_unlock:
 		step++;
 		goto next_step;
 	}
-	return nwritten;
+out:
+	if (nwritten)
+		f2fs_submit_merged_bio(sbi, NODE, WRITE);

IIRC, we don't need to flush this, since f2fs_submit_merged_bio_cond() would handle this in f2fs_wait_on_page_writeback().

Quoted 4 lines

Yes, it covers all the cases in f2fs private codes, but there are still some codes in mm or fs directory, and they didn't use f2fs_wait_on_page_writeback when waiting page writeback. Such as do_writepages && filemap_fdatawait in __writeback_single_inode...

Quoted 5 lines

The do_writepages() is okay, which will call f2fs_write_node_pages(). The __writeback_single_inode() won't do filemap_fdatawait() with WB_SYNC_ALL. We don't need to take care of truncation as well.

Any missing one?

Another is: while testing with first version of checkpoint error injection, I encounter below dump stack:

"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
mount D ffff8801c1bf7960 0 97685 97397 0x00080000
ffff8801c1bf7960 ffff8801c1bf7930 ffff880175900000 ffff8801c1bf7980 ffff8801c1bf8000 0000000000000000 7fffffffffffffff ffff88021f7be340 ffffffff817c8880 ffff8801c1bf7978 ffffffff817c80a5 ffff880214f58fc0

Call Trace:
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c80a5>] schedule+0x35/0x80
 [<ffffffff817cb152>] schedule_timeout+0x292/0x3d0
 [<ffffffff81022ab5>] ? xen_clocksource_get_cycles+0x15/0x20
 [<ffffffff810eeb5c>] ? ktime_get+0x3c/0xb0
 [<ffffffff817c8880>] ? bit_wait+0x50/0x50
 [<ffffffff817c7906>] io_schedule_timeout+0xa6/0x110
 [<ffffffff817c889b>] bit_wait_io+0x1b/0x60
 [<ffffffff817c84e4>] __wait_on_bit+0x64/0x90
 [<ffffffff8117dcd4>] wait_on_page_bit+0xc4/0xd0
 [<ffffffff810bc4d0>] ? autoremove_wake_function+0x40/0x40
 [<ffffffff81190a29>] truncate_inode_pages_range+0x409/0x840
 [<ffffffff811a406d>] ? pcpu_free_area+0x13d/0x1a0
 [<ffffffff810bc025>] ? wake_up_bit+0x25/0x30
 [<ffffffff81190ecc>] truncate_inode_pages_final+0x4c/0x60
 [<ffffffffa025e9e8>] f2fs_evict_inode+0x48/0x390 [f2fs]
 [<ffffffff812212f7>] evict+0xc7/0x1a0
 [<ffffffff81221f77>] iput+0x197/0x200
 [<ffffffffa0268242>] f2fs_fill_super+0xab2/0x1130 [f2fs]
 [<ffffffff81209454>] mount_bdev+0x184/0x1c0
 [<ffffffffa0267790>] ? f2fs_commit_super+0x100/0x100 [f2fs]
 [<ffffffffa02646a5>] f2fs_mount+0x15/0x20 [f2fs]
 [<ffffffff81209e19>] mount_fs+0x39/0x160
 [<ffffffff81225e47>] vfs_kern_mount+0x67/0x110
 [<ffffffff812283bb>] do_mount+0x1bb/0xc80
 [<ffffffff81229163>] SyS_mount+0x83/0xd0
 [<ffffffff8100391e>] do_syscall_64+0x6e/0x170
 [<ffffffff817cc325>] entry_SYSCALL64_slow_path+0x25/0x25
Quoted 6 lines
Quoted 4 lines
Quoted 1 line

Any thoughts?

I think this should not happen normally, since f2fs_stop_checkpoint() calls f2fs_flush_merged_bios().

In write_end_io, f2fs_stop_checkpoint will not call f2fs_flush_merged_bios.

So, in write_node_page(), we call f2fs_submit_merged_bio() if f2fs_cp_error() is set.

Quoted 11 lines

One other problem here is it can cause latency during waiting writeback:

In fsync()
1. fsync_node_pages a/b/c pages is submitted, and d/e/f pages is still in bio cache as we didn't commit the bio in the end.
2. wait_on_node_pages_writeback will wait a/b/c pages for writebacking, then submit bio with d/e/f pages and the wait until they have writebacked to device.

Here we need to submit cached bios at the end of fsync_node_pages/sync_node_pages to let them being merged in block layer as much as possible, and also to avoid suffering more delay time due to double submit&wait.

I think this is more reasonable to me. ;)

Thanks,