| Message ID | 20240606095213.4087668-1-chao@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,1/2] f2fs: fix to add missing sb_{start, end}_intwrite() for ckpt thread | expand |
On 2024/6/6 17:52, Chao Yu wrote: > After commit 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount > option"), checkpoint can be triggered in background thread, it missed > to cover f2fs inner checkpoint operation w/ sb_{start,end}_intwrite(), > fix it. It needs to use sb_start_intwrite_trylock(), otherwise, it will cause deadlock as below: - freeze_super - sb_wait_write(SB_FREEZE_WRITE) - sb_wait_write(SB_FREEZE_PAGEFAULT) - sb_wait_write(SB_FREEZE_FS) - sync - iterate_supers - super_lock_shared - down_read(&sb->s_umount) - sync_fs_one_sb - f2fs_sync_fs - f2fs_issue_checkpoint - wait_for_completion - issue_checkpoint_thread - sb_start_intwrite(sbi->sb); - thaw_super - super_lock_excl - down_write(&sb->s_umount) Thanks, > > Fixes: 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount option") > Cc: Daeho Jeong <daehojeong@google.com> > Signed-off-by: Chao Yu <chao@kernel.org> > --- > fs/f2fs/checkpoint.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > index 55d444bec5c0..66eaad591b60 100644 > --- a/fs/f2fs/checkpoint.c > +++ b/fs/f2fs/checkpoint.c > @@ -1828,8 +1828,11 @@ static int issue_checkpoint_thread(void *data) > if (kthread_should_stop()) > return 0; > > - if (!llist_empty(&cprc->issue_list)) > + if (!llist_empty(&cprc->issue_list)) { > + sb_start_intwrite(sbi->sb); > __checkpoint_and_complete_reqs(sbi); > + sb_end_intwrite(sbi->sb); > + } > > wait_event_interruptible(*q, > kthread_should_stop() || !llist_empty(&cprc->issue_list));
On Sat, Jun 8, 2024 at 5:36 AM Chao Yu <chao@kernel.org> wrote: > > On 2024/6/6 17:52, Chao Yu wrote: > > After commit 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount > > option"), checkpoint can be triggered in background thread, it missed > > to cover f2fs inner checkpoint operation w/ sb_{start,end}_intwrite(), > > fix it. > > It needs to use sb_start_intwrite_trylock(), otherwise, it will cause > deadlock as below: > > - freeze_super > - sb_wait_write(SB_FREEZE_WRITE) > - sb_wait_write(SB_FREEZE_PAGEFAULT) > - sb_wait_write(SB_FREEZE_FS) > - sync > - iterate_supers > - super_lock_shared > - down_read(&sb->s_umount) > - sync_fs_one_sb > - f2fs_sync_fs > - f2fs_issue_checkpoint > - wait_for_completion > - issue_checkpoint_thread > - sb_start_intwrite(sbi->sb); > > - thaw_super > - super_lock_excl > - down_write(&sb->s_umount) > > Thanks, > > > > > Fixes: 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount option") > > Cc: Daeho Jeong <daehojeong@google.com> > > Signed-off-by: Chao Yu <chao@kernel.org> > > --- > > fs/f2fs/checkpoint.c | 5 ++++- > > 1 file changed, 4 insertions(+), 1 deletion(-) > > > > diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c > > index 55d444bec5c0..66eaad591b60 100644 > > --- a/fs/f2fs/checkpoint.c > > +++ b/fs/f2fs/checkpoint.c > > @@ -1828,8 +1828,11 @@ static int issue_checkpoint_thread(void *data) > > if (kthread_should_stop()) > > return 0; > > > > - if (!llist_empty(&cprc->issue_list)) > > + if (!llist_empty(&cprc->issue_list)) { > > + sb_start_intwrite(sbi->sb); > > __checkpoint_and_complete_reqs(sbi); > > + sb_end_intwrite(sbi->sb); > > + } > > > > wait_event_interruptible(*q, > > kthread_should_stop() || !llist_empty(&cprc->issue_list)); > > Reviewed-by: Daeho Jeong <daehojeong@google.com> Thanks. > _______________________________________________ > Linux-f2fs-devel mailing list > Linux-f2fs-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Hi all, Please ignore this patch, because during freeze_super(), it will call sync_fs() to clear all dirty data, there should be no race case after that. Thanks, On 2024/6/10 23:10, Daeho Jeong wrote: > On Sat, Jun 8, 2024 at 5:36 AM Chao Yu <chao@kernel.org> wrote: >> >> On 2024/6/6 17:52, Chao Yu wrote: >>> After commit 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount >>> option"), checkpoint can be triggered in background thread, it missed >>> to cover f2fs inner checkpoint operation w/ sb_{start,end}_intwrite(), >>> fix it. >> >> It needs to use sb_start_intwrite_trylock(), otherwise, it will cause >> deadlock as below: >> >> - freeze_super >> - sb_wait_write(SB_FREEZE_WRITE) >> - sb_wait_write(SB_FREEZE_PAGEFAULT) >> - sb_wait_write(SB_FREEZE_FS) >> - sync >> - iterate_supers >> - super_lock_shared >> - down_read(&sb->s_umount) >> - sync_fs_one_sb >> - f2fs_sync_fs >> - f2fs_issue_checkpoint >> - wait_for_completion >> - issue_checkpoint_thread >> - sb_start_intwrite(sbi->sb); >> >> - thaw_super >> - super_lock_excl >> - down_write(&sb->s_umount) >> >> Thanks, >> >>> >>> Fixes: 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount option") >>> Cc: Daeho Jeong <daehojeong@google.com> >>> Signed-off-by: Chao Yu <chao@kernel.org> >>> --- >>> fs/f2fs/checkpoint.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c >>> index 55d444bec5c0..66eaad591b60 100644 >>> --- a/fs/f2fs/checkpoint.c >>> +++ b/fs/f2fs/checkpoint.c >>> @@ -1828,8 +1828,11 @@ static int issue_checkpoint_thread(void *data) >>> if (kthread_should_stop()) >>> return 0; >>> >>> - if (!llist_empty(&cprc->issue_list)) >>> + if (!llist_empty(&cprc->issue_list)) { >>> + sb_start_intwrite(sbi->sb); >>> __checkpoint_and_complete_reqs(sbi); >>> + sb_end_intwrite(sbi->sb); >>> + } >>> >>> wait_event_interruptible(*q, >>> kthread_should_stop() || !llist_empty(&cprc->issue_list)); >> >> > > Reviewed-by: Daeho Jeong <daehojeong@google.com> > > Thanks. > > > >> _______________________________________________ >> Linux-f2fs-devel mailing list >> Linux-f2fs-devel@lists.sourceforge.net >> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 55d444bec5c0..66eaad591b60 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1828,8 +1828,11 @@ static int issue_checkpoint_thread(void *data) if (kthread_should_stop()) return 0; - if (!llist_empty(&cprc->issue_list)) + if (!llist_empty(&cprc->issue_list)) { + sb_start_intwrite(sbi->sb); __checkpoint_and_complete_reqs(sbi); + sb_end_intwrite(sbi->sb); + } wait_event_interruptible(*q, kthread_should_stop() || !llist_empty(&cprc->issue_list));
After commit 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount option"), checkpoint can be triggered in background thread, it missed to cover f2fs inner checkpoint operation w/ sb_{start,end}_intwrite(), fix it. Fixes: 261eeb9c1585 ("f2fs: introduce checkpoint_merge mount option") Cc: Daeho Jeong <daehojeong@google.com> Signed-off-by: Chao Yu <chao@kernel.org> --- fs/f2fs/checkpoint.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)