* [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-23 17:01 ` Jaegeuk Kim 0 siblings, 0 replies; 18+ 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 related [flat|nested] 18+ messages in thread
* [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-23 17:01 ` Jaegeuk Kim 0 siblings, 0 replies; 18+ 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 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-23 17:01 ` [f2fs-dev] " Jaegeuk Kim @ 2021-08-24 1:08 ` Chao Yu -1 siblings, 0 replies; 18+ 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] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-24 1:08 ` Chao Yu 0 siblings, 0 replies; 18+ 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, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-24 1:08 ` Chao Yu @ 2021-08-24 17:09 ` Jaegeuk Kim -1 siblings, 0 replies; 18+ 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, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ 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 0 siblings, 0 replies; 18+ 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] 18+ 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 -1 siblings, 0 replies; 18+ 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] 18+ 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 0 siblings, 0 replies; 18+ 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, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ 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 -1 siblings, 0 replies; 18+ 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] 18+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-25 21:31 ` Jaegeuk Kim 0 siblings, 0 replies; 18+ 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, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-23 17:01 ` [f2fs-dev] " Jaegeuk Kim @ 2021-08-25 21:33 ` Jaegeuk Kim -1 siblings, 0 replies; 18+ 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 related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-25 21:33 ` Jaegeuk Kim 0 siblings, 0 replies; 18+ 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 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-25 21:33 ` [f2fs-dev] " Jaegeuk Kim @ 2021-08-26 0:16 ` Chao Yu -1 siblings, 0 replies; 18+ 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] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v2] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-26 0:16 ` Chao Yu 0 siblings, 0 replies; 18+ 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); > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-25 21:33 ` [f2fs-dev] " Jaegeuk Kim @ 2021-08-26 16:52 ` Jaegeuk Kim -1 siblings, 0 replies; 18+ 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 related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-26 16:52 ` Jaegeuk Kim 0 siblings, 0 replies; 18+ 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 _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable 2021-08-26 16:52 ` Jaegeuk Kim @ 2021-08-27 14:36 ` Chao Yu -1 siblings, 0 replies; 18+ 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] 18+ messages in thread
* Re: [f2fs-dev] [PATCH v3] f2fs: don't ignore writing pages on fsync during checkpoint=disable @ 2021-08-27 14:36 ` Chao Yu 0 siblings, 0 replies; 18+ 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, _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-08-27 14:36 UTC | newest] Thread overview: 18+ 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-23 17:01 ` [f2fs-dev] " Jaegeuk Kim 2021-08-24 1:08 ` Chao Yu 2021-08-24 1:08 ` Chao Yu 2021-08-24 17:09 ` Jaegeuk Kim 2021-08-24 17:09 ` Jaegeuk Kim 2021-08-24 23:30 ` Chao Yu 2021-08-24 23:30 ` Chao Yu 2021-08-25 21:31 ` Jaegeuk Kim 2021-08-25 21:31 ` Jaegeuk Kim 2021-08-25 21:33 ` [PATCH v2] " Jaegeuk Kim 2021-08-25 21:33 ` [f2fs-dev] " Jaegeuk Kim 2021-08-26 0:16 ` Chao Yu 2021-08-26 0:16 ` Chao Yu 2021-08-26 16:52 ` [f2fs-dev] [PATCH v3] " Jaegeuk Kim 2021-08-26 16:52 ` Jaegeuk Kim 2021-08-27 14:36 ` Chao Yu 2021-08-27 14:36 ` Chao Yu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.