f2fs: guarantee to write dirty data when enabling checkpoint back

PATCH In review Jaegeuk Kim via Linux-f2fs-devel 3 participants 7 messages Sep 8, 2021 – Sep 13, 2021

Subsystems (2)

Stable branch F2FS file system

lore

Share & tools

lore

Jaegeuk Kim via Linux-f2fs-devel

author

2021-09-08 22:00 UTC #0 lore raw

PATCH 1/1 f2fs: guarantee to write dirty data when enabling checkpoint back

From: Jaegeuk Kim <[email protected]>

commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.

We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.

Reviewed-by Chao Yu <[email protected]>

Signed-off-by Jaegeuk Kim <[email protected]>

---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

fs/f2fs/file.c +2 -3

index 5c74b2997197..6ee8b1e0e174 100644
@@ -259,8 +259,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	};
 	unsigned int seq_id = 0;
 
-	if (unlikely(f2fs_readonly(inode->i_sb) ||
-				is_sbi_flag_set(sbi, SBI_CP_DISABLED)))
+	if (unlikely(f2fs_readonly(inode->i_sb)))
 		return 0;
 
 	trace_f2fs_sync_file_enter(inode);

@@ -274,7 +273,7 @@ static int f2fs_do_sync_file(struct file *file, loff_t start, loff_t end,
 	ret = file_write_and_wait_range(file, start, end);
 	clear_inode_flag(inode, FI_NEED_IPU);
 
-	if (ret) {
+	if (ret || is_sbi_flag_set(sbi, SBI_CP_DISABLED)) {
 		trace_f2fs_sync_file_exit(inode, cp_reason, datasync, ret);
 		return ret;
 	}

fs/f2fs/super.c +10 -1

index c52988067887..476b2c497d28 100644
@@ -1764,8 +1764,17 @@ static int f2fs_disable_checkpoint(struct f2fs_sb_info *sbi)
 
 static void f2fs_enable_checkpoint(struct f2fs_sb_info *sbi)
 {
+	int retry = DEFAULT_RETRY_IO_COUNT;
+
 	/* we should flush all the data to keep data consistency */
-	sync_inodes_sb(sbi->sb);
+	do {
+		sync_inodes_sb(sbi->sb);
+		cond_resched();
+		congestion_wait(BLK_RW_ASYNC, DEFAULT_IO_TIMEOUT);
+	} while (get_pages(sbi, F2FS_DIRTY_DATA) && retry--);
+
+	if (unlikely(retry < 0))
+		f2fs_warn(sbi, "checkpoint=enable has some unwritten data.");
 
 	down_write(&sbi->gc_lock);
 	f2fs_dirty_to_prefree(sbi);

Signature · 1 line

2.33.0.153.gba50c8fa24-goog

Greg KH

maintainer

2021-09-09 05:23 UTC #1 lore raw

Quoted 9 lines · Jaegeuk Kim

From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.

Quoted 6 lines

Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

What stable kernel(s) are you wanting to have this backported to?

thanks,

greg k-h

Jaegeuk Kim

maintainer

2021-09-09 05:28 UTC #2 lore raw

Quoted 10 lines · Greg KH

Quoted 9 lines · Jaegeuk Kim
From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.

Quoted 8 lines

Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)
What stable kernel(s) are you wanting to have this backported to?

5.10 please.

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2021-09-09 05:32 UTC #3 lore raw

Quoted 18 lines · Jaegeuk Kim

Quoted 17 lines · Greg KH
Quoted 15 lines · Jaegeuk Kim
From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.
Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

Quoted 3 lines

Quoted 1 line
What stable kernel(s) are you wanting to have this backported to?
5.10 please.

Why would you want to skip 5.14.y and 5.13.y? You never want anyone to upgrade stable kernel releases and have a regression.

thanks,

greg k-h

Jaegeuk Kim

maintainer

2021-09-09 05:42 UTC #4 lore raw

Quoted 19 lines · Greg KH

Quoted 18 lines · Jaegeuk Kim
Quoted 17 lines · Greg KH
Quoted 15 lines · Jaegeuk Kim
From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.
Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
What stable kernel(s) are you wanting to have this backported to?
5.10 please.
Why would you want to skip 5.14.y and 5.13.y? You never want anyone to upgrade stable kernel releases and have a regression.

I was just looking at the essential kernel version, since the fix is only related to checkpoint=disable feature used in android only. Feel free to merge it into any stable kernels if you want.

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2021-09-09 06:09 UTC #5 lore raw

Quoted 22 lines · Jaegeuk Kim

Quoted 21 lines · Greg KH
Quoted 20 lines · Jaegeuk Kim
Quoted 18 lines · Greg KH
Quoted 9 lines · Jaegeuk Kim
From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.
Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)
What stable kernel(s) are you wanting to have this backported to?

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
5.10 please.
Why would you want to skip 5.14.y and 5.13.y? You never want anyone to upgrade stable kernel releases and have a regression.
I was just looking at the essential kernel version, since the fix is only related to checkpoint=disable feature used in android only. Feel free to merge it into any stable kernels if you want.

No regressions for any stable releases is key here, Android is just one user of the kernel...

And in the future, just put a cc: stable in the signed-off-by area when you submit the patch and it will be handled automatically, like the documentation states :)

thanks,

greg k-h

Greg KH

maintainer

2021-09-13 09:24 UTC #6 lore raw

Quoted 32 lines · Greg KH

Quoted 22 lines · Jaegeuk Kim
Quoted 21 lines · Greg KH
Quoted 20 lines · Jaegeuk Kim
Quoted 18 lines · Greg KH
Quoted 9 lines · Jaegeuk Kim
From: Jaegeuk Kim <[email protected]>
commit dddd3d65293a52c2c3850c19b1e5115712e534d8 upstream.
We must flush all the dirty data when enabling checkpoint back. Let's guarantee that first by adding a retry logic on sync_inodes_sb(). In addition to that, this patch adds to flush data in fsync when checkpoint is disabled, which can mitigate the sync_inodes_sb() failures in advance.
Quoted 6 lines
Reviewed-by Chao Yu <[email protected]>
Signed-off-by Jaegeuk Kim <[email protected]>
---
 fs/f2fs/file.c  |  5 ++---
 fs/f2fs/super.c | 11 ++++++++++-
 2 files changed, 12 insertions(+), 4 deletions(-)
What stable kernel(s) are you wanting to have this backported to?
Quoted 8 lines
Quoted 4 lines
Quoted 1 line
5.10 please.
Why would you want to skip 5.14.y and 5.13.y? You never want anyone to upgrade stable kernel releases and have a regression.
I was just looking at the essential kernel version, since the fix is only related to checkpoint=disable feature used in android only. Feel free to merge it into any stable kernels if you want.

Quoted 6 lines

No regressions for any stable releases is key here, Android is just one user of the kernel...
And in the future, just put a cc: stable in the signed-off-by area when you submit the patch and it will be handled automatically, like the documentation states :)

Now queued up.

greg k-h