Messages in this thread | | | Date | Wed, 10 Apr 2024 09:24:59 +0800 | Subject | Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs | From | Chao Yu <> |
| |
On 2024/4/10 0:21, Jaegeuk Kim wrote: > On 04/09, Chao Yu wrote: >> On 2024/4/5 3:52, Jaegeuk Kim wrote: >>> Shutdown does not check the error of thaw_super due to readonly, which >>> causes a deadlock like below. >>> >>> f2fs_ioc_shutdown(F2FS_GOING_DOWN_FULLSYNC) issue_discard_thread >>> - bdev_freeze >>> - freeze_super >>> - f2fs_stop_checkpoint() >>> - f2fs_handle_critical_error - sb_start_write >>> - set RO - waiting >>> - bdev_thaw >>> - thaw_super_locked >>> - return -EINVAL, if sb_rdonly() >>> - f2fs_stop_discard_thread >>> -> wait for kthread_stop(discard_thread); >>> >>> Reported-by: "Light Hsieh (謝明燈)" <Light.Hsieh@mediatek.com> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org> >>> --- >>> fs/f2fs/super.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c >>> index df9765b41dac..ba6288e870c5 100644 >>> --- a/fs/f2fs/super.c >>> +++ b/fs/f2fs/super.c >>> @@ -4135,9 +4135,16 @@ void f2fs_handle_critical_error(struct f2fs_sb_info *sbi, unsigned char reason, >>> if (shutdown) >>> set_sbi_flag(sbi, SBI_IS_SHUTDOWN); >>> - /* continue filesystem operators if errors=continue */ >>> - if (continue_fs || f2fs_readonly(sb)) >>> + /* >>> + * Continue filesystem operators if errors=continue. Should not set >>> + * RO by shutdown, since RO bypasses thaw_super which can hang the >>> + * system. >>> + */ >>> + if (continue_fs || f2fs_readonly(sb) || >>> + reason == STOP_CP_REASON_SHUTDOWN) { >>> + f2fs_warn(sbi, "Stopped filesystem due to readon: %d", reason); >>> return; >> >> Do we need to set RO after bdev_thaw() in f2fs_do_shutdown()? > > IIRC, shutdown doesn't need to set RO as we stopped the checkpoint. > I'm more concerned on any side effect caused by this RO change.
Okay, I just wonder whether we need to follow semantics of errors=remount-ro semantics, but it looks fine since shutdown operation simulated by ioctl could not be treated as an inner critical error,
errors=%s Specify f2fs behavior on critical errors. This supports modes: "panic", "continue" and "remount-ro", respectively, trigger panic immediately, continue without doing anything, and remount the partition in read-only mode. By default it uses "continue" mode.
Also, it keeps the behavior consistent w/ what we do for errors=panic case.
if (F2FS_OPTION(sbi).errors == MOUNT_ERRORS_PANIC && !shutdown && !system_going_down() && ^^^^^^^^^ !is_sbi_flag_set(sbi, SBI_IS_SHUTDOWN)) panic("F2FS-fs (device %s): panic forced after error\n", sb->s_id);
Thanks,
> >> >> Thanks, >> >>> + } >>> f2fs_warn(sbi, "Remounting filesystem read-only"); >>> /*
| |