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 (謝明燈)" <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
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;
+ }
f2fs_warn(sbi, "Remounting filesystem read-only");
/*
--
2.44.0.478.gd926399ef9-goog
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 (謝明燈)" <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> 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()?
Thanks,
> + }
>
> f2fs_warn(sbi, "Remounting filesystem read-only");
> /*
On Thu, Apr 4, 2024 at 12:54 PM Jaegeuk Kim <[email protected]> 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 (謝明燈)" <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
> 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) {
I think we can use "shutdown" variable instead of "reason ==
STOP_CP_REASON_SHUTDOWN" to be concise.
> + f2fs_warn(sbi, "Stopped filesystem due to readon: %d", reason);
readon -> reason?
> return;
> + }
>
> f2fs_warn(sbi, "Remounting filesystem read-only");
> /*
> --
> 2.44.0.478.gd926399ef9-goog
>
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
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 (謝明燈)" <[email protected]>
> > Signed-off-by: Jaegeuk Kim <[email protected]>
> > ---
> > 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.
>
> Thanks,
>
> > + }
> > f2fs_warn(sbi, "Remounting filesystem read-only");
> > /*
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 (謝明燈)" <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
Change log from v1:
- use better variable
- fix typo
fs/f2fs/super.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 8ac4734c2df6..df32573d1f62 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -4159,9 +4159,15 @@ 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) || shutdown) {
+ f2fs_warn(sbi, "Stopped filesystem due to reason: %d", reason);
return;
+ }
f2fs_warn(sbi, "Remounting filesystem read-only");
/*
--
2.44.0.478.gd926399ef9-goog
On Tue, Apr 9, 2024 at 9:21 AM Jaegeuk Kim <[email protected]> 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 (謝明燈)" <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
> ---
>
> Change log from v1:
> - use better variable
> - fix typo
>
> fs/f2fs/super.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 8ac4734c2df6..df32573d1f62 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -4159,9 +4159,15 @@ 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) || shutdown) {
> + f2fs_warn(sbi, "Stopped filesystem due to reason: %d", reason);
> return;
> + }
>
> f2fs_warn(sbi, "Remounting filesystem read-only");
> /*
> --
> 2.44.0.478.gd926399ef9-goog
>
>
Reviewed-by: Daeho Jeong <[email protected]>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
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 (謝明燈)" <[email protected]>
>>> Signed-off-by: Jaegeuk Kim <[email protected]>
>>> ---
>>> 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");
>>> /*
On 2024/4/10 0:20, 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 (謝明燈)" <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,