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 <[email protected]>
---
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
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,
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,
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,
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,
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
>
From 64fe93a7f9c35c2b5a34cfa3cf84158852c201be Mon Sep 17 00:00:00 2001
From: Jaegeuk Kim <[email protected]>
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 <[email protected]>
---
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
On 2021/8/27 0:52, Jaegeuk Kim wrote:
> From 64fe93a7f9c35c2b5a34cfa3cf84158852c201be Mon Sep 17 00:00:00 2001
> From: Jaegeuk Kim <[email protected]>
> 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 <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,