Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-09 Thread 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 (謝明燈)" 
Signed-off-by: Jaegeuk Kim 
---
   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=%sSpecify 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");
/*



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-09 Thread Jaegeuk Kim 
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 (謝明燈)" 
> > Signed-off-by: Jaegeuk Kim 
> > ---
> >   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");
> > /*


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-09 Thread Daeho Jeong 
On Thu, Apr 4, 2024 at 12:54 PM 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 (謝明燈)" 
> Signed-off-by: Jaegeuk Kim 
> ---
>  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
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-08 Thread Chao Yu 

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 (謝明燈)" 
Signed-off-by: Jaegeuk Kim 
---
  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");

/*



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[f2fs-dev] [PATCH] f2fs: don't set RO when shutting down f2fs

2024-04-04 Thread Jaegeuk Kim 
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 (謝明燈)" 
Signed-off-by: Jaegeuk Kim 
---
 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



___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel