linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable
@ 2021-08-23 17:01 Jaegeuk Kim
  2021-08-24  1:08 ` [f2fs-dev] " Chao Yu
  2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim
  0 siblings, 2 replies; 9+ messages in thread
From: Jaegeuk Kim @ 2021-08-23 17:01 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel; +Cc: Jaegeuk Kim

We must flush dirty pages when calling fsync() during checkpoint=disable.
Returning zero makes inode being clear, which fails to flush them when
enabling checkpoint back even by sync_inodes_sb().

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/file.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d4fc5e0d2ffe..45c54332358b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -262,8 +262,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);
@@ -277,7 +276,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;
 	}
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-23 17:01 [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable Jaegeuk Kim
@ 2021-08-24  1:08 ` Chao Yu
  2021-08-24 17:09   ` Jaegeuk Kim
  2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim
  1 sibling, 1 reply; 9+ messages in thread
From: Chao Yu @ 2021-08-24  1:08 UTC (permalink / raw)
  To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel

On 2021/8/24 1:01, Jaegeuk Kim wrote:
> We must flush dirty pages when calling fsync() during checkpoint=disable.
> Returning zero makes inode being clear, which fails to flush them when
> enabling checkpoint back even by sync_inodes_sb().

Without this patch, file can be persisted via checkpoint=enable as well, my
testcase:

- mount -t f2fs -o checkpoint=disable,checkpoint_nomerge /dev/pmem0 /mnt/f2fs/
- cp file /mnt/f2fs/
- xfs_io /mnt/f2fs/file -c "fdatasync"
- mount -o remount,checkpoint=enable /dev/pmem0 /mnt/f2fs/
- umount /mnt/f2fs
- mount /dev/pmem0 /mnt/f2fs
- md5sum file /mnt/f2fs/file
chksum values are the same.

Am I missing something?

Thanks,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-24  1:08 ` [f2fs-dev] " Chao Yu
@ 2021-08-24 17:09   ` Jaegeuk Kim
  2021-08-24 23:30     ` Chao Yu
  0 siblings, 1 reply; 9+ messages in thread
From: Jaegeuk Kim @ 2021-08-24 17:09 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 08/24, Chao Yu wrote:
> On 2021/8/24 1:01, Jaegeuk Kim wrote:
> > We must flush dirty pages when calling fsync() during checkpoint=disable.
> > Returning zero makes inode being clear, which fails to flush them when
> > enabling checkpoint back even by sync_inodes_sb().
> 
> Without this patch, file can be persisted via checkpoint=enable as well, my
> testcase:
> 
> - mount -t f2fs -o checkpoint=disable,checkpoint_nomerge /dev/pmem0 /mnt/f2fs/
> - cp file /mnt/f2fs/
> - xfs_io /mnt/f2fs/file -c "fdatasync"
> - mount -o remount,checkpoint=enable /dev/pmem0 /mnt/f2fs/
> - umount /mnt/f2fs
> - mount /dev/pmem0 /mnt/f2fs
> - md5sum file /mnt/f2fs/file
> chksum values are the same.
> 
> Am I missing something?

I'm trying to address one subtle issue where a file has only NEW_ADDR by the
checkpoint=disable test. I don't think this hurts anything but can see
some mitigation of the issue.

> 
> Thanks,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-24 17:09   ` Jaegeuk Kim
@ 2021-08-24 23:30     ` Chao Yu
  2021-08-25 21:31       ` Jaegeuk Kim
  0 siblings, 1 reply; 9+ messages in thread
From: Chao Yu @ 2021-08-24 23:30 UTC (permalink / raw)
  To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel

On 2021/8/25 1:09, Jaegeuk Kim wrote:
> On 08/24, Chao Yu wrote:
>> On 2021/8/24 1:01, Jaegeuk Kim wrote:
>>> We must flush dirty pages when calling fsync() during checkpoint=disable.
>>> Returning zero makes inode being clear, which fails to flush them when
>>> enabling checkpoint back even by sync_inodes_sb().
>>
>> Without this patch, file can be persisted via checkpoint=enable as well, my
>> testcase:
>>
>> - mount -t f2fs -o checkpoint=disable,checkpoint_nomerge /dev/pmem0 /mnt/f2fs/
>> - cp file /mnt/f2fs/
>> - xfs_io /mnt/f2fs/file -c "fdatasync"
>> - mount -o remount,checkpoint=enable /dev/pmem0 /mnt/f2fs/
>> - umount /mnt/f2fs
>> - mount /dev/pmem0 /mnt/f2fs
>> - md5sum file /mnt/f2fs/file
>> chksum values are the same.
>>
>> Am I missing something?
> 
> I'm trying to address one subtle issue where a file has only NEW_ADDR by the

Oh, I doubt that we may failed to flush data of all inodes due to failures during
sync_inodes_sb(), additionally, how about adding retry logic for sync_inodes_sb()
if there is still any F2FS_DIRTY_DATA reference counts in f2fs_enable_checkpoint()
to mitigate this issue, e.g.:

f2fs_enable_checkpoint()

	do {
		sync_inode_sb();
		congestion_wait();
		cond_resched();
	} while (get_pages(sbi, F2FS_DIRTY_DATA) && retry_count--)

	if (get_pages(sbi, F2FS_DIRTY_DATA))
		f2fs_warm("");

Thanks,

> checkpoint=disable test. I don't think this hurts anything but can see
> some mitigation of the issue.
> 
>>
>> Thanks,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-24 23:30     ` Chao Yu
@ 2021-08-25 21:31       ` Jaegeuk Kim
  0 siblings, 0 replies; 9+ messages in thread
From: Jaegeuk Kim @ 2021-08-25 21:31 UTC (permalink / raw)
  To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel

On 08/25, Chao Yu wrote:
> On 2021/8/25 1:09, Jaegeuk Kim wrote:
> > On 08/24, Chao Yu wrote:
> > > On 2021/8/24 1:01, Jaegeuk Kim wrote:
> > > > We must flush dirty pages when calling fsync() during checkpoint=disable.
> > > > Returning zero makes inode being clear, which fails to flush them when
> > > > enabling checkpoint back even by sync_inodes_sb().
> > > 
> > > Without this patch, file can be persisted via checkpoint=enable as well, my
> > > testcase:
> > > 
> > > - mount -t f2fs -o checkpoint=disable,checkpoint_nomerge /dev/pmem0 /mnt/f2fs/
> > > - cp file /mnt/f2fs/
> > > - xfs_io /mnt/f2fs/file -c "fdatasync"
> > > - mount -o remount,checkpoint=enable /dev/pmem0 /mnt/f2fs/
> > > - umount /mnt/f2fs
> > > - mount /dev/pmem0 /mnt/f2fs
> > > - md5sum file /mnt/f2fs/file
> > > chksum values are the same.
> > > 
> > > Am I missing something?
> > 
> > I'm trying to address one subtle issue where a file has only NEW_ADDR by the
> 
> Oh, I doubt that we may failed to flush data of all inodes due to failures during
> sync_inodes_sb(), additionally, how about adding retry logic for sync_inodes_sb()
> if there is still any F2FS_DIRTY_DATA reference counts in f2fs_enable_checkpoint()
> to mitigate this issue, e.g.:
> 
> f2fs_enable_checkpoint()
> 
> 	do {
> 		sync_inode_sb();
> 		congestion_wait();
> 		cond_resched();
> 	} while (get_pages(sbi, F2FS_DIRTY_DATA) && retry_count--)
> 
> 	if (get_pages(sbi, F2FS_DIRTY_DATA))
> 		f2fs_warm("");

Agreed. Sent v2.

> 
> Thanks,
> 
> > checkpoint=disable test. I don't think this hurts anything but can see
> > some mitigation of the issue.
> > 
> > > 
> > > Thanks,

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-23 17:01 [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable Jaegeuk Kim
  2021-08-24  1:08 ` [f2fs-dev] " Chao Yu
@ 2021-08-25 21:33 ` Jaegeuk Kim
  2021-08-26  0:16   ` [f2fs-dev] " Chao Yu
  2021-08-26 16:52   ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
  1 sibling, 2 replies; 9+ messages in thread
From: Jaegeuk Kim @ 2021-08-25 21:33 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel

We must flush all the dirty data when enabling checkpoint back. Let's guarantee
that first. In order to mitigate any failure, let's flush data in fsync as well
during checkpoint=disable.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
v2 from v1:
 - handle sync_inodes_sb() failure

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

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cc2080866c54..3330efb41f22 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -263,8 +263,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);
@@ -278,7 +277,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;
 	}
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 49e153fd8183..d2f97dfb17af 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2088,8 +2088,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))
+		f2fs_warn(sbi, "checkpoint=enable has some unwritten data.");
 
 	down_write(&sbi->gc_lock);
 	f2fs_dirty_to_prefree(sbi);
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim
@ 2021-08-26  0:16   ` Chao Yu
  2021-08-26 16:52   ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
  1 sibling, 0 replies; 9+ messages in thread
From: Chao Yu @ 2021-08-26  0:16 UTC (permalink / raw)
  To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel

On 2021/8/26 5:33, Jaegeuk Kim wrote:
> We must flush all the dirty data when enabling checkpoint back. Let's guarantee
> that first. In order to mitigate any failure, let's flush data in fsync as well
> during checkpoint=disable.

It needs to update comments a bit with respect to update part of v2?

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
> v2 from v1:
>   - handle sync_inodes_sb() failure
> 
>   fs/f2fs/file.c  |  5 ++---
>   fs/f2fs/super.c | 11 ++++++++++-
>   2 files changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index cc2080866c54..3330efb41f22 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -263,8 +263,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);
> @@ -278,7 +277,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;
>   	}
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 49e153fd8183..d2f97dfb17af 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2088,8 +2088,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))

Well, if we break the loop due to retry-- == 0, value of retry will be -1 here.

So should be:

if (unlikely(retry < 0))

Thanks,

> +		f2fs_warn(sbi, "checkpoint=enable has some unwritten data.");
>   
>   	down_write(&sbi->gc_lock);
>   	f2fs_dirty_to_prefree(sbi);
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim
  2021-08-26  0:16   ` [f2fs-dev] " Chao Yu
@ 2021-08-26 16:52   ` Jaegeuk Kim
  2021-08-27 14:36     ` Chao Yu
  1 sibling, 1 reply; 9+ messages in thread
From: Jaegeuk Kim @ 2021-08-26 16:52 UTC (permalink / raw)
  To: linux-kernel, linux-f2fs-devel

From 64fe93a7f9c35c2b5a34cfa3cf84158852c201be Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <jaegeuk@kernel.org>
Date: Thu, 19 Aug 2021 14:00:57 -0700
Subject: [PATCH] f2fs: guarantee to write dirty data when enabling checkpoint
 back

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.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 Change log from v2:
- repharse the patch description a bit
- fix retry condition check

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

diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cc2080866c54..3330efb41f22 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -263,8 +263,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);
@@ -278,7 +277,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;
 	}
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 49e153fd8183..b8fecf4f37e0 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2088,8 +2088,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);
-- 
2.33.0.rc2.250.ged5fa647cd-goog


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable
  2021-08-26 16:52   ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
@ 2021-08-27 14:36     ` Chao Yu
  0 siblings, 0 replies; 9+ messages in thread
From: Chao Yu @ 2021-08-27 14:36 UTC (permalink / raw)
  To: Jaegeuk Kim, linux-kernel, linux-f2fs-devel

On 2021/8/27 0:52, Jaegeuk Kim wrote:
>  From 64fe93a7f9c35c2b5a34cfa3cf84158852c201be Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <jaegeuk@kernel.org>
> Date: Thu, 19 Aug 2021 14:00:57 -0700
> Subject: [PATCH] f2fs: guarantee to write dirty data when enabling checkpoint
>   back
> 
> 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.
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-27 14:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-23 17:01 [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable Jaegeuk Kim
2021-08-24  1:08 ` [f2fs-dev] " Chao Yu
2021-08-24 17:09   ` Jaegeuk Kim
2021-08-24 23:30     ` Chao Yu
2021-08-25 21:31       ` Jaegeuk Kim
2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim
2021-08-26  0:16   ` [f2fs-dev] " Chao Yu
2021-08-26 16:52   ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim
2021-08-27 14:36     ` Chao Yu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).