| Message ID | 20210908220020.599899-1-jaegeuk@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | f2fs: guarantee to write dirty data when enabling checkpoint back | expand |
On Wed, Sep 08, 2021 at 03:00:20PM -0700, Jaegeuk Kim wrote: > From: Jaegeuk Kim <jaegeuk@kernel.org> > > 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 <chao@kernel.org> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > --- > 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
On Wed, Sep 08, 2021 at 10:28:37PM -0700, Jaegeuk Kim wrote: > On 09/09, Greg KH wrote: > > On Wed, Sep 08, 2021 at 03:00:20PM -0700, Jaegeuk Kim wrote: > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > 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 <chao@kernel.org> > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > --- > > > 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. 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
On Wed, Sep 08, 2021 at 10:42:53PM -0700, Jaegeuk Kim wrote: > On 09/09, Greg KH wrote: > > On Wed, Sep 08, 2021 at 10:28:37PM -0700, Jaegeuk Kim wrote: > > > On 09/09, Greg KH wrote: > > > > On Wed, Sep 08, 2021 at 03:00:20PM -0700, Jaegeuk Kim wrote: > > > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > > > > > 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 <chao@kernel.org> > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > --- > > > > > 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. > > > > 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
On Thu, Sep 09, 2021 at 08:09:05AM +0200, Greg KH wrote: > On Wed, Sep 08, 2021 at 10:42:53PM -0700, Jaegeuk Kim wrote: > > On 09/09, Greg KH wrote: > > > On Wed, Sep 08, 2021 at 10:28:37PM -0700, Jaegeuk Kim wrote: > > > > On 09/09, Greg KH wrote: > > > > > On Wed, Sep 08, 2021 at 03:00:20PM -0700, Jaegeuk Kim wrote: > > > > > > From: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > > > > > > > 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 <chao@kernel.org> > > > > > > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> > > > > > > --- > > > > > > 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. > > > > > > 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 :) Now queued up. greg k-h
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c index 5c74b2997197..6ee8b1e0e174 100644 --- a/fs/f2fs/file.c +++ b/fs/f2fs/file.c @@ -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; } diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c index c52988067887..476b2c497d28 100644 --- a/fs/f2fs/super.c +++ b/fs/f2fs/super.c @@ -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);