linux-kernel - Re: [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250401-konsens-nahebringen-fa1c80956371@brauner>
Date: Tue, 1 Apr 2025 11:35:56 +0200
From: Christian Brauner <brauner@...nel.org>
To: Jan Kara <jack@...e.cz>
Cc: linux-fsdevel@...r.kernel.org, Ard Biesheuvel <ardb@...nel.org>, 
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	James Bottomley <James.Bottomley@...senpartnership.com>, mcgrof@...nel.org, hch@...radead.org, david@...morbit.com, 
	rafael@...nel.org, djwong@...nel.org, pavel@...nel.org, peterz@...radead.org, 
	mingo@...hat.com, will@...nel.org, boqun.feng@...il.com
Subject: Re: [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing

On Tue, Apr 01, 2025 at 11:16:18AM +0200, Jan Kara wrote:
> On Tue 01-04-25 02:32:46, Christian Brauner wrote:
> > From: Luis Chamberlain <mcgrof@...nel.org>
> > 
> > The kernel power management now supports allowing the VFS
> > to handle filesystem freezing freezes and thawing. Take advantage
> > of that and remove the kthread freezing. This is needed so that we
> > properly really stop IO in flight without races after userspace
> > has been frozen. Without this we rely on kthread freezing and
> > its semantics are loose and error prone.
> > 
> > The filesystem therefore is in charge of properly dealing with
> > quiescing of the filesystem through its callbacks if it thinks
> > it knows better than how the VFS handles it.
> > 
> > The following Coccinelle rule was used as to remove the now superfluous
> > freezer calls:
> > 
> > make coccicheck MODE=patch SPFLAGS="--in-place --no-show-diff" COCCI=./fs-freeze-cleanup.cocci M=fs/ext4
> > 
> > virtual patch
> > 
> > @ remove_set_freezable @
> > expression time;
> > statement S, S2;
> > expression task, current;
> > @@
> > 
> > (
> > -       set_freezable();
> > |
> > -       if (try_to_freeze())
> > -               continue;
> > |
> > -       try_to_freeze();
> > |
> > -       freezable_schedule();
> > +       schedule();
> > |
> > -       freezable_schedule_timeout(time);
> > +       schedule_timeout(time);
> > |
> > -       if (freezing(task)) { S }
> > |
> > -       if (freezing(task)) { S }
> > -       else
> > 	    { S2 }
> > |
> > -       freezing(current)
> > )
> > 
> > @ remove_wq_freezable @
> > expression WQ_E, WQ_ARG1, WQ_ARG2, WQ_ARG3, WQ_ARG4;
> > identifier fs_wq_fn;
> > @@
> > 
> > (
> >     WQ_E = alloc_workqueue(WQ_ARG1,
> > -                              WQ_ARG2 | WQ_FREEZABLE,
> > +                              WQ_ARG2,
> > 			   ...);
> > |
> >     WQ_E = alloc_workqueue(WQ_ARG1,
> > -                              WQ_ARG2 | WQ_FREEZABLE | WQ_ARG3,
> > +                              WQ_ARG2 | WQ_ARG3,
> > 			   ...);
> > |
> >     WQ_E = alloc_workqueue(WQ_ARG1,
> > -                              WQ_ARG2 | WQ_ARG3 | WQ_FREEZABLE,
> > +                              WQ_ARG2 | WQ_ARG3,
> > 			   ...);
> > |
> >     WQ_E = alloc_workqueue(WQ_ARG1,
> > -                              WQ_ARG2 | WQ_ARG3 | WQ_FREEZABLE | WQ_ARG4,
> > +                              WQ_ARG2 | WQ_ARG3 | WQ_ARG4,
> > 			   ...);
> > |
> > 	    WQ_E =
> > -               WQ_ARG1 | WQ_FREEZABLE
> > +               WQ_ARG1
> > |
> > 	    WQ_E =
> > -               WQ_ARG1 | WQ_FREEZABLE | WQ_ARG3
> > +               WQ_ARG1 | WQ_ARG3
> > |
> >     fs_wq_fn(
> > -               WQ_FREEZABLE | WQ_ARG2 | WQ_ARG3
> > +               WQ_ARG2 | WQ_ARG3
> >     )
> > |
> >     fs_wq_fn(
> > -               WQ_FREEZABLE | WQ_ARG2
> > +               WQ_ARG2
> >     )
> > |
> >     fs_wq_fn(
> > -               WQ_FREEZABLE
> > +               0
> >     )
> > )
> > 
> > @ add_auto_flag @
> > expression E1;
> > identifier fs_type;
> > @@
> > 
> > struct file_system_type fs_type = {
> > 	.fs_flags = E1
> > +                   | FS_AUTOFREEZE
> > 	,
> > };
> > 
> > Generated-by: Coccinelle SmPL
> > Signed-off-by: Luis Chamberlain <mcgrof@...nel.org>
> > Link: https://lore.kernel.org/r/20250326112220.1988619-5-mcgrof@kernel.org
> > Signed-off-by: Christian Brauner <brauner@...nel.org>
> > ---
> >  fs/ext4/mballoc.c | 2 +-
> >  fs/ext4/super.c   | 3 ---
> >  2 files changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 0d523e9fb3d5..ae235ec5ff3a 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -6782,7 +6782,7 @@ static ext4_grpblk_t ext4_last_grp_cluster(struct super_block *sb,
> >  
> >  static bool ext4_trim_interrupted(void)
> >  {
> > -	return fatal_signal_pending(current) || freezing(current);
> > +	return fatal_signal_pending(current);
> >  }
> 
> This change should not happen. ext4_trim_interrupted() makes sure FITRIM
> ioctl doesn't cause hibernation failures and has nothing to do with kthread
> freezing...
> 
> Otherwise the patch looks good.

Afaict, we don't have to do these changes now. Yes, once fsfreeze
reliably works in the suspend/resume codepaths then we can switch all
that off and remove the old freezer. But we should only do that once we
have some experience with the new filesystem freezing during
suspend/hibernate. So we should place this under a
/sys/power/freeze_filesystems knob and wait a few kernel releases to see
whether we see significant problems. How does that sound to you?