f2fs: fix to redirty page if fail to gc data page

PATCH In review Chao Yu 3 participants 9 messages May 21, 2016 – Jun 3, 2016 F2FS file system

lore

Share & tools

lore

Chao Yu

author maintainer

2016-05-21 05:19 UTC #0 lore raw

PATCH 1/1 f2fs: fix to redirty page if fail to gc data page

From: Chao Yu <[email protected]>

If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/f2fs/gc.c +4 -1

index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
 		clear_cold_data(page);
 	}
 out:

Signature · 1 line

2.7.2

Chao Yu

2016-05-28 00:54 UTC #1 lore raw

Ping,

Quoted 32 lines · Chao Yu

From: Chao Yu <[email protected]>

If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.

Signed-off-by Chao Yu <[email protected]>

---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

fs/f2fs/gc.c +4 -1

index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
 		clear_cold_data(page);
 	}
 out:

Jaegeuk Kim

maintainer

2016-05-30 02:37 UTC #2 lore raw

In reply to Chao Yu

Hi Chao,

Quoted 18 lines · Chao Yu

From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +0 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};

fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664

Quoted 10 lines

fs/f2fs/gc.c +4 -1

+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);

If this page is truncated with -ENOENT, we don't need to set it dirty again. I expect that, if we get an error here, do_garbage_collect() would retry FG_GC again.

Thanks,

Quoted 5 lines

fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2

Chao Yu

2016-05-31 06:10 UTC #3 lore raw

Hi Jaegeuk,

Quoted 25 lines · Jaegeuk Kim

Hi Chao,
Quoted 22 lines · Chao Yu
From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +2 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);

Quoted 8 lines

fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664
Quoted 6 lines
fs/f2fs/gc.c +2 -1
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
If this page is truncated with -ENOENT, we don't need to set it dirty again.

Agree

Quoted 1 line

I expect that, if we get an error here, do_garbage_collect() would retry FG_GC

IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think?

Quoted 11 lines

again.
Thanks,
Quoted 5 lines
fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2
.

Jaegeuk Kim

maintainer

2016-06-03 05:09 UTC #4 lore raw

Quoted 37 lines · Chao Yu

Hi Jaegeuk,
Quoted 25 lines · Jaegeuk Kim
Hi Chao,
Quoted 22 lines · Chao Yu
From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +2 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
Quoted 8 lines
fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664
Quoted 6 lines
fs/f2fs/gc.c +2 -1
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
If this page is truncated with -ENOENT, we don't need to set it dirty again.

Quoted 7 lines

Agree
Quoted 1 line
I expect that, if we get an error here, do_garbage_collect() would retry FG_GC
IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think?

Mostly I expected here -ENOENT caused by race condition. Do we have another expectation?

Thanks,

Quoted 12 lines

Quoted 12 lines
again.
Thanks,
Quoted 5 lines
fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2
.

Chao Yu

2016-06-03 05:13 UTC #5 lore raw

Quoted 40 lines · Jaegeuk Kim

Quoted 39 lines · Chao Yu
Hi Jaegeuk,
Quoted 25 lines · Jaegeuk Kim
Hi Chao,
Quoted 22 lines · Chao Yu
From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +2 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
Quoted 8 lines
fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664
Quoted 6 lines
fs/f2fs/gc.c +2 -1
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
If this page is truncated with -ENOENT, we don't need to set it dirty again.
Agree

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
I expect that, if we get an error here, do_garbage_collect() would retry FG_GC
IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think?
Mostly I expected here -ENOENT caused by race condition.

If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to worry about this case, right?

Quoted 1 line

Do we have another expectation?

ENOMEM or EIO?

Thanks,

Quoted 17 lines

Thanks,
Quoted 5 lines
Quoted 5 lines
again.
Thanks,
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2
.
.

Jaegeuk Kim

maintainer

2016-06-03 05:17 UTC #6 lore raw

Quoted 47 lines · Chao Yu

Quoted 46 lines · Jaegeuk Kim
Quoted 44 lines · Chao Yu
Hi Jaegeuk,
Quoted 25 lines · Jaegeuk Kim
Hi Chao,
Quoted 22 lines · Chao Yu
From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +2 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
Quoted 8 lines
fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664
Quoted 6 lines
fs/f2fs/gc.c +2 -1
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
If this page is truncated with -ENOENT, we don't need to set it dirty again.
Agree
Quoted 1 line
I expect that, if we get an error here, do_garbage_collect() would retry FG_GC
IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think?

Quoted 8 lines

Quoted 1 line
Mostly I expected here -ENOENT caused by race condition.
If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to worry about this case, right?
Quoted 1 line
Do we have another expectation?
ENOMEM or EIO?

EIO will stop everything.
ENOMEM would be better to wait for a while from page reclaim?

Quoted 20 lines

Thanks,
Quoted 17 lines
Thanks,
Quoted 5 lines
Quoted 5 lines
again.
Thanks,
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2
.
.

Chao Yu

2016-06-03 05:59 UTC #7 lore raw

Quoted 53 lines · Jaegeuk Kim

Quoted 52 lines · Chao Yu
Quoted 40 lines · Jaegeuk Kim
Quoted 39 lines · Chao Yu
Hi Jaegeuk,
Quoted 25 lines · Jaegeuk Kim
Hi Chao,
Quoted 22 lines · Chao Yu
From: Chao Yu <[email protected]>
If we fail to move data page during foreground GC, we should give another chance to writeback that page which was set dirty previously by writer.
Signed-off-by Chao Yu <[email protected]>
---
 fs/f2fs/gc.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/f2fs/gc.c +2 -0
index 38d56f6..ee213a8 100644
@@ -653,12 +653,15 @@ static void move_data_page(struct inode *inode, block_t bidx, int gc_type)
 			.page = page,
 			.encrypted_page = NULL,
 		};
+		bool is_dirty = PageDirty(page);
+
 		set_page_dirty(page);
 		f2fs_wait_on_page_writeback(page, DATA, true);
Quoted 8 lines
fs/f2fs/gc.c static void move_data_page(struct inode *inode, block_t bidx, int gc_type) · line 664
Quoted 6 lines
fs/f2fs/gc.c +2 -1
 		if (clear_page_dirty_for_io(page))
 			inode_dec_dirty_pages(inode);
 		set_cold_data(page);
-		do_write_data_page(&fio);
+		if (do_write_data_page(&fio) && is_dirty)
+			set_page_dirty(page);
If this page is truncated with -ENOENT, we don't need to set it dirty again.
Agree
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
I expect that, if we get an error here, do_garbage_collect() would retry FG_GC
IIRC, you have reworked the FG_GC flows changed from an infinite loop to trying do the movement just one time. Here, I think if there are just few of blocks are failed to be moved, we can give one more time for retrying. How do you think?
Mostly I expected here -ENOENT caused by race condition.
If we hit ENOENT case, we can pass get_valid_blocks check, so we don't need to worry about this case, right?

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Do we have another expectation?
ENOMEM or EIO?
EIO will stop everything.
ENOMEM would be better to wait for a while from page reclaim?

Agree, but for ioctl path, IMO, we don't need to let user waiting for ENOMEM case looping.

Quoted 23 lines

Quoted 21 lines
Thanks,
Quoted 17 lines
Thanks,
Quoted 5 lines
Quoted 5 lines
again.
Thanks,
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
fs/f2fs/gc.c +0 -0
 		clear_cold_data(page);
 	}
 out:
Signature · 1 line
2.7.2
.
.
.

Jaegeuk Kim

maintainer

2016-06-03 17:36 UTC #8 lore raw

On Fri, Jun 03, 2016 at 01:59:15PM +0800, Chao Yu wrote:
...

Quoted 9 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Do we have another expectation?
ENOMEM or EIO?
EIO will stop everything.
ENOMEM would be better to wait for a while from page reclaim?
Agree, but for ioctl path, IMO, we don't need to let user waiting for ENOMEM case looping.

Well, if user wanted to do a synchronous gc, we need that, IMO.