* [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 9:16 ` Jan Kara
2025-04-01 0:32 ` [PATCH 2/6] btrfs: " Christian Brauner
` (8 subsequent siblings)
9 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
From: Luis Chamberlain <mcgrof@kernel.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@kernel.org>
Link: https://lore.kernel.org/r/20250326112220.1988619-5-mcgrof@kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.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);
}
static int ext4_try_to_trim_range(struct super_block *sb,
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 8122d4ffb3b5..020c818078d7 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -3778,7 +3778,6 @@ static int ext4_lazyinit_thread(void *arg)
unsigned long next_wakeup, cur;
BUG_ON(NULL == eli);
- set_freezable();
cont_thread:
while (true) {
@@ -3837,8 +3836,6 @@ static int ext4_lazyinit_thread(void *arg)
}
mutex_unlock(&eli->li_list_mtx);
- try_to_freeze();
-
cur = jiffies;
if (!next_wakeup_initialized || time_after_eq(cur, next_wakeup)) {
cond_resched();
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing
2025-04-01 0:32 ` [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing Christian Brauner
@ 2025-04-01 9:16 ` Jan Kara
2025-04-01 9:35 ` Christian Brauner
0 siblings, 1 reply; 59+ messages in thread
From: Jan Kara @ 2025-04-01 9:16 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Tue 01-04-25 02:32:46, Christian Brauner wrote:
> From: Luis Chamberlain <mcgrof@kernel.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@kernel.org>
> Link: https://lore.kernel.org/r/20250326112220.1988619-5-mcgrof@kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.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.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing
2025-04-01 9:16 ` Jan Kara
@ 2025-04-01 9:35 ` Christian Brauner
2025-04-01 10:08 ` Jan Kara
0 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 9:35 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
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@kernel.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@kernel.org>
> > Link: https://lore.kernel.org/r/20250326112220.1988619-5-mcgrof@kernel.org
> > Signed-off-by: Christian Brauner <brauner@kernel.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?
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing
2025-04-01 9:35 ` Christian Brauner
@ 2025-04-01 10:08 ` Jan Kara
0 siblings, 0 replies; 59+ messages in thread
From: Jan Kara @ 2025-04-01 10:08 UTC (permalink / raw)
To: Christian Brauner
Cc: Jan Kara, linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Tue 01-04-25 11:35:56, Christian Brauner wrote:
> On Tue, Apr 01, 2025 at 11:16:18AM +0200, Jan Kara wrote:
> > > ---
> > > 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?
I agree that enabling this with some knob to allow easy way out if things
don't work makes sense. And the removal of kthread freezing can be done
somewhat later when we are more confident filesystem freezing on
hibernation is solid.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 2/6] btrfs: replace kthread freezing with auto fs freezing
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
2025-04-01 0:32 ` [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 0:32 ` [PATCH 3/6] xfs: " Christian Brauner
` (7 subsequent siblings)
9 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
From: Luis Chamberlain <mcgrof@kernel.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/btrfs
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@kernel.org>
Link: https://lore.kernel.org/r/20250326112220.1988619-6-mcgrof@kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/btrfs/disk-io.c | 4 ++--
fs/btrfs/scrub.c | 2 +-
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 1a916716cefe..bce3ae569fe0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1962,8 +1962,8 @@ static void btrfs_init_qgroup(struct btrfs_fs_info *fs_info)
static int btrfs_init_workqueues(struct btrfs_fs_info *fs_info)
{
u32 max_active = fs_info->thread_pool_size;
- unsigned int flags = WQ_MEM_RECLAIM | WQ_FREEZABLE | WQ_UNBOUND;
- unsigned int ordered_flags = WQ_MEM_RECLAIM | WQ_FREEZABLE;
+ unsigned int flags = WQ_MEM_RECLAIM | WQ_UNBOUND;
+ unsigned int ordered_flags = WQ_MEM_RECLAIM;
fs_info->workers =
btrfs_alloc_workqueue(fs_info, "worker", flags, max_active, 16);
diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c
index 2c5edcee9450..5790177b4c2f 100644
--- a/fs/btrfs/scrub.c
+++ b/fs/btrfs/scrub.c
@@ -2877,7 +2877,7 @@ static void scrub_workers_put(struct btrfs_fs_info *fs_info)
static noinline_for_stack int scrub_workers_get(struct btrfs_fs_info *fs_info)
{
struct workqueue_struct *scrub_workers = NULL;
- unsigned int flags = WQ_FREEZABLE | WQ_UNBOUND;
+ unsigned int flags = WQ_UNBOUND;
int max_active = fs_info->thread_pool_size;
int ret = -ENOMEM;
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 3/6] xfs: replace kthread freezing with auto fs freezing
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
2025-04-01 0:32 ` [PATCH 1/6] ext4: replace kthread freezing with auto fs freezing Christian Brauner
2025-04-01 0:32 ` [PATCH 2/6] btrfs: " Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 1:11 ` Dave Chinner
2025-04-01 0:32 ` [PATCH 4/6] fs: add owner of freeze/thaw Christian Brauner
` (6 subsequent siblings)
9 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
From: Luis Chamberlain <mcgrof@kernel.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/xfs
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@kernel.org>
Link: https://lore.kernel.org/r/20250326112220.1988619-7-mcgrof@kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/xfs/xfs_discard.c | 2 +-
fs/xfs/xfs_log.c | 3 +--
fs/xfs/xfs_log_cil.c | 2 +-
fs/xfs/xfs_mru_cache.c | 2 +-
fs/xfs/xfs_pwork.c | 2 +-
fs/xfs/xfs_super.c | 14 +++++++-------
fs/xfs/xfs_trans_ail.c | 3 ---
fs/xfs/xfs_zone_gc.c | 2 --
8 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/fs/xfs/xfs_discard.c b/fs/xfs/xfs_discard.c
index c1a306268ae4..1596cf0ecb9b 100644
--- a/fs/xfs/xfs_discard.c
+++ b/fs/xfs/xfs_discard.c
@@ -333,7 +333,7 @@ xfs_trim_gather_extents(
static bool
xfs_trim_should_stop(void)
{
- return fatal_signal_pending(current) || freezing(current);
+ return fatal_signal_pending(current);
}
/*
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 6493bdb57351..317f6db292fb 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1489,8 +1489,7 @@ xlog_alloc_log(
log->l_iclog->ic_prev = prev_iclog; /* re-write 1st prev ptr */
log->l_ioend_workqueue = alloc_workqueue("xfs-log/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM |
- WQ_HIGHPRI),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_HIGHPRI),
0, mp->m_super->s_id);
if (!log->l_ioend_workqueue)
goto out_free_iclog;
diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
index 1ca406ec1b40..8ff5d68394e6 100644
--- a/fs/xfs/xfs_log_cil.c
+++ b/fs/xfs/xfs_log_cil.c
@@ -1932,7 +1932,7 @@ xlog_cil_init(
* concurrency the log spinlocks will be exposed to.
*/
cil->xc_push_wq = alloc_workqueue("xfs-cil/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM | WQ_UNBOUND),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_UNBOUND),
4, log->l_mp->m_super->s_id);
if (!cil->xc_push_wq)
goto out_destroy_cil;
diff --git a/fs/xfs/xfs_mru_cache.c b/fs/xfs/xfs_mru_cache.c
index d0f5b403bdbe..c9a49c6f6129 100644
--- a/fs/xfs/xfs_mru_cache.c
+++ b/fs/xfs/xfs_mru_cache.c
@@ -293,7 +293,7 @@ int
xfs_mru_cache_init(void)
{
xfs_mru_reap_wq = alloc_workqueue("xfs_mru_cache",
- XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_FREEZABLE), 1);
+ XFS_WQFLAGS(WQ_MEM_RECLAIM), 1);
if (!xfs_mru_reap_wq)
return -ENOMEM;
return 0;
diff --git a/fs/xfs/xfs_pwork.c b/fs/xfs/xfs_pwork.c
index c283b801cc5d..3f5bf53f8778 100644
--- a/fs/xfs/xfs_pwork.c
+++ b/fs/xfs/xfs_pwork.c
@@ -72,7 +72,7 @@ xfs_pwork_init(
trace_xfs_pwork_init(mp, nr_threads, current->pid);
pctl->wq = alloc_workqueue("%s-%d",
- WQ_UNBOUND | WQ_SYSFS | WQ_FREEZABLE, nr_threads, tag,
+ WQ_UNBOUND | WQ_SYSFS, nr_threads, tag,
current->pid);
if (!pctl->wq)
return -ENOMEM;
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 53944cc7af24..06eb51a3d13b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -565,37 +565,37 @@ xfs_init_mount_workqueues(
struct xfs_mount *mp)
{
mp->m_buf_workqueue = alloc_workqueue("xfs-buf/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM),
1, mp->m_super->s_id);
if (!mp->m_buf_workqueue)
goto out;
mp->m_unwritten_workqueue = alloc_workqueue("xfs-conv/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM),
0, mp->m_super->s_id);
if (!mp->m_unwritten_workqueue)
goto out_destroy_buf;
mp->m_reclaim_workqueue = alloc_workqueue("xfs-reclaim/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM),
0, mp->m_super->s_id);
if (!mp->m_reclaim_workqueue)
goto out_destroy_unwritten;
mp->m_blockgc_wq = alloc_workqueue("xfs-blockgc/%s",
- XFS_WQFLAGS(WQ_UNBOUND | WQ_FREEZABLE | WQ_MEM_RECLAIM),
+ XFS_WQFLAGS(WQ_UNBOUND | WQ_MEM_RECLAIM),
0, mp->m_super->s_id);
if (!mp->m_blockgc_wq)
goto out_destroy_reclaim;
mp->m_inodegc_wq = alloc_workqueue("xfs-inodegc/%s",
- XFS_WQFLAGS(WQ_FREEZABLE | WQ_MEM_RECLAIM),
+ XFS_WQFLAGS(WQ_MEM_RECLAIM),
1, mp->m_super->s_id);
if (!mp->m_inodegc_wq)
goto out_destroy_blockgc;
mp->m_sync_workqueue = alloc_workqueue("xfs-sync/%s",
- XFS_WQFLAGS(WQ_FREEZABLE), 0, mp->m_super->s_id);
+ XFS_WQFLAGS(0), 0, mp->m_super->s_id);
if (!mp->m_sync_workqueue)
goto out_destroy_inodegc;
@@ -2488,7 +2488,7 @@ xfs_init_workqueues(void)
* max_active value for this workqueue.
*/
xfs_alloc_wq = alloc_workqueue("xfsalloc",
- XFS_WQFLAGS(WQ_MEM_RECLAIM | WQ_FREEZABLE), 0);
+ XFS_WQFLAGS(WQ_MEM_RECLAIM), 0);
if (!xfs_alloc_wq)
return -ENOMEM;
diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
index 0fcb1828e598..ad8183db0780 100644
--- a/fs/xfs/xfs_trans_ail.c
+++ b/fs/xfs/xfs_trans_ail.c
@@ -636,7 +636,6 @@ xfsaild(
unsigned int noreclaim_flag;
noreclaim_flag = memalloc_noreclaim_save();
- set_freezable();
while (1) {
/*
@@ -695,8 +694,6 @@ xfsaild(
__set_current_state(TASK_RUNNING);
- try_to_freeze();
-
tout = xfsaild_push(ailp);
}
diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
index c5136ea9bb1d..1875b6551ab0 100644
--- a/fs/xfs/xfs_zone_gc.c
+++ b/fs/xfs/xfs_zone_gc.c
@@ -993,7 +993,6 @@ xfs_zone_gc_handle_work(
}
__set_current_state(TASK_RUNNING);
- try_to_freeze();
if (reset_list)
xfs_zone_gc_reset_zones(data, reset_list);
@@ -1041,7 +1040,6 @@ xfs_zoned_gcd(
unsigned int nofs_flag;
nofs_flag = memalloc_nofs_save();
- set_freezable();
for (;;) {
set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 3/6] xfs: replace kthread freezing with auto fs freezing
2025-04-01 0:32 ` [PATCH 3/6] xfs: " Christian Brauner
@ 2025-04-01 1:11 ` Dave Chinner
2025-04-01 7:17 ` Christian Brauner
0 siblings, 1 reply; 59+ messages in thread
From: Dave Chinner @ 2025-04-01 1:11 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, rafael, djwong, pavel, peterz,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 02:32:48AM +0200, Christian Brauner wrote:
> From: Luis Chamberlain <mcgrof@kernel.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.
>
.....
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 0fcb1828e598..ad8183db0780 100644
> --- a/fs/xfs/xfs_trans_ail.c
> +++ b/fs/xfs/xfs_trans_ail.c
> @@ -636,7 +636,6 @@ xfsaild(
> unsigned int noreclaim_flag;
>
> noreclaim_flag = memalloc_noreclaim_save();
> - set_freezable();
>
> while (1) {
> /*
> @@ -695,8 +694,6 @@ xfsaild(
>
> __set_current_state(TASK_RUNNING);
>
> - try_to_freeze();
> -
> tout = xfsaild_push(ailp);
> }
>
So what about the TASK_FREEZABLE flag that is set in this code
before sleeping?
i.e. this code before we schedule():
if (tout && tout <= 20)
set_current_state(TASK_KILLABLE|TASK_FREEZABLE);
else
set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
Shouldn't TASK_FREEZABLE go away, too?
> diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> index c5136ea9bb1d..1875b6551ab0 100644
> --- a/fs/xfs/xfs_zone_gc.c
> +++ b/fs/xfs/xfs_zone_gc.c
> @@ -993,7 +993,6 @@ xfs_zone_gc_handle_work(
> }
>
> __set_current_state(TASK_RUNNING);
> - try_to_freeze();
>
> if (reset_list)
> xfs_zone_gc_reset_zones(data, reset_list);
> @@ -1041,7 +1040,6 @@ xfs_zoned_gcd(
> unsigned int nofs_flag;
>
> nofs_flag = memalloc_nofs_save();
> - set_freezable();
>
> for (;;) {
> set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);
Same question here for this newly merged code, too...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 3/6] xfs: replace kthread freezing with auto fs freezing
2025-04-01 1:11 ` Dave Chinner
@ 2025-04-01 7:17 ` Christian Brauner
2025-04-01 11:35 ` Dave Chinner
0 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 7:17 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, rafael, djwong, pavel, peterz,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 12:11:04PM +1100, Dave Chinner wrote:
> On Tue, Apr 01, 2025 at 02:32:48AM +0200, Christian Brauner wrote:
> > From: Luis Chamberlain <mcgrof@kernel.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.
> >
> .....
>
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 0fcb1828e598..ad8183db0780 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -636,7 +636,6 @@ xfsaild(
> > unsigned int noreclaim_flag;
> >
> > noreclaim_flag = memalloc_noreclaim_save();
> > - set_freezable();
> >
> > while (1) {
> > /*
> > @@ -695,8 +694,6 @@ xfsaild(
> >
> > __set_current_state(TASK_RUNNING);
> >
> > - try_to_freeze();
> > -
> > tout = xfsaild_push(ailp);
> > }
> >
>
> So what about the TASK_FREEZABLE flag that is set in this code
> before sleeping?
>
> i.e. this code before we schedule():
>
> if (tout && tout <= 20)
> set_current_state(TASK_KILLABLE|TASK_FREEZABLE);
> else
> set_current_state(TASK_INTERRUPTIBLE|TASK_FREEZABLE);
>
> Shouldn't TASK_FREEZABLE go away, too?
Thanks for spotting! Yes, yesterday late at night I just took Luis
patches as they are and had only gotten around to testing btrfs. The
coccinelle scripts seemed to have missed those. I'll wait for comments
and will do another pass and send out v2.
> > diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> > index c5136ea9bb1d..1875b6551ab0 100644
> > --- a/fs/xfs/xfs_zone_gc.c
> > +++ b/fs/xfs/xfs_zone_gc.c
> > @@ -993,7 +993,6 @@ xfs_zone_gc_handle_work(
> > }
> >
> > __set_current_state(TASK_RUNNING);
> > - try_to_freeze();
> >
> > if (reset_list)
> > xfs_zone_gc_reset_zones(data, reset_list);
> > @@ -1041,7 +1040,6 @@ xfs_zoned_gcd(
> > unsigned int nofs_flag;
> >
> > nofs_flag = memalloc_nofs_save();
> > - set_freezable();
> >
> > for (;;) {
> > set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);
>
> Same question here for this newly merged code, too...
I'm not sure if this is supposed to be a snipe or not but just in case
this is a hidden question: This isn't merged. Per the cover letter this
is in a work.* branch. Anything that is considered mergable is in
vfs-6.16.* branches. But since we're pre -rc1 even those branches are
not yet showing up in -next.
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 3/6] xfs: replace kthread freezing with auto fs freezing
2025-04-01 7:17 ` Christian Brauner
@ 2025-04-01 11:35 ` Dave Chinner
2025-04-01 12:45 ` Christian Brauner
0 siblings, 1 reply; 59+ messages in thread
From: Dave Chinner @ 2025-04-01 11:35 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, rafael, djwong, pavel, peterz,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 09:17:12AM +0200, Christian Brauner wrote:
> On Tue, Apr 01, 2025 at 12:11:04PM +1100, Dave Chinner wrote:
> > On Tue, Apr 01, 2025 at 02:32:48AM +0200, Christian Brauner wrote:
> > > diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> > > index c5136ea9bb1d..1875b6551ab0 100644
> > > --- a/fs/xfs/xfs_zone_gc.c
> > > +++ b/fs/xfs/xfs_zone_gc.c
> > > @@ -993,7 +993,6 @@ xfs_zone_gc_handle_work(
> > > }
> > >
> > > __set_current_state(TASK_RUNNING);
> > > - try_to_freeze();
> > >
> > > if (reset_list)
> > > xfs_zone_gc_reset_zones(data, reset_list);
> > > @@ -1041,7 +1040,6 @@ xfs_zoned_gcd(
> > > unsigned int nofs_flag;
> > >
> > > nofs_flag = memalloc_nofs_save();
> > > - set_freezable();
> > >
> > > for (;;) {
> > > set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);
> >
> > Same question here for this newly merged code, too...
>
> I'm not sure if this is supposed to be a snipe or not but just in case
> this is a hidden question:
No, I meant that this is changing shiny new just-merged XFS code
(part of zone device support). It only just arrived this merge
window and is largely just doing the same thing as the older aild
code. It is probably safe to assume that this new code has never
been tested against hibernate...
-Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 3/6] xfs: replace kthread freezing with auto fs freezing
2025-04-01 11:35 ` Dave Chinner
@ 2025-04-01 12:45 ` Christian Brauner
0 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 12:45 UTC (permalink / raw)
To: Dave Chinner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, rafael, djwong, pavel, peterz,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 10:35:58PM +1100, Dave Chinner wrote:
> On Tue, Apr 01, 2025 at 09:17:12AM +0200, Christian Brauner wrote:
> > On Tue, Apr 01, 2025 at 12:11:04PM +1100, Dave Chinner wrote:
> > > On Tue, Apr 01, 2025 at 02:32:48AM +0200, Christian Brauner wrote:
> > > > diff --git a/fs/xfs/xfs_zone_gc.c b/fs/xfs/xfs_zone_gc.c
> > > > index c5136ea9bb1d..1875b6551ab0 100644
> > > > --- a/fs/xfs/xfs_zone_gc.c
> > > > +++ b/fs/xfs/xfs_zone_gc.c
> > > > @@ -993,7 +993,6 @@ xfs_zone_gc_handle_work(
> > > > }
> > > >
> > > > __set_current_state(TASK_RUNNING);
> > > > - try_to_freeze();
> > > >
> > > > if (reset_list)
> > > > xfs_zone_gc_reset_zones(data, reset_list);
> > > > @@ -1041,7 +1040,6 @@ xfs_zoned_gcd(
> > > > unsigned int nofs_flag;
> > > >
> > > > nofs_flag = memalloc_nofs_save();
> > > > - set_freezable();
> > > >
> > > > for (;;) {
> > > > set_current_state(TASK_INTERRUPTIBLE | TASK_FREEZABLE);
> > >
> > > Same question here for this newly merged code, too...
> >
> > I'm not sure if this is supposed to be a snipe or not but just in case
> > this is a hidden question:
>
> No, I meant that this is changing shiny new just-merged XFS code
> (part of zone device support). It only just arrived this merge
> window and is largely just doing the same thing as the older aild
> code. It is probably safe to assume that this new code has never
> been tested against hibernate...
Ah, my brain is completely fried. Apparently reading English is a skill
I've lost since coming back from Montreal. Thanks!
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH 4/6] fs: add owner of freeze/thaw
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (2 preceding siblings ...)
2025-04-01 0:32 ` [PATCH 3/6] xfs: " Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 0:32 ` [PATCH 5/6] fs: allow pagefault based writers to be frozen Christian Brauner
` (5 subsequent siblings)
9 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
For some kernel subsystems it is paramount that they are guaranteed that
they are the owner of the freeze to avoid any risk of deadlocks. This is
the case for the power subsystem. Enable it to recognize whether it did
actually freeze the filesystem.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/f2fs/gc.c | 6 ++--
fs/gfs2/super.c | 20 +++++++------
fs/gfs2/sys.c | 4 +--
fs/ioctl.c | 8 +++---
fs/super.c | 68 +++++++++++++++++++++++++++++++++++++--------
fs/xfs/scrub/fscounters.c | 4 +--
fs/xfs/xfs_notify_failure.c | 6 ++--
include/linux/fs.h | 13 ++++++---
8 files changed, 91 insertions(+), 38 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 2b8f9239bede..3e8af62c9e15 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -2271,12 +2271,12 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
if (err)
return err;
- err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
if (err)
return err;
if (f2fs_readonly(sbi->sb)) {
- err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
if (err)
return err;
return -EROFS;
@@ -2333,6 +2333,6 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
out_err:
f2fs_up_write(&sbi->cp_global_sem);
f2fs_up_write(&sbi->gc_lock);
- thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
return err;
}
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 44e5658b896c..519943189109 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -674,7 +674,7 @@ static int gfs2_sync_fs(struct super_block *sb, int wait)
return sdp->sd_log_error;
}
-static int gfs2_do_thaw(struct gfs2_sbd *sdp)
+static int gfs2_do_thaw(struct gfs2_sbd *sdp, enum freeze_holder who, const void *freeze_owner)
{
struct super_block *sb = sdp->sd_vfs;
int error;
@@ -682,7 +682,7 @@ static int gfs2_do_thaw(struct gfs2_sbd *sdp)
error = gfs2_freeze_lock_shared(sdp);
if (error)
goto fail;
- error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = thaw_super(sb, who, freeze_owner);
if (!error)
return 0;
@@ -703,14 +703,14 @@ void gfs2_freeze_func(struct work_struct *work)
if (test_bit(SDF_FROZEN, &sdp->sd_flags))
goto freeze_failed;
- error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
if (error)
goto freeze_failed;
gfs2_freeze_unlock(sdp);
set_bit(SDF_FROZEN, &sdp->sd_flags);
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, FREEZE_HOLDER_USERSPACE, NULL);
if (error)
goto out;
@@ -731,7 +731,8 @@ void gfs2_freeze_func(struct work_struct *work)
*
*/
-static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
+static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
int error;
@@ -744,7 +745,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
}
for (;;) {
- error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sb, who, freeze_owner);
if (error) {
fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n",
error);
@@ -758,7 +759,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
break;
}
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, who, freeze_owner);
if (error)
goto out;
@@ -799,7 +800,8 @@ static int gfs2_freeze_fs(struct super_block *sb)
*
*/
-static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
+static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
int error;
@@ -814,7 +816,7 @@ static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
atomic_inc(&sb->s_active);
gfs2_freeze_unlock(sdp);
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, who, freeze_owner);
if (!error) {
clear_bit(SDF_FREEZE_INITIATOR, &sdp->sd_flags);
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index ecc699f8d9fc..748125653d6c 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -174,10 +174,10 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
switch (n) {
case 0:
- error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
+ error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
break;
case 1:
- error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
break;
default:
return -EINVAL;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index c91fd2b46a77..bedc83fc2f20 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -396,8 +396,8 @@ static int ioctl_fsfreeze(struct file *filp)
/* Freeze */
if (sb->s_op->freeze_super)
- return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
- return freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+ return freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int ioctl_fsthaw(struct file *filp)
@@ -409,8 +409,8 @@ static int ioctl_fsthaw(struct file *filp)
/* Thaw */
if (sb->s_op->thaw_super)
- return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
- return thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+ return thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int ioctl_file_dedupe_range(struct file *file,
diff --git a/fs/super.c b/fs/super.c
index 3c4a496d6438..606072a3fab9 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -39,7 +39,8 @@
#include <uapi/linux/mount.h>
#include "internal.h"
-static int thaw_super_locked(struct super_block *sb, enum freeze_holder who);
+static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner);
static LIST_HEAD(super_blocks);
static DEFINE_SPINLOCK(sb_lock);
@@ -1148,7 +1149,7 @@ static void do_thaw_all_callback(struct super_block *sb, void *unused)
if (IS_ENABLED(CONFIG_BLOCK))
while (sb->s_bdev && !bdev_thaw(sb->s_bdev))
pr_warn("Emergency Thaw on %pg\n", sb->s_bdev);
- thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE, NULL);
return;
}
@@ -1522,10 +1523,10 @@ static int fs_bdev_freeze(struct block_device *bdev)
if (sb->s_op->freeze_super)
error = sb->s_op->freeze_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
else
error = freeze_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
if (!error)
error = sync_blockdev(bdev);
deactivate_super(sb);
@@ -1571,10 +1572,10 @@ static int fs_bdev_thaw(struct block_device *bdev)
if (sb->s_op->thaw_super)
error = sb->s_op->thaw_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
else
error = thaw_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
deactivate_super(sb);
return error;
}
@@ -1946,7 +1947,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
}
#define FREEZE_HOLDERS (FREEZE_HOLDER_KERNEL | FREEZE_HOLDER_USERSPACE)
-#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST)
+#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST | FREEZE_EXCL)
static inline int freeze_inc(struct super_block *sb, enum freeze_holder who)
{
@@ -1977,6 +1978,21 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
WARN_ON_ONCE((who & ~FREEZE_FLAGS));
WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+ if (who & FREEZE_EXCL) {
+ if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
+ return false;
+
+ if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ return false;
+
+ return (sb->s_writers.freeze_kcount +
+ sb->s_writers.freeze_ucount) == 0;
+ }
+
+ /* This filesystem is already exclusively frozen. */
+ if (sb->s_writers.freeze_owner)
+ return false;
+
if (who & FREEZE_HOLDER_KERNEL)
return (who & FREEZE_MAY_NEST) ||
sb->s_writers.freeze_kcount == 0;
@@ -1986,10 +2002,30 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
return false;
}
+static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
+{
+ WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+ WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+ if (who & FREEZE_EXCL) {
+ if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
+ return false;
+ if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
+ return false;
+ if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ return false;
+ return sb->s_writers.freeze_owner == freeze_owner;
+ }
+
+ return sb->s_writers.freeze_owner == NULL;
+}
+
/**
* freeze_super - lock the filesystem and force it into a consistent state
* @sb: the super to lock
* @who: context that wants to freeze
+ * @freeze_owner: owner of the freeze
*
* Syncs the super to make sure the filesystem is consistent and calls the fs's
* freeze_fs. Subsequent calls to this without first thawing the fs may return
@@ -2041,7 +2077,7 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
* Return: If the freeze was successful zero is returned. If the freeze
* failed a negative error code is returned.
*/
-int freeze_super(struct super_block *sb, enum freeze_holder who)
+int freeze_super(struct super_block *sb, enum freeze_holder who, const void *freeze_owner)
{
int ret;
@@ -2075,6 +2111,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
if (sb_rdonly(sb)) {
/* Nothing to do really... */
WARN_ON_ONCE(freeze_inc(sb, who) > 1);
+ sb->s_writers.freeze_owner = freeze_owner;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
wake_up_var(&sb->s_writers.frozen);
super_unlock_excl(sb);
@@ -2122,6 +2159,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
* when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
*/
WARN_ON_ONCE(freeze_inc(sb, who) > 1);
+ sb->s_writers.freeze_owner = freeze_owner;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
wake_up_var(&sb->s_writers.frozen);
lockdep_sb_freeze_release(sb);
@@ -2136,13 +2174,17 @@ EXPORT_SYMBOL(freeze_super);
* removes that state without releasing the other state or unlocking the
* filesystem.
*/
-static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
+static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
int error = -EINVAL;
if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
goto out_unlock;
+ if (!may_unfreeze(sb, who, freeze_owner))
+ goto out_unlock;
+
/*
* All freezers share a single active reference.
* So just unlock in case there are any left.
@@ -2152,6 +2194,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
if (sb_rdonly(sb)) {
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.freeze_owner = NULL;
wake_up_var(&sb->s_writers.frozen);
goto out_deactivate;
}
@@ -2169,6 +2212,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
}
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.freeze_owner = NULL;
wake_up_var(&sb->s_writers.frozen);
sb_freeze_unlock(sb, SB_FREEZE_FS);
out_deactivate:
@@ -2184,6 +2228,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
* thaw_super -- unlock filesystem
* @sb: the super to thaw
* @who: context that wants to freeze
+ * @freeze_owner: owner of the freeze
*
* Unlocks the filesystem and marks it writeable again after freeze_super()
* if there are no remaining freezes on the filesystem.
@@ -2197,13 +2242,14 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
* have been frozen through the block layer via multiple block devices.
* The filesystem remains frozen until all block devices are unfrozen.
*/
-int thaw_super(struct super_block *sb, enum freeze_holder who)
+int thaw_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
if (!super_lock_excl(sb)) {
WARN_ON_ONCE("Dying superblock while thawing!");
return -EINVAL;
}
- return thaw_super_locked(sb, who);
+ return thaw_super_locked(sb, who, freeze_owner);
}
EXPORT_SYMBOL(thaw_super);
diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
index e629663e460a..9b598c5790ad 100644
--- a/fs/xfs/scrub/fscounters.c
+++ b/fs/xfs/scrub/fscounters.c
@@ -123,7 +123,7 @@ xchk_fsfreeze(
{
int error;
- error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
+ error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
trace_xchk_fsfreeze(sc, error);
return error;
}
@@ -135,7 +135,7 @@ xchk_fsthaw(
int error;
/* This should always succeed, we have a kernel freeze */
- error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
+ error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
trace_xchk_fsthaw(sc, error);
return error;
}
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index ed8d8ed42f0a..3545dc1d953c 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -127,7 +127,7 @@ xfs_dax_notify_failure_freeze(
struct super_block *sb = mp->m_super;
int error;
- error = freeze_super(sb, FREEZE_HOLDER_KERNEL);
+ error = freeze_super(sb, FREEZE_HOLDER_KERNEL, NULL);
if (error)
xfs_emerg(mp, "already frozen by kernel, err=%d", error);
@@ -143,7 +143,7 @@ xfs_dax_notify_failure_thaw(
int error;
if (kernel_frozen) {
- error = thaw_super(sb, FREEZE_HOLDER_KERNEL);
+ error = thaw_super(sb, FREEZE_HOLDER_KERNEL, NULL);
if (error)
xfs_emerg(mp, "still frozen after notify failure, err=%d",
error);
@@ -153,7 +153,7 @@ xfs_dax_notify_failure_thaw(
* Also thaw userspace call anyway because the device is about to be
* removed immediately.
*/
- thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1aa578412f1b..b379a46b5576 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1307,6 +1307,7 @@ struct sb_writers {
unsigned short frozen; /* Is sb frozen? */
int freeze_kcount; /* How many kernel freeze requests? */
int freeze_ucount; /* How many userspace freeze requests? */
+ const void *freeze_owner; /* Owner of the freeze */
struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
};
@@ -2270,6 +2271,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
* @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
* @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
* @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
+ * @FREEZE_EXCL: whether actual freezing must be done by the caller
*
* Indicate who the owner of the freeze or thaw request is and whether
* the freeze needs to be exclusive or can nest.
@@ -2283,6 +2285,7 @@ enum freeze_holder {
FREEZE_HOLDER_KERNEL = (1U << 0),
FREEZE_HOLDER_USERSPACE = (1U << 1),
FREEZE_MAY_NEST = (1U << 2),
+ FREEZE_EXCL = (1U << 3),
};
struct super_operations {
@@ -2296,9 +2299,9 @@ struct super_operations {
void (*evict_inode) (struct inode *);
void (*put_super) (struct super_block *);
int (*sync_fs)(struct super_block *sb, int wait);
- int (*freeze_super) (struct super_block *, enum freeze_holder who);
+ int (*freeze_super) (struct super_block *, enum freeze_holder who, const void *owner);
int (*freeze_fs) (struct super_block *);
- int (*thaw_super) (struct super_block *, enum freeze_holder who);
+ int (*thaw_super) (struct super_block *, enum freeze_holder who, const void *owner);
int (*unfreeze_fs) (struct super_block *);
int (*statfs) (struct dentry *, struct kstatfs *);
int (*remount_fs) (struct super_block *, int *, char *);
@@ -2706,8 +2709,10 @@ extern int unregister_filesystem(struct file_system_type *);
extern int vfs_statfs(const struct path *, struct kstatfs *);
extern int user_statfs(const char __user *, struct kstatfs *);
extern int fd_statfs(int, struct kstatfs *);
-int freeze_super(struct super_block *super, enum freeze_holder who);
-int thaw_super(struct super_block *super, enum freeze_holder who);
+int freeze_super(struct super_block *super, enum freeze_holder who,
+ const void *freeze_owner);
+int thaw_super(struct super_block *super, enum freeze_holder who,
+ const void *freeze_owner);
extern __printf(2, 3)
int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
extern int super_setup_bdi(struct super_block *sb);
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 5/6] fs: allow pagefault based writers to be frozen
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (3 preceding siblings ...)
2025-04-01 0:32 ` [PATCH 4/6] fs: add owner of freeze/thaw Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 0:32 ` [PATCH 6/6] power: freeze filesystems during suspend/resume Christian Brauner
` (4 subsequent siblings)
9 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
Otherwise tasks such as systemd-journald that mmap a file and write to
it will not be frozen after we've frozen the filesystem.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/fs.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b379a46b5576..528e73f192ac 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1782,7 +1782,8 @@ static inline void __sb_end_write(struct super_block *sb, int level)
static inline void __sb_start_write(struct super_block *sb, int level)
{
percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1,
- level == SB_FREEZE_WRITE);
+ (level == SB_FREEZE_WRITE ||
+ level == SB_FREEZE_PAGEFAULT));
}
static inline bool __sb_start_write_trylock(struct super_block *sb, int level)
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* [PATCH 6/6] power: freeze filesystems during suspend/resume
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (4 preceding siblings ...)
2025-04-01 0:32 ` [PATCH 5/6] fs: allow pagefault based writers to be frozen Christian Brauner
@ 2025-04-01 0:32 ` Christian Brauner
2025-04-01 8:16 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (3 subsequent siblings)
9 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 0:32 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
Now all the pieces are in place to actually allow the power subsystem
to freeze/thaw filesystems during suspend/resume. Filesystems are only
frozen and thawed if the power subsystem does actually own the freeze.
Othwerwise it risks thawing filesystems it didn't own. This could be
done differently be e.g., keepin the filesystems that were actually
frozen on a list and then unfreezing them from that list. This is
disgustingly unclean though and reeks of an ugly hack.
If the filesystem is already frozen by the time we've frozen all
userspace processes we don't care to freeze it again. That's userspace's
job once the process resumes. We only actually freeze filesystems if we
absolutely have to and we ignore other failures to freeze for now.
We could bubble up errors and fail suspend/resume if the error isn't
EBUSY (aka it's already frozen) but I don't think that this is worth it.
Filesystem freezing during suspend/resume is best-effort. If the user
has 500 ext4 filesystems mounted and 4 fail to freeze for whatever
reason then we simply skip them.
What we have now is already a big improvement and let's see how we fare
with it before making our lives even harder (and uglier) than we have
to.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/super.c | 14 ++++++++++----
kernel/power/hibernate.c | 13 ++++++++++++-
kernel/power/suspend.c | 8 ++++++++
3 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 606072a3fab9..dd0d6def4a55 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1187,6 +1187,8 @@ static inline bool get_active_super(struct super_block *sb)
return active;
}
+static const void *filesystems_freeze_ptr;
+
static void filesystems_freeze_callback(struct super_block *sb, void *unused)
{
if (!sb->s_op->freeze_fs && !sb->s_op->freeze_super)
@@ -1196,9 +1198,11 @@ static void filesystems_freeze_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->freeze_super)
- sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ sb->s_op->freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
else
- freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
deactivate_super(sb);
}
@@ -1218,9 +1222,11 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->thaw_super)
- sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ sb->s_op->thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
else
- thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
deactivate_super(sb);
}
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 50ec26ea696b..1803b7d24757 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -777,6 +777,7 @@ int hibernate(void)
goto Restore;
ksys_sync_helper();
+ filesystems_freeze();
error = freeze_processes();
if (error)
@@ -841,6 +842,7 @@ int hibernate(void)
error = load_image_and_restore();
}
thaw_processes();
+ filesystems_thaw();
/* Don't bother checking whether freezer_test_done is true */
freezer_test_done = false;
@@ -881,6 +883,8 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
if (error)
goto restore;
+ filesystems_freeze();
+
error = freeze_processes();
if (error)
goto exit;
@@ -940,6 +944,7 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
thaw_processes();
exit:
+ filesystems_thaw();
pm_notifier_call_chain(PM_POST_HIBERNATION);
restore:
@@ -1028,19 +1033,25 @@ static int software_resume(void)
if (error)
goto Restore;
+ filesystems_freeze();
+
pm_pr_dbg("Preparing processes for hibernation restore.\n");
error = freeze_processes();
- if (error)
+ if (error) {
+ filesystems_thaw();
goto Close_Finish;
+ }
error = freeze_kernel_threads();
if (error) {
thaw_processes();
+ filesystems_thaw();
goto Close_Finish;
}
error = load_image_and_restore();
thaw_processes();
+ filesystems_thaw();
Finish:
pm_notifier_call_chain(PM_POST_RESTORE);
Restore:
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8eaec4ab121d..4c476271f7f2 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -30,6 +30,7 @@
#include <trace/events/power.h>
#include <linux/compiler.h>
#include <linux/moduleparam.h>
+#include <linux/fs.h>
#include "power.h"
@@ -374,6 +375,8 @@ static int suspend_prepare(suspend_state_t state)
if (error)
goto Restore;
+ if (sync_on_suspend_enabled)
+ filesystems_freeze();
trace_suspend_resume(TPS("freeze_processes"), 0, true);
error = suspend_freeze_processes();
trace_suspend_resume(TPS("freeze_processes"), 0, false);
@@ -550,6 +553,8 @@ int suspend_devices_and_enter(suspend_state_t state)
static void suspend_finish(void)
{
suspend_thaw_processes();
+ if (sync_on_suspend_enabled)
+ filesystems_thaw();
pm_notifier_call_chain(PM_POST_SUSPEND);
pm_restore_console();
}
@@ -587,6 +592,7 @@ static int enter_state(suspend_state_t state)
trace_suspend_resume(TPS("sync_filesystems"), 0, true);
ksys_sync_helper();
trace_suspend_resume(TPS("sync_filesystems"), 0, false);
+ filesystems_freeze();
}
pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
@@ -609,6 +615,8 @@ static int enter_state(suspend_state_t state)
pm_pr_dbg("Finishing wakeup.\n");
suspend_finish();
Unlock:
+ if (sync_on_suspend_enabled)
+ filesystems_thaw();
mutex_unlock(&system_transition_mutex);
return error;
}
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (5 preceding siblings ...)
2025-04-01 0:32 ` [PATCH 6/6] power: freeze filesystems during suspend/resume Christian Brauner
@ 2025-04-01 8:16 ` Christian Brauner
2025-04-01 9:32 ` Jan Kara
` (2 subsequent siblings)
9 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 8:16 UTC (permalink / raw)
To: jack, rafael
Cc: Ard Biesheuvel, linux-efi, linux-kernel, James Bottomley, mcgrof,
hch, david, djwong, pavel, peterz, mingo, will, boqun.feng,
linux-fsdevel
On Tue, Apr 01, 2025 at 02:32:45AM +0200, Christian Brauner wrote:
> The whole shebang can also be found at:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
>
> I know nothing about power or hibernation.
I would like to place this behind a Kconfig option and add a
/sys/power/freeze_on_suspend option as these changes are pretty
sensitive and to give userspace the ability to experiment with this for
a while until we remove it. That means we should skip the removal of all
the freezer changes in the filesystems until we're happy enough that
this works reliable enough.
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (6 preceding siblings ...)
2025-04-01 8:16 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
@ 2025-04-01 9:32 ` Jan Kara
2025-04-01 13:03 ` Christian Brauner
2025-04-01 14:14 ` [PATCH 0/6] " Peter Zijlstra
2025-04-01 17:02 ` James Bottomley
9 siblings, 1 reply; 59+ messages in thread
From: Jan Kara @ 2025-04-01 9:32 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, djwong, pavel,
peterz, mingo, will, boqun.feng
On Tue 01-04-25 02:32:45, Christian Brauner wrote:
> The whole shebang can also be found at:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
>
> I know nothing about power or hibernation. I've tested it as best as I
> could. Works for me (TM).
>
> I need to catch some actual sleep now...
>
> ---
>
> Now all the pieces are in place to actually allow the power subsystem to
> freeze/thaw filesystems during suspend/resume. Filesystems are only
> frozen and thawed if the power subsystem does actually own the freeze.
>
> Othwerwise it risks thawing filesystems it didn't own. This could be
> done differently be e.g., keeping the filesystems that were actually
> frozen on a list and then unfreezing them from that list. This is
> disgustingly unclean though and reeks of an ugly hack.
>
> If the filesystem is already frozen by the time we've frozen all
> userspace processes we don't care to freeze it again. That's userspace's
> job once the process resumes. We only actually freeze filesystems if we
> absolutely have to and we ignore other failures to freeze.
Hum, I don't follow here. I supposed we'll use FREEZE_MAY_NEST |
FREEZE_HOLDER_KERNEL for freezing from power subsystem. As far as I
remember we have specifically designed nesting of freeze counters so that
this way power subsystem can be sure freezing succeeds even if the
filesystem is already frozen (by userspace or the kernel) and similarly
power subsystem cannot thaw a filesystem frozen by somebody else. It will
just drop its freeze refcount... What am I missing?
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 9:32 ` Jan Kara
@ 2025-04-01 13:03 ` Christian Brauner
2025-04-01 16:57 ` Jan Kara
0 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 13:03 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, rafael, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, Apr 01, 2025 at 11:32:49AM +0200, Jan Kara wrote:
> On Tue 01-04-25 02:32:45, Christian Brauner wrote:
> > The whole shebang can also be found at:
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> >
> > I know nothing about power or hibernation. I've tested it as best as I
> > could. Works for me (TM).
> >
> > I need to catch some actual sleep now...
> >
> > ---
> >
> > Now all the pieces are in place to actually allow the power subsystem to
> > freeze/thaw filesystems during suspend/resume. Filesystems are only
> > frozen and thawed if the power subsystem does actually own the freeze.
> >
> > Othwerwise it risks thawing filesystems it didn't own. This could be
> > done differently be e.g., keeping the filesystems that were actually
> > frozen on a list and then unfreezing them from that list. This is
> > disgustingly unclean though and reeks of an ugly hack.
> >
> > If the filesystem is already frozen by the time we've frozen all
> > userspace processes we don't care to freeze it again. That's userspace's
> > job once the process resumes. We only actually freeze filesystems if we
> > absolutely have to and we ignore other failures to freeze.
>
> Hum, I don't follow here. I supposed we'll use FREEZE_MAY_NEST |
> FREEZE_HOLDER_KERNEL for freezing from power subsystem. As far as I
> remember we have specifically designed nesting of freeze counters so that
> this way power subsystem can be sure freezing succeeds even if the
> filesystem is already frozen (by userspace or the kernel) and similarly
> power subsystem cannot thaw a filesystem frozen by somebody else. It will
> just drop its freeze refcount... What am I missing?
If we have 10 filesystems and suspend/hibernate manges to freeze 5 and
then fails on the 6th for whatever odd reason (current or future) then
power needs to undo the freeze of the first 5 filesystems. We can't just
walk the list again because while it's unlikely that a new filesystem
got added in the meantime we still cannot tell what filesystems the
power subsystem actually managed to get a freeze reference count on that
we need to drop during thaw.
There's various ways out of this ugliness. Either we record the
filesystems the power subsystem managed to freeze on a temporary list in
the callbacks and then walk that list backwards during thaw to undo the
freezing or we make sure that the power subsystem just actually
exclusively freezes things it can freeze and marking such filesystems as
being owned by power for the duration of the suspend or resume cycle. I
opted for the latter as that seemed the clean thing to do even if it
means more code changes. What are your thoughts on this?
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 13:03 ` Christian Brauner
@ 2025-04-01 16:57 ` Jan Kara
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
0 siblings, 1 reply; 59+ messages in thread
From: Jan Kara @ 2025-04-01 16:57 UTC (permalink / raw)
To: Christian Brauner
Cc: Jan Kara, linux-fsdevel, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, djwong, pavel,
peterz, mingo, will, boqun.feng
On Tue 01-04-25 15:03:33, Christian Brauner wrote:
> On Tue, Apr 01, 2025 at 11:32:49AM +0200, Jan Kara wrote:
> > On Tue 01-04-25 02:32:45, Christian Brauner wrote:
> > > The whole shebang can also be found at:
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> > >
> > > I know nothing about power or hibernation. I've tested it as best as I
> > > could. Works for me (TM).
> > >
> > > I need to catch some actual sleep now...
> > >
> > > ---
> > >
> > > Now all the pieces are in place to actually allow the power subsystem to
> > > freeze/thaw filesystems during suspend/resume. Filesystems are only
> > > frozen and thawed if the power subsystem does actually own the freeze.
> > >
> > > Othwerwise it risks thawing filesystems it didn't own. This could be
> > > done differently be e.g., keeping the filesystems that were actually
> > > frozen on a list and then unfreezing them from that list. This is
> > > disgustingly unclean though and reeks of an ugly hack.
> > >
> > > If the filesystem is already frozen by the time we've frozen all
> > > userspace processes we don't care to freeze it again. That's userspace's
> > > job once the process resumes. We only actually freeze filesystems if we
> > > absolutely have to and we ignore other failures to freeze.
> >
> > Hum, I don't follow here. I supposed we'll use FREEZE_MAY_NEST |
> > FREEZE_HOLDER_KERNEL for freezing from power subsystem. As far as I
> > remember we have specifically designed nesting of freeze counters so that
> > this way power subsystem can be sure freezing succeeds even if the
> > filesystem is already frozen (by userspace or the kernel) and similarly
> > power subsystem cannot thaw a filesystem frozen by somebody else. It will
> > just drop its freeze refcount... What am I missing?
>
> If we have 10 filesystems and suspend/hibernate manges to freeze 5 and
> then fails on the 6th for whatever odd reason (current or future) then
> power needs to undo the freeze of the first 5 filesystems. We can't just
> walk the list again because while it's unlikely that a new filesystem
> got added in the meantime we still cannot tell what filesystems the
> power subsystem actually managed to get a freeze reference count on that
> we need to drop during thaw.
>
> There's various ways out of this ugliness. Either we record the
> filesystems the power subsystem managed to freeze on a temporary list in
> the callbacks and then walk that list backwards during thaw to undo the
> freezing or we make sure that the power subsystem just actually
> exclusively freezes things it can freeze and marking such filesystems as
> being owned by power for the duration of the suspend or resume cycle. I
> opted for the latter as that seemed the clean thing to do even if it
> means more code changes. What are your thoughts on this?
Ah, I see. Thanks for explanation. So failure to freeze filesystem should
be rare (mostly only due to IO errors or similar serious issues) hence
I'd consider failing hibernation in case we fail to freeze some filesystem
appropriate. The function that's walking all superblocks and freezing them
could just walk from the superblock where freezing failed towards the end
and thaw all filesystems. That way the function also has the nice property
that it either freezes everything or keeps things as they were.
But you've touched on an interesting case I didn't consider: New
superblocks can be added to the end of the list while we are walking it.
These superblocks will not be frozen and on resume (or error recovery) this
will confuse things. Your "freeze owner" stuff deals with this problem
nicely. Somewhat lighter fix for this may be to provide the superblock to
start from / end with to these loops iterating and freezing / thawing
superblocks. It doesn't seem too hacky but if you prefer your freeze owner
approach I won't object.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 16:57 ` Jan Kara
@ 2025-04-02 14:07 ` Christian Brauner
2025-04-02 14:07 ` [PATCH v2 1/4] fs: add owner of freeze/thaw Christian Brauner
` (4 more replies)
0 siblings, 5 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 14:07 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
Now all the pieces are in place to actually allow the power subsystem to
freeze/thaw filesystems during suspend/resume. Filesystems are only
frozen and thawed if the power subsystem does actually own the freeze.
Othwerwise it risks thawing filesystems it didn't own. This could be
done differently be e.g., keeping the filesystems that were actually
frozen on a list and then unfreezing them from that list. This is
disgustingly unclean though and reeks of an ugly hack.
If the filesystem is already frozen by the time we've frozen all
userspace processes we don't care to freeze it again. That's userspace's
job once the process resumes. We only actually freeze filesystems if we
absolutely have to and we ignore other failures to freeze.
We could bubble up errors and fail suspend/resume if the error isn't
EBUSY (aka it's already frozen) but I don't think that this is worth it.
Filesystem freezing during suspend/resume is best-effort. If the user
has 500 ext4 filesystems mounted and 4 fail to freeze for whatever
reason then we simply skip them.
What we have now is already a big improvement and let's see how we fare
with it before making our lives even harder (and uglier) than we have
to.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
Changes in v2:
- Drop all patches that remove TASK_FREEZABLE.
- Expand commit messages a bit.
- Link to v1: https://lore.kernel.org/r/20250401-work-freeze-v1-0-d000611d4ab0@kernel.org
---
Christian Brauner (4):
fs: add owner of freeze/thaw
fs: allow all writers to be frozen
power: freeze filesystems during suspend/resume
kernfs: add warning about implementing freeze/thaw
fs/f2fs/gc.c | 6 ++--
fs/gfs2/super.c | 20 ++++++-----
fs/gfs2/sys.c | 4 +--
fs/ioctl.c | 8 ++---
fs/kernfs/mount.c | 15 +++++++++
fs/super.c | 82 ++++++++++++++++++++++++++++++++++++---------
fs/xfs/scrub/fscounters.c | 4 +--
fs/xfs/xfs_notify_failure.c | 6 ++--
include/linux/fs.h | 16 +++++----
kernel/power/hibernate.c | 16 ++++++++-
kernel/power/main.c | 31 +++++++++++++++++
kernel/power/power.h | 4 +++
kernel/power/suspend.c | 7 ++++
13 files changed, 174 insertions(+), 45 deletions(-)
---
base-commit: 62dfd8d59e2d16873398ede5b1835e302df789b3
change-id: 20250401-work-freeze-693b5b5a78e0
^ permalink raw reply [flat|nested] 59+ messages in thread* [PATCH v2 1/4] fs: add owner of freeze/thaw
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
@ 2025-04-02 14:07 ` Christian Brauner
2025-04-03 14:56 ` Jan Kara
2025-04-02 14:07 ` [PATCH v2 2/4] fs: allow all writers to be frozen Christian Brauner
` (3 subsequent siblings)
4 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 14:07 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
For some kernel subsystems it is paramount that they are guaranteed that
they are the owner of the freeze to avoid any risk of deadlocks. This is
the case for the power subsystem. Enable it to recognize whether it did
actually freeze the filesystem.
If userspace has 10 filesystems and suspend/hibernate manges to freeze 5
and then fails on the 6th for whatever odd reason (current or future)
then power needs to undo the freeze of the first 5 filesystems. It can't
just walk the list again because while it's unlikely that a new
filesystem got added in the meantime it still cannot tell which
filesystems the power subsystem actually managed to get a freeze
reference count on that needs to be dropped during thaw.
There's various ways out of this ugliness. For example, record the
filesystems the power subsystem managed to freeze on a temporary list in
the callbacks and then walk that list backwards during thaw to undo the
freezing or make sure that the power subsystem just actually exclusively
freezes things it can freeze and marking such filesystems as being owned
by power for the duration of the suspend or resume cycle. I opted for
the latter as that seemed the clean thing to do even if it means more
code changes.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/f2fs/gc.c | 6 ++--
fs/gfs2/super.c | 20 ++++++------
fs/gfs2/sys.c | 4 +--
fs/ioctl.c | 8 ++---
fs/super.c | 76 ++++++++++++++++++++++++++++++++++++---------
fs/xfs/scrub/fscounters.c | 4 +--
fs/xfs/xfs_notify_failure.c | 6 ++--
include/linux/fs.h | 13 +++++---
8 files changed, 95 insertions(+), 42 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 2b8f9239bede..3e8af62c9e15 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -2271,12 +2271,12 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
if (err)
return err;
- err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
if (err)
return err;
if (f2fs_readonly(sbi->sb)) {
- err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
if (err)
return err;
return -EROFS;
@@ -2333,6 +2333,6 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
out_err:
f2fs_up_write(&sbi->cp_global_sem);
f2fs_up_write(&sbi->gc_lock);
- thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
return err;
}
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 44e5658b896c..519943189109 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -674,7 +674,7 @@ static int gfs2_sync_fs(struct super_block *sb, int wait)
return sdp->sd_log_error;
}
-static int gfs2_do_thaw(struct gfs2_sbd *sdp)
+static int gfs2_do_thaw(struct gfs2_sbd *sdp, enum freeze_holder who, const void *freeze_owner)
{
struct super_block *sb = sdp->sd_vfs;
int error;
@@ -682,7 +682,7 @@ static int gfs2_do_thaw(struct gfs2_sbd *sdp)
error = gfs2_freeze_lock_shared(sdp);
if (error)
goto fail;
- error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = thaw_super(sb, who, freeze_owner);
if (!error)
return 0;
@@ -703,14 +703,14 @@ void gfs2_freeze_func(struct work_struct *work)
if (test_bit(SDF_FROZEN, &sdp->sd_flags))
goto freeze_failed;
- error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
if (error)
goto freeze_failed;
gfs2_freeze_unlock(sdp);
set_bit(SDF_FROZEN, &sdp->sd_flags);
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, FREEZE_HOLDER_USERSPACE, NULL);
if (error)
goto out;
@@ -731,7 +731,8 @@ void gfs2_freeze_func(struct work_struct *work)
*
*/
-static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
+static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
int error;
@@ -744,7 +745,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
}
for (;;) {
- error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sb, who, freeze_owner);
if (error) {
fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n",
error);
@@ -758,7 +759,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
break;
}
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, who, freeze_owner);
if (error)
goto out;
@@ -799,7 +800,8 @@ static int gfs2_freeze_fs(struct super_block *sb)
*
*/
-static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
+static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
struct gfs2_sbd *sdp = sb->s_fs_info;
int error;
@@ -814,7 +816,7 @@ static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
atomic_inc(&sb->s_active);
gfs2_freeze_unlock(sdp);
- error = gfs2_do_thaw(sdp);
+ error = gfs2_do_thaw(sdp, who, freeze_owner);
if (!error) {
clear_bit(SDF_FREEZE_INITIATOR, &sdp->sd_flags);
diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
index ecc699f8d9fc..748125653d6c 100644
--- a/fs/gfs2/sys.c
+++ b/fs/gfs2/sys.c
@@ -174,10 +174,10 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
switch (n) {
case 0:
- error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
+ error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
break;
case 1:
- error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
+ error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
break;
default:
return -EINVAL;
diff --git a/fs/ioctl.c b/fs/ioctl.c
index c91fd2b46a77..bedc83fc2f20 100644
--- a/fs/ioctl.c
+++ b/fs/ioctl.c
@@ -396,8 +396,8 @@ static int ioctl_fsfreeze(struct file *filp)
/* Freeze */
if (sb->s_op->freeze_super)
- return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
- return freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+ return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+ return freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int ioctl_fsthaw(struct file *filp)
@@ -409,8 +409,8 @@ static int ioctl_fsthaw(struct file *filp)
/* Thaw */
if (sb->s_op->thaw_super)
- return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
- return thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
+ return thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int ioctl_file_dedupe_range(struct file *file,
diff --git a/fs/super.c b/fs/super.c
index 3c4a496d6438..3ddded4360c6 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -39,7 +39,8 @@
#include <uapi/linux/mount.h>
#include "internal.h"
-static int thaw_super_locked(struct super_block *sb, enum freeze_holder who);
+static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner);
static LIST_HEAD(super_blocks);
static DEFINE_SPINLOCK(sb_lock);
@@ -1148,7 +1149,7 @@ static void do_thaw_all_callback(struct super_block *sb, void *unused)
if (IS_ENABLED(CONFIG_BLOCK))
while (sb->s_bdev && !bdev_thaw(sb->s_bdev))
pr_warn("Emergency Thaw on %pg\n", sb->s_bdev);
- thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE, NULL);
return;
}
@@ -1195,9 +1196,9 @@ static void filesystems_freeze_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->freeze_super)
- sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
else
- freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
deactivate_super(sb);
}
@@ -1217,9 +1218,9 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->thaw_super)
- sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
else
- thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
+ thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
deactivate_super(sb);
}
@@ -1522,10 +1523,10 @@ static int fs_bdev_freeze(struct block_device *bdev)
if (sb->s_op->freeze_super)
error = sb->s_op->freeze_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
else
error = freeze_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
if (!error)
error = sync_blockdev(bdev);
deactivate_super(sb);
@@ -1571,10 +1572,10 @@ static int fs_bdev_thaw(struct block_device *bdev)
if (sb->s_op->thaw_super)
error = sb->s_op->thaw_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
else
error = thaw_super(sb,
- FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
+ FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
deactivate_super(sb);
return error;
}
@@ -1946,7 +1947,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
}
#define FREEZE_HOLDERS (FREEZE_HOLDER_KERNEL | FREEZE_HOLDER_USERSPACE)
-#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST)
+#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST | FREEZE_EXCL)
static inline int freeze_inc(struct super_block *sb, enum freeze_holder who)
{
@@ -1977,6 +1978,21 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
WARN_ON_ONCE((who & ~FREEZE_FLAGS));
WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+ if (who & FREEZE_EXCL) {
+ if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
+ return false;
+
+ if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ return false;
+
+ return (sb->s_writers.freeze_kcount +
+ sb->s_writers.freeze_ucount) == 0;
+ }
+
+ /* This filesystem is already exclusively frozen. */
+ if (sb->s_writers.freeze_owner)
+ return false;
+
if (who & FREEZE_HOLDER_KERNEL)
return (who & FREEZE_MAY_NEST) ||
sb->s_writers.freeze_kcount == 0;
@@ -1986,10 +2002,30 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
return false;
}
+static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
+{
+ WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+ WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+ if (who & FREEZE_EXCL) {
+ if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
+ return false;
+ if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
+ return false;
+ if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ return false;
+ return sb->s_writers.freeze_owner == freeze_owner;
+ }
+
+ return sb->s_writers.freeze_owner == NULL;
+}
+
/**
* freeze_super - lock the filesystem and force it into a consistent state
* @sb: the super to lock
* @who: context that wants to freeze
+ * @freeze_owner: owner of the freeze
*
* Syncs the super to make sure the filesystem is consistent and calls the fs's
* freeze_fs. Subsequent calls to this without first thawing the fs may return
@@ -2041,7 +2077,7 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
* Return: If the freeze was successful zero is returned. If the freeze
* failed a negative error code is returned.
*/
-int freeze_super(struct super_block *sb, enum freeze_holder who)
+int freeze_super(struct super_block *sb, enum freeze_holder who, const void *freeze_owner)
{
int ret;
@@ -2075,6 +2111,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
if (sb_rdonly(sb)) {
/* Nothing to do really... */
WARN_ON_ONCE(freeze_inc(sb, who) > 1);
+ sb->s_writers.freeze_owner = freeze_owner;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
wake_up_var(&sb->s_writers.frozen);
super_unlock_excl(sb);
@@ -2122,6 +2159,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
* when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
*/
WARN_ON_ONCE(freeze_inc(sb, who) > 1);
+ sb->s_writers.freeze_owner = freeze_owner;
sb->s_writers.frozen = SB_FREEZE_COMPLETE;
wake_up_var(&sb->s_writers.frozen);
lockdep_sb_freeze_release(sb);
@@ -2136,13 +2174,17 @@ EXPORT_SYMBOL(freeze_super);
* removes that state without releasing the other state or unlocking the
* filesystem.
*/
-static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
+static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
int error = -EINVAL;
if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
goto out_unlock;
+ if (!may_unfreeze(sb, who, freeze_owner))
+ goto out_unlock;
+
/*
* All freezers share a single active reference.
* So just unlock in case there are any left.
@@ -2152,6 +2194,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
if (sb_rdonly(sb)) {
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.freeze_owner = NULL;
wake_up_var(&sb->s_writers.frozen);
goto out_deactivate;
}
@@ -2169,6 +2212,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
}
sb->s_writers.frozen = SB_UNFROZEN;
+ sb->s_writers.freeze_owner = NULL;
wake_up_var(&sb->s_writers.frozen);
sb_freeze_unlock(sb, SB_FREEZE_FS);
out_deactivate:
@@ -2184,6 +2228,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
* thaw_super -- unlock filesystem
* @sb: the super to thaw
* @who: context that wants to freeze
+ * @freeze_owner: owner of the freeze
*
* Unlocks the filesystem and marks it writeable again after freeze_super()
* if there are no remaining freezes on the filesystem.
@@ -2197,13 +2242,14 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
* have been frozen through the block layer via multiple block devices.
* The filesystem remains frozen until all block devices are unfrozen.
*/
-int thaw_super(struct super_block *sb, enum freeze_holder who)
+int thaw_super(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
if (!super_lock_excl(sb)) {
WARN_ON_ONCE("Dying superblock while thawing!");
return -EINVAL;
}
- return thaw_super_locked(sb, who);
+ return thaw_super_locked(sb, who, freeze_owner);
}
EXPORT_SYMBOL(thaw_super);
diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
index e629663e460a..9b598c5790ad 100644
--- a/fs/xfs/scrub/fscounters.c
+++ b/fs/xfs/scrub/fscounters.c
@@ -123,7 +123,7 @@ xchk_fsfreeze(
{
int error;
- error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
+ error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
trace_xchk_fsfreeze(sc, error);
return error;
}
@@ -135,7 +135,7 @@ xchk_fsthaw(
int error;
/* This should always succeed, we have a kernel freeze */
- error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
+ error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
trace_xchk_fsthaw(sc, error);
return error;
}
diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
index ed8d8ed42f0a..3545dc1d953c 100644
--- a/fs/xfs/xfs_notify_failure.c
+++ b/fs/xfs/xfs_notify_failure.c
@@ -127,7 +127,7 @@ xfs_dax_notify_failure_freeze(
struct super_block *sb = mp->m_super;
int error;
- error = freeze_super(sb, FREEZE_HOLDER_KERNEL);
+ error = freeze_super(sb, FREEZE_HOLDER_KERNEL, NULL);
if (error)
xfs_emerg(mp, "already frozen by kernel, err=%d", error);
@@ -143,7 +143,7 @@ xfs_dax_notify_failure_thaw(
int error;
if (kernel_frozen) {
- error = thaw_super(sb, FREEZE_HOLDER_KERNEL);
+ error = thaw_super(sb, FREEZE_HOLDER_KERNEL, NULL);
if (error)
xfs_emerg(mp, "still frozen after notify failure, err=%d",
error);
@@ -153,7 +153,7 @@ xfs_dax_notify_failure_thaw(
* Also thaw userspace call anyway because the device is about to be
* removed immediately.
*/
- thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+ thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
}
static int
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1aa578412f1b..b379a46b5576 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1307,6 +1307,7 @@ struct sb_writers {
unsigned short frozen; /* Is sb frozen? */
int freeze_kcount; /* How many kernel freeze requests? */
int freeze_ucount; /* How many userspace freeze requests? */
+ const void *freeze_owner; /* Owner of the freeze */
struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
};
@@ -2270,6 +2271,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
* @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
* @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
* @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
+ * @FREEZE_EXCL: whether actual freezing must be done by the caller
*
* Indicate who the owner of the freeze or thaw request is and whether
* the freeze needs to be exclusive or can nest.
@@ -2283,6 +2285,7 @@ enum freeze_holder {
FREEZE_HOLDER_KERNEL = (1U << 0),
FREEZE_HOLDER_USERSPACE = (1U << 1),
FREEZE_MAY_NEST = (1U << 2),
+ FREEZE_EXCL = (1U << 3),
};
struct super_operations {
@@ -2296,9 +2299,9 @@ struct super_operations {
void (*evict_inode) (struct inode *);
void (*put_super) (struct super_block *);
int (*sync_fs)(struct super_block *sb, int wait);
- int (*freeze_super) (struct super_block *, enum freeze_holder who);
+ int (*freeze_super) (struct super_block *, enum freeze_holder who, const void *owner);
int (*freeze_fs) (struct super_block *);
- int (*thaw_super) (struct super_block *, enum freeze_holder who);
+ int (*thaw_super) (struct super_block *, enum freeze_holder who, const void *owner);
int (*unfreeze_fs) (struct super_block *);
int (*statfs) (struct dentry *, struct kstatfs *);
int (*remount_fs) (struct super_block *, int *, char *);
@@ -2706,8 +2709,10 @@ extern int unregister_filesystem(struct file_system_type *);
extern int vfs_statfs(const struct path *, struct kstatfs *);
extern int user_statfs(const char __user *, struct kstatfs *);
extern int fd_statfs(int, struct kstatfs *);
-int freeze_super(struct super_block *super, enum freeze_holder who);
-int thaw_super(struct super_block *super, enum freeze_holder who);
+int freeze_super(struct super_block *super, enum freeze_holder who,
+ const void *freeze_owner);
+int thaw_super(struct super_block *super, enum freeze_holder who,
+ const void *freeze_owner);
extern __printf(2, 3)
int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
extern int super_setup_bdi(struct super_block *sb);
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 1/4] fs: add owner of freeze/thaw
2025-04-02 14:07 ` [PATCH v2 1/4] fs: add owner of freeze/thaw Christian Brauner
@ 2025-04-03 14:56 ` Jan Kara
2025-04-03 19:33 ` Christian Brauner
2025-04-04 10:24 ` [PATCH] fs: allow nesting with FREEZE_EXCL Christian Brauner
0 siblings, 2 replies; 59+ messages in thread
From: Jan Kara @ 2025-04-03 14:56 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Wed 02-04-25 16:07:31, Christian Brauner wrote:
> For some kernel subsystems it is paramount that they are guaranteed that
> they are the owner of the freeze to avoid any risk of deadlocks. This is
> the case for the power subsystem. Enable it to recognize whether it did
> actually freeze the filesystem.
>
> If userspace has 10 filesystems and suspend/hibernate manges to freeze 5
> and then fails on the 6th for whatever odd reason (current or future)
> then power needs to undo the freeze of the first 5 filesystems. It can't
> just walk the list again because while it's unlikely that a new
> filesystem got added in the meantime it still cannot tell which
> filesystems the power subsystem actually managed to get a freeze
> reference count on that needs to be dropped during thaw.
>
> There's various ways out of this ugliness. For example, record the
> filesystems the power subsystem managed to freeze on a temporary list in
> the callbacks and then walk that list backwards during thaw to undo the
> freezing or make sure that the power subsystem just actually exclusively
> freezes things it can freeze and marking such filesystems as being owned
> by power for the duration of the suspend or resume cycle. I opted for
> the latter as that seemed the clean thing to do even if it means more
> code changes.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
I have realized a slight catch with this approach that if hibernation races
with filesystem freezing (e.g. DM reconfiguration), then hibernation need
not freeze a filesystem because it's already frozen but userspace may thaw
the filesystem before hibernation actually happens (relatively harmless).
If the race happens the other way around, DM reconfiguration may
unexpectedly fail with EBUSY (rather unexpected). So somehow tracking which
fs was frozen by suspend while properly nesting with other freeze users may
be actually a better approach (maybe just a sb flag even though it's
somewhat hacky?).
Honza
> ---
> fs/f2fs/gc.c | 6 ++--
> fs/gfs2/super.c | 20 ++++++------
> fs/gfs2/sys.c | 4 +--
> fs/ioctl.c | 8 ++---
> fs/super.c | 76 ++++++++++++++++++++++++++++++++++++---------
> fs/xfs/scrub/fscounters.c | 4 +--
> fs/xfs/xfs_notify_failure.c | 6 ++--
> include/linux/fs.h | 13 +++++---
> 8 files changed, 95 insertions(+), 42 deletions(-)
>
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 2b8f9239bede..3e8af62c9e15 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -2271,12 +2271,12 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
> if (err)
> return err;
>
> - err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
> + err = freeze_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
> if (err)
> return err;
>
> if (f2fs_readonly(sbi->sb)) {
> - err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
> + err = thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
> if (err)
> return err;
> return -EROFS;
> @@ -2333,6 +2333,6 @@ int f2fs_resize_fs(struct file *filp, __u64 block_count)
> out_err:
> f2fs_up_write(&sbi->cp_global_sem);
> f2fs_up_write(&sbi->gc_lock);
> - thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE);
> + thaw_super(sbi->sb, FREEZE_HOLDER_USERSPACE, NULL);
> return err;
> }
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 44e5658b896c..519943189109 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -674,7 +674,7 @@ static int gfs2_sync_fs(struct super_block *sb, int wait)
> return sdp->sd_log_error;
> }
>
> -static int gfs2_do_thaw(struct gfs2_sbd *sdp)
> +static int gfs2_do_thaw(struct gfs2_sbd *sdp, enum freeze_holder who, const void *freeze_owner)
> {
> struct super_block *sb = sdp->sd_vfs;
> int error;
> @@ -682,7 +682,7 @@ static int gfs2_do_thaw(struct gfs2_sbd *sdp)
> error = gfs2_freeze_lock_shared(sdp);
> if (error)
> goto fail;
> - error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> + error = thaw_super(sb, who, freeze_owner);
> if (!error)
> return 0;
>
> @@ -703,14 +703,14 @@ void gfs2_freeze_func(struct work_struct *work)
> if (test_bit(SDF_FROZEN, &sdp->sd_flags))
> goto freeze_failed;
>
> - error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> + error = freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> if (error)
> goto freeze_failed;
>
> gfs2_freeze_unlock(sdp);
> set_bit(SDF_FROZEN, &sdp->sd_flags);
>
> - error = gfs2_do_thaw(sdp);
> + error = gfs2_do_thaw(sdp, FREEZE_HOLDER_USERSPACE, NULL);
> if (error)
> goto out;
>
> @@ -731,7 +731,8 @@ void gfs2_freeze_func(struct work_struct *work)
> *
> */
>
> -static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
> +static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> struct gfs2_sbd *sdp = sb->s_fs_info;
> int error;
> @@ -744,7 +745,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
> }
>
> for (;;) {
> - error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> + error = freeze_super(sb, who, freeze_owner);
> if (error) {
> fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n",
> error);
> @@ -758,7 +759,7 @@ static int gfs2_freeze_super(struct super_block *sb, enum freeze_holder who)
> break;
> }
>
> - error = gfs2_do_thaw(sdp);
> + error = gfs2_do_thaw(sdp, who, freeze_owner);
> if (error)
> goto out;
>
> @@ -799,7 +800,8 @@ static int gfs2_freeze_fs(struct super_block *sb)
> *
> */
>
> -static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
> +static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> struct gfs2_sbd *sdp = sb->s_fs_info;
> int error;
> @@ -814,7 +816,7 @@ static int gfs2_thaw_super(struct super_block *sb, enum freeze_holder who)
> atomic_inc(&sb->s_active);
> gfs2_freeze_unlock(sdp);
>
> - error = gfs2_do_thaw(sdp);
> + error = gfs2_do_thaw(sdp, who, freeze_owner);
>
> if (!error) {
> clear_bit(SDF_FREEZE_INITIATOR, &sdp->sd_flags);
> diff --git a/fs/gfs2/sys.c b/fs/gfs2/sys.c
> index ecc699f8d9fc..748125653d6c 100644
> --- a/fs/gfs2/sys.c
> +++ b/fs/gfs2/sys.c
> @@ -174,10 +174,10 @@ static ssize_t freeze_store(struct gfs2_sbd *sdp, const char *buf, size_t len)
>
> switch (n) {
> case 0:
> - error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
> + error = thaw_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
> break;
> case 1:
> - error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE);
> + error = freeze_super(sdp->sd_vfs, FREEZE_HOLDER_USERSPACE, NULL);
> break;
> default:
> return -EINVAL;
> diff --git a/fs/ioctl.c b/fs/ioctl.c
> index c91fd2b46a77..bedc83fc2f20 100644
> --- a/fs/ioctl.c
> +++ b/fs/ioctl.c
> @@ -396,8 +396,8 @@ static int ioctl_fsfreeze(struct file *filp)
>
> /* Freeze */
> if (sb->s_op->freeze_super)
> - return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> - return freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> + return sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> + return freeze_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> }
>
> static int ioctl_fsthaw(struct file *filp)
> @@ -409,8 +409,8 @@ static int ioctl_fsthaw(struct file *filp)
>
> /* Thaw */
> if (sb->s_op->thaw_super)
> - return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> - return thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> + return sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> + return thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> }
>
> static int ioctl_file_dedupe_range(struct file *file,
> diff --git a/fs/super.c b/fs/super.c
> index 3c4a496d6438..3ddded4360c6 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -39,7 +39,8 @@
> #include <uapi/linux/mount.h>
> #include "internal.h"
>
> -static int thaw_super_locked(struct super_block *sb, enum freeze_holder who);
> +static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner);
>
> static LIST_HEAD(super_blocks);
> static DEFINE_SPINLOCK(sb_lock);
> @@ -1148,7 +1149,7 @@ static void do_thaw_all_callback(struct super_block *sb, void *unused)
> if (IS_ENABLED(CONFIG_BLOCK))
> while (sb->s_bdev && !bdev_thaw(sb->s_bdev))
> pr_warn("Emergency Thaw on %pg\n", sb->s_bdev);
> - thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
> + thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE, NULL);
> return;
> }
>
> @@ -1195,9 +1196,9 @@ static void filesystems_freeze_callback(struct super_block *sb, void *unused)
> return;
>
> if (sb->s_op->freeze_super)
> - sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
> + sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> else
> - freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
> + freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
>
> deactivate_super(sb);
> }
> @@ -1217,9 +1218,9 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
> return;
>
> if (sb->s_op->thaw_super)
> - sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
> + sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> else
> - thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL);
> + thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
>
> deactivate_super(sb);
> }
> @@ -1522,10 +1523,10 @@ static int fs_bdev_freeze(struct block_device *bdev)
>
> if (sb->s_op->freeze_super)
> error = sb->s_op->freeze_super(sb,
> - FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
> + FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
> else
> error = freeze_super(sb,
> - FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
> + FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
> if (!error)
> error = sync_blockdev(bdev);
> deactivate_super(sb);
> @@ -1571,10 +1572,10 @@ static int fs_bdev_thaw(struct block_device *bdev)
>
> if (sb->s_op->thaw_super)
> error = sb->s_op->thaw_super(sb,
> - FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
> + FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
> else
> error = thaw_super(sb,
> - FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE);
> + FREEZE_MAY_NEST | FREEZE_HOLDER_USERSPACE, NULL);
> deactivate_super(sb);
> return error;
> }
> @@ -1946,7 +1947,7 @@ static int wait_for_partially_frozen(struct super_block *sb)
> }
>
> #define FREEZE_HOLDERS (FREEZE_HOLDER_KERNEL | FREEZE_HOLDER_USERSPACE)
> -#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST)
> +#define FREEZE_FLAGS (FREEZE_HOLDERS | FREEZE_MAY_NEST | FREEZE_EXCL)
>
> static inline int freeze_inc(struct super_block *sb, enum freeze_holder who)
> {
> @@ -1977,6 +1978,21 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
>
> + if (who & FREEZE_EXCL) {
> + if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> + return false;
> +
> + if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + return false;
> +
> + return (sb->s_writers.freeze_kcount +
> + sb->s_writers.freeze_ucount) == 0;
> + }
> +
> + /* This filesystem is already exclusively frozen. */
> + if (sb->s_writers.freeze_owner)
> + return false;
> +
> if (who & FREEZE_HOLDER_KERNEL)
> return (who & FREEZE_MAY_NEST) ||
> sb->s_writers.freeze_kcount == 0;
> @@ -1986,10 +2002,30 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> return false;
> }
>
> +static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> +{
> + WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> + WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
> +
> + if (who & FREEZE_EXCL) {
> + if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
> + return false;
> + if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> + return false;
> + if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + return false;
> + return sb->s_writers.freeze_owner == freeze_owner;
> + }
> +
> + return sb->s_writers.freeze_owner == NULL;
> +}
> +
> /**
> * freeze_super - lock the filesystem and force it into a consistent state
> * @sb: the super to lock
> * @who: context that wants to freeze
> + * @freeze_owner: owner of the freeze
> *
> * Syncs the super to make sure the filesystem is consistent and calls the fs's
> * freeze_fs. Subsequent calls to this without first thawing the fs may return
> @@ -2041,7 +2077,7 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> * Return: If the freeze was successful zero is returned. If the freeze
> * failed a negative error code is returned.
> */
> -int freeze_super(struct super_block *sb, enum freeze_holder who)
> +int freeze_super(struct super_block *sb, enum freeze_holder who, const void *freeze_owner)
> {
> int ret;
>
> @@ -2075,6 +2111,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
> if (sb_rdonly(sb)) {
> /* Nothing to do really... */
> WARN_ON_ONCE(freeze_inc(sb, who) > 1);
> + sb->s_writers.freeze_owner = freeze_owner;
> sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> wake_up_var(&sb->s_writers.frozen);
> super_unlock_excl(sb);
> @@ -2122,6 +2159,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
> * when frozen is set to SB_FREEZE_COMPLETE, and for thaw_super().
> */
> WARN_ON_ONCE(freeze_inc(sb, who) > 1);
> + sb->s_writers.freeze_owner = freeze_owner;
> sb->s_writers.frozen = SB_FREEZE_COMPLETE;
> wake_up_var(&sb->s_writers.frozen);
> lockdep_sb_freeze_release(sb);
> @@ -2136,13 +2174,17 @@ EXPORT_SYMBOL(freeze_super);
> * removes that state without releasing the other state or unlocking the
> * filesystem.
> */
> -static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
> +static int thaw_super_locked(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> int error = -EINVAL;
>
> if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
> goto out_unlock;
>
> + if (!may_unfreeze(sb, who, freeze_owner))
> + goto out_unlock;
> +
> /*
> * All freezers share a single active reference.
> * So just unlock in case there are any left.
> @@ -2152,6 +2194,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>
> if (sb_rdonly(sb)) {
> sb->s_writers.frozen = SB_UNFROZEN;
> + sb->s_writers.freeze_owner = NULL;
> wake_up_var(&sb->s_writers.frozen);
> goto out_deactivate;
> }
> @@ -2169,6 +2212,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
> }
>
> sb->s_writers.frozen = SB_UNFROZEN;
> + sb->s_writers.freeze_owner = NULL;
> wake_up_var(&sb->s_writers.frozen);
> sb_freeze_unlock(sb, SB_FREEZE_FS);
> out_deactivate:
> @@ -2184,6 +2228,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
> * thaw_super -- unlock filesystem
> * @sb: the super to thaw
> * @who: context that wants to freeze
> + * @freeze_owner: owner of the freeze
> *
> * Unlocks the filesystem and marks it writeable again after freeze_super()
> * if there are no remaining freezes on the filesystem.
> @@ -2197,13 +2242,14 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
> * have been frozen through the block layer via multiple block devices.
> * The filesystem remains frozen until all block devices are unfrozen.
> */
> -int thaw_super(struct super_block *sb, enum freeze_holder who)
> +int thaw_super(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> if (!super_lock_excl(sb)) {
> WARN_ON_ONCE("Dying superblock while thawing!");
> return -EINVAL;
> }
> - return thaw_super_locked(sb, who);
> + return thaw_super_locked(sb, who, freeze_owner);
> }
> EXPORT_SYMBOL(thaw_super);
>
> diff --git a/fs/xfs/scrub/fscounters.c b/fs/xfs/scrub/fscounters.c
> index e629663e460a..9b598c5790ad 100644
> --- a/fs/xfs/scrub/fscounters.c
> +++ b/fs/xfs/scrub/fscounters.c
> @@ -123,7 +123,7 @@ xchk_fsfreeze(
> {
> int error;
>
> - error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
> + error = freeze_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
> trace_xchk_fsfreeze(sc, error);
> return error;
> }
> @@ -135,7 +135,7 @@ xchk_fsthaw(
> int error;
>
> /* This should always succeed, we have a kernel freeze */
> - error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL);
> + error = thaw_super(sc->mp->m_super, FREEZE_HOLDER_KERNEL, NULL);
> trace_xchk_fsthaw(sc, error);
> return error;
> }
> diff --git a/fs/xfs/xfs_notify_failure.c b/fs/xfs/xfs_notify_failure.c
> index ed8d8ed42f0a..3545dc1d953c 100644
> --- a/fs/xfs/xfs_notify_failure.c
> +++ b/fs/xfs/xfs_notify_failure.c
> @@ -127,7 +127,7 @@ xfs_dax_notify_failure_freeze(
> struct super_block *sb = mp->m_super;
> int error;
>
> - error = freeze_super(sb, FREEZE_HOLDER_KERNEL);
> + error = freeze_super(sb, FREEZE_HOLDER_KERNEL, NULL);
> if (error)
> xfs_emerg(mp, "already frozen by kernel, err=%d", error);
>
> @@ -143,7 +143,7 @@ xfs_dax_notify_failure_thaw(
> int error;
>
> if (kernel_frozen) {
> - error = thaw_super(sb, FREEZE_HOLDER_KERNEL);
> + error = thaw_super(sb, FREEZE_HOLDER_KERNEL, NULL);
> if (error)
> xfs_emerg(mp, "still frozen after notify failure, err=%d",
> error);
> @@ -153,7 +153,7 @@ xfs_dax_notify_failure_thaw(
> * Also thaw userspace call anyway because the device is about to be
> * removed immediately.
> */
> - thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> + thaw_super(sb, FREEZE_HOLDER_USERSPACE, NULL);
> }
>
> static int
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1aa578412f1b..b379a46b5576 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1307,6 +1307,7 @@ struct sb_writers {
> unsigned short frozen; /* Is sb frozen? */
> int freeze_kcount; /* How many kernel freeze requests? */
> int freeze_ucount; /* How many userspace freeze requests? */
> + const void *freeze_owner; /* Owner of the freeze */
> struct percpu_rw_semaphore rw_sem[SB_FREEZE_LEVELS];
> };
>
> @@ -2270,6 +2271,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
> * @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
> * @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
> * @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
> + * @FREEZE_EXCL: whether actual freezing must be done by the caller
> *
> * Indicate who the owner of the freeze or thaw request is and whether
> * the freeze needs to be exclusive or can nest.
> @@ -2283,6 +2285,7 @@ enum freeze_holder {
> FREEZE_HOLDER_KERNEL = (1U << 0),
> FREEZE_HOLDER_USERSPACE = (1U << 1),
> FREEZE_MAY_NEST = (1U << 2),
> + FREEZE_EXCL = (1U << 3),
> };
>
> struct super_operations {
> @@ -2296,9 +2299,9 @@ struct super_operations {
> void (*evict_inode) (struct inode *);
> void (*put_super) (struct super_block *);
> int (*sync_fs)(struct super_block *sb, int wait);
> - int (*freeze_super) (struct super_block *, enum freeze_holder who);
> + int (*freeze_super) (struct super_block *, enum freeze_holder who, const void *owner);
> int (*freeze_fs) (struct super_block *);
> - int (*thaw_super) (struct super_block *, enum freeze_holder who);
> + int (*thaw_super) (struct super_block *, enum freeze_holder who, const void *owner);
> int (*unfreeze_fs) (struct super_block *);
> int (*statfs) (struct dentry *, struct kstatfs *);
> int (*remount_fs) (struct super_block *, int *, char *);
> @@ -2706,8 +2709,10 @@ extern int unregister_filesystem(struct file_system_type *);
> extern int vfs_statfs(const struct path *, struct kstatfs *);
> extern int user_statfs(const char __user *, struct kstatfs *);
> extern int fd_statfs(int, struct kstatfs *);
> -int freeze_super(struct super_block *super, enum freeze_holder who);
> -int thaw_super(struct super_block *super, enum freeze_holder who);
> +int freeze_super(struct super_block *super, enum freeze_holder who,
> + const void *freeze_owner);
> +int thaw_super(struct super_block *super, enum freeze_holder who,
> + const void *freeze_owner);
> extern __printf(2, 3)
> int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
> extern int super_setup_bdi(struct super_block *sb);
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 1/4] fs: add owner of freeze/thaw
2025-04-03 14:56 ` Jan Kara
@ 2025-04-03 19:33 ` Christian Brauner
2025-04-04 10:24 ` [PATCH] fs: allow nesting with FREEZE_EXCL Christian Brauner
1 sibling, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-03 19:33 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Thu, Apr 03, 2025 at 04:56:57PM +0200, Jan Kara wrote:
> On Wed 02-04-25 16:07:31, Christian Brauner wrote:
> > For some kernel subsystems it is paramount that they are guaranteed that
> > they are the owner of the freeze to avoid any risk of deadlocks. This is
> > the case for the power subsystem. Enable it to recognize whether it did
> > actually freeze the filesystem.
> >
> > If userspace has 10 filesystems and suspend/hibernate manges to freeze 5
> > and then fails on the 6th for whatever odd reason (current or future)
> > then power needs to undo the freeze of the first 5 filesystems. It can't
> > just walk the list again because while it's unlikely that a new
> > filesystem got added in the meantime it still cannot tell which
> > filesystems the power subsystem actually managed to get a freeze
> > reference count on that needs to be dropped during thaw.
> >
> > There's various ways out of this ugliness. For example, record the
> > filesystems the power subsystem managed to freeze on a temporary list in
> > the callbacks and then walk that list backwards during thaw to undo the
> > freezing or make sure that the power subsystem just actually exclusively
> > freezes things it can freeze and marking such filesystems as being owned
> > by power for the duration of the suspend or resume cycle. I opted for
> > the latter as that seemed the clean thing to do even if it means more
> > code changes.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
>
> I have realized a slight catch with this approach that if hibernation races
> with filesystem freezing (e.g. DM reconfiguration), then hibernation need
> not freeze a filesystem because it's already frozen but userspace may thaw
> the filesystem before hibernation actually happens (relatively harmless).
> If the race happens the other way around, DM reconfiguration may
> unexpectedly fail with EBUSY (rather unexpected). So somehow tracking which
> fs was frozen by suspend while properly nesting with other freeze users may
> be actually a better approach (maybe just a sb flag even though it's
> somewhat hacky?).
The approach that I originally had was to add FREEZE_POWER which adds a
simple boolean into the sb_writers instead of a holder and then this
simply nests with the rest. I'll try to post that diff tomorrow.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH] fs: allow nesting with FREEZE_EXCL
2025-04-03 14:56 ` Jan Kara
2025-04-03 19:33 ` Christian Brauner
@ 2025-04-04 10:24 ` Christian Brauner
2025-04-07 9:08 ` Christoph Hellwig
2025-05-07 11:18 ` Jan Kara
1 sibling, 2 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-04 10:24 UTC (permalink / raw)
To: Jan Kara
Cc: Christian Brauner, linux-fsdevel, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, rafael, djwong,
pavel, peterz, mingo, will, boqun.feng
If hibernation races with filesystem freezing (e.g. DM reconfiguration),
then hibernation need not freeze a filesystem because it's already
frozen but userspace may thaw the filesystem before hibernation actually
happens.
If the race happens the other way around, DM reconfiguration may
unexpectedly fail with EBUSY.
So allow FREEZE_EXCL to nest with other holders. An exclusive freezer
cannot be undone by any of the other concurrent freezers.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/super.c | 71 ++++++++++++++++++++++++++++++++++++++++++------------
include/linux/fs.h | 2 +-
2 files changed, 56 insertions(+), 17 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index b4bdbc509dba..e2fee655fbed 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1979,26 +1979,34 @@ static inline int freeze_dec(struct super_block *sb, enum freeze_holder who)
return sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount;
}
-static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
+static inline bool may_freeze(struct super_block *sb, enum freeze_holder who,
+ const void *freeze_owner)
{
+ lockdep_assert_held(&sb->s_umount);
+
WARN_ON_ONCE((who & ~FREEZE_FLAGS));
WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
if (who & FREEZE_EXCL) {
if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
return false;
-
- if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
return false;
-
- return (sb->s_writers.freeze_kcount +
- sb->s_writers.freeze_ucount) == 0;
+ if (WARN_ON_ONCE(!freeze_owner))
+ return false;
+ /* This freeze already has a specific owner. */
+ if (sb->s_writers.freeze_owner)
+ return false;
+ /*
+ * This is already frozen multiple times so we're just
+ * going to take a reference count and mark it as
+ * belonging to use.
+ */
+ if (sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount)
+ sb->s_writers.freeze_owner = freeze_owner;
+ return true;
}
- /* This filesystem is already exclusively frozen. */
- if (sb->s_writers.freeze_owner)
- return false;
-
if (who & FREEZE_HOLDER_KERNEL)
return (who & FREEZE_MAY_NEST) ||
sb->s_writers.freeze_kcount == 0;
@@ -2011,20 +2019,51 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
const void *freeze_owner)
{
+ lockdep_assert_held(&sb->s_umount);
+
WARN_ON_ONCE((who & ~FREEZE_FLAGS));
WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
if (who & FREEZE_EXCL) {
- if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
- return false;
if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
return false;
- if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
+ if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
+ return false;
+ if (WARN_ON_ONCE(!freeze_owner))
+ return false;
+ if (WARN_ON_ONCE(sb->s_writers.freeze_kcount == 0))
return false;
- return sb->s_writers.freeze_owner == freeze_owner;
+ /* This isn't exclusively frozen. */
+ if (!sb->s_writers.freeze_owner)
+ return false;
+ /* This isn't exclusively frozen by us. */
+ if (sb->s_writers.freeze_owner != freeze_owner)
+ return false;
+ /*
+ * This is still frozen multiple times so we're just
+ * going to drop our reference count and undo our
+ * exclusive freeze.
+ */
+ if ((sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount) > 1)
+ sb->s_writers.freeze_owner = NULL;
+ return true;
+ }
+
+ if (who & FREEZE_HOLDER_KERNEL) {
+ /*
+ * Someone's trying to steal the reference belonging to
+ * @sb->s_writers.freeze_owner.
+ */
+ if (sb->s_writers.freeze_kcount == 1 &&
+ sb->s_writers.freeze_owner)
+ return false;
+ return sb->s_writers.freeze_kcount > 0;
}
- return sb->s_writers.freeze_owner == NULL;
+ if (who & FREEZE_HOLDER_USERSPACE)
+ return sb->s_writers.freeze_ucount > 0;
+
+ return false;
}
/**
@@ -2095,7 +2134,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
retry:
if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
- if (may_freeze(sb, who))
+ if (may_freeze(sb, who, freeze_owner))
ret = !!WARN_ON_ONCE(freeze_inc(sb, who) == 1);
else
ret = -EBUSY;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 1edcba3cd68e..7a3f821d2723 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2270,7 +2270,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
* @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
* @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
* @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
- * @FREEZE_EXCL: whether actual freezing must be done by the caller
+ * @FREEZE_EXCL: a freeze that can only be undone by the owner
*
* Indicate who the owner of the freeze or thaw request is and whether
* the freeze needs to be exclusive or can nest.
---
base-commit: a83fe97e0d53f7d2b0fc62fd9a322a963cb30306
change-id: 20250404-work-freeze-5eacb515f044
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH] fs: allow nesting with FREEZE_EXCL
2025-04-04 10:24 ` [PATCH] fs: allow nesting with FREEZE_EXCL Christian Brauner
@ 2025-04-07 9:08 ` Christoph Hellwig
2025-05-07 11:18 ` Jan Kara
1 sibling, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2025-04-07 9:08 UTC (permalink / raw)
To: Christian Brauner
Cc: Jan Kara, linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Fri, Apr 04, 2025 at 12:24:09PM +0200, Christian Brauner wrote:
> If hibernation races with filesystem freezing (e.g. DM reconfiguration),
> then hibernation need not freeze a filesystem because it's already
> frozen but userspace may thaw the filesystem before hibernation actually
> happens.
>
> If the race happens the other way around, DM reconfiguration may
> unexpectedly fail with EBUSY.
>
> So allow FREEZE_EXCL to nest with other holders. An exclusive freezer
> cannot be undone by any of the other concurrent freezers.
What is FREEZE_EXCL? I can't find it anywhere including in linux-next.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH] fs: allow nesting with FREEZE_EXCL
2025-04-04 10:24 ` [PATCH] fs: allow nesting with FREEZE_EXCL Christian Brauner
2025-04-07 9:08 ` Christoph Hellwig
@ 2025-05-07 11:18 ` Jan Kara
2025-05-09 10:38 ` Christian Brauner
1 sibling, 1 reply; 59+ messages in thread
From: Jan Kara @ 2025-05-07 11:18 UTC (permalink / raw)
To: Christian Brauner
Cc: Jan Kara, linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Fri 04-04-25 12:24:09, Christian Brauner wrote:
> If hibernation races with filesystem freezing (e.g. DM reconfiguration),
> then hibernation need not freeze a filesystem because it's already
> frozen but userspace may thaw the filesystem before hibernation actually
> happens.
>
> If the race happens the other way around, DM reconfiguration may
> unexpectedly fail with EBUSY.
>
> So allow FREEZE_EXCL to nest with other holders. An exclusive freezer
> cannot be undone by any of the other concurrent freezers.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
This has fallen through the cracks in my inbox but the patch now looks good
to me. Maybe we should fold it into "fs: add owner of freeze/thaw" to not
have strange intermediate state in the series?
Honza
> ---
> fs/super.c | 71 ++++++++++++++++++++++++++++++++++++++++++------------
> include/linux/fs.h | 2 +-
> 2 files changed, 56 insertions(+), 17 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index b4bdbc509dba..e2fee655fbed 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1979,26 +1979,34 @@ static inline int freeze_dec(struct super_block *sb, enum freeze_holder who)
> return sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount;
> }
>
> -static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> +static inline bool may_freeze(struct super_block *sb, enum freeze_holder who,
> + const void *freeze_owner)
> {
> + lockdep_assert_held(&sb->s_umount);
> +
> WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
>
> if (who & FREEZE_EXCL) {
> if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> return false;
> -
> - if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
> return false;
> -
> - return (sb->s_writers.freeze_kcount +
> - sb->s_writers.freeze_ucount) == 0;
> + if (WARN_ON_ONCE(!freeze_owner))
> + return false;
> + /* This freeze already has a specific owner. */
> + if (sb->s_writers.freeze_owner)
> + return false;
> + /*
> + * This is already frozen multiple times so we're just
> + * going to take a reference count and mark it as
> + * belonging to use.
> + */
> + if (sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount)
> + sb->s_writers.freeze_owner = freeze_owner;
> + return true;
> }
>
> - /* This filesystem is already exclusively frozen. */
> - if (sb->s_writers.freeze_owner)
> - return false;
> -
> if (who & FREEZE_HOLDER_KERNEL)
> return (who & FREEZE_MAY_NEST) ||
> sb->s_writers.freeze_kcount == 0;
> @@ -2011,20 +2019,51 @@ static inline bool may_freeze(struct super_block *sb, enum freeze_holder who)
> static inline bool may_unfreeze(struct super_block *sb, enum freeze_holder who,
> const void *freeze_owner)
> {
> + lockdep_assert_held(&sb->s_umount);
> +
> WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
>
> if (who & FREEZE_EXCL) {
> - if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
> - return false;
> if (WARN_ON_ONCE(!(who & FREEZE_HOLDER_KERNEL)))
> return false;
> - if (who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL))
> + if (WARN_ON_ONCE(who & ~(FREEZE_EXCL | FREEZE_HOLDER_KERNEL)))
> + return false;
> + if (WARN_ON_ONCE(!freeze_owner))
> + return false;
> + if (WARN_ON_ONCE(sb->s_writers.freeze_kcount == 0))
> return false;
> - return sb->s_writers.freeze_owner == freeze_owner;
> + /* This isn't exclusively frozen. */
> + if (!sb->s_writers.freeze_owner)
> + return false;
> + /* This isn't exclusively frozen by us. */
> + if (sb->s_writers.freeze_owner != freeze_owner)
> + return false;
> + /*
> + * This is still frozen multiple times so we're just
> + * going to drop our reference count and undo our
> + * exclusive freeze.
> + */
> + if ((sb->s_writers.freeze_kcount + sb->s_writers.freeze_ucount) > 1)
> + sb->s_writers.freeze_owner = NULL;
> + return true;
> + }
> +
> + if (who & FREEZE_HOLDER_KERNEL) {
> + /*
> + * Someone's trying to steal the reference belonging to
> + * @sb->s_writers.freeze_owner.
> + */
> + if (sb->s_writers.freeze_kcount == 1 &&
> + sb->s_writers.freeze_owner)
> + return false;
> + return sb->s_writers.freeze_kcount > 0;
> }
>
> - return sb->s_writers.freeze_owner == NULL;
> + if (who & FREEZE_HOLDER_USERSPACE)
> + return sb->s_writers.freeze_ucount > 0;
> +
> + return false;
> }
>
> /**
> @@ -2095,7 +2134,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who, const void *fre
>
> retry:
> if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> - if (may_freeze(sb, who))
> + if (may_freeze(sb, who, freeze_owner))
> ret = !!WARN_ON_ONCE(freeze_inc(sb, who) == 1);
> else
> ret = -EBUSY;
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 1edcba3cd68e..7a3f821d2723 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2270,7 +2270,7 @@ extern loff_t vfs_dedupe_file_range_one(struct file *src_file, loff_t src_pos,
> * @FREEZE_HOLDER_KERNEL: kernel wants to freeze or thaw filesystem
> * @FREEZE_HOLDER_USERSPACE: userspace wants to freeze or thaw filesystem
> * @FREEZE_MAY_NEST: whether nesting freeze and thaw requests is allowed
> - * @FREEZE_EXCL: whether actual freezing must be done by the caller
> + * @FREEZE_EXCL: a freeze that can only be undone by the owner
> *
> * Indicate who the owner of the freeze or thaw request is and whether
> * the freeze needs to be exclusive or can nest.
>
> ---
> base-commit: a83fe97e0d53f7d2b0fc62fd9a322a963cb30306
> change-id: 20250404-work-freeze-5eacb515f044
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH] fs: allow nesting with FREEZE_EXCL
2025-05-07 11:18 ` Jan Kara
@ 2025-05-09 10:38 ` Christian Brauner
0 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-05-09 10:38 UTC (permalink / raw)
To: Jan Kara
Cc: linux-fsdevel, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Wed, May 07, 2025 at 01:18:34PM +0200, Jan Kara wrote:
> On Fri 04-04-25 12:24:09, Christian Brauner wrote:
> > If hibernation races with filesystem freezing (e.g. DM reconfiguration),
> > then hibernation need not freeze a filesystem because it's already
> > frozen but userspace may thaw the filesystem before hibernation actually
> > happens.
> >
> > If the race happens the other way around, DM reconfiguration may
> > unexpectedly fail with EBUSY.
> >
> > So allow FREEZE_EXCL to nest with other holders. An exclusive freezer
> > cannot be undone by any of the other concurrent freezers.
> >
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
>
> This has fallen through the cracks in my inbox but the patch now looks good
> to me. Maybe we should fold it into "fs: add owner of freeze/thaw" to not
> have strange intermediate state in the series?
Done.
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 2/4] fs: allow all writers to be frozen
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
2025-04-02 14:07 ` [PATCH v2 1/4] fs: add owner of freeze/thaw Christian Brauner
@ 2025-04-02 14:07 ` Christian Brauner
2025-04-02 15:32 ` Christian Brauner
2025-04-03 14:59 ` Jan Kara
2025-04-02 14:07 ` [PATCH v2 3/4] power: freeze filesystems during suspend/resume Christian Brauner
` (2 subsequent siblings)
4 siblings, 2 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 14:07 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
During freeze/thaw we need to be able to freeze all writers during
suspend/hibernate. Otherwise tasks such as systemd-journald that mmap a
file and write to it will not be frozen after we've already frozen the
filesystem.
This has some risk of not being able to freeze processes in case a
process has acquired SB_FREEZE_PAGEFAULT under mmap_sem or
SB_FREEZE_INTERNAL under some other filesytem specific lock. If the
filesystem is frozen, a task can block on the frozen filesystem with
e.g., mmap_sem held. If some other task then blocks on grabbing that
mmap_sem, hibernation ill fail because it is unable to hibernate a task
holding mmap_sem. This could be fixed by making a range of filesystem
related locks use freezable sleeping. That's impractical and not
warranted just for suspend/hibernate. Assume that this is an infrequent
problem and we've given userspace a way to skip filesystem freezing
through a sysfs file.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
include/linux/fs.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b379a46b5576..1edcba3cd68e 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1781,8 +1781,7 @@ static inline void __sb_end_write(struct super_block *sb, int level)
static inline void __sb_start_write(struct super_block *sb, int level)
{
- percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1,
- level == SB_FREEZE_WRITE);
+ percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1, true);
}
static inline bool __sb_start_write_trylock(struct super_block *sb, int level)
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 2/4] fs: allow all writers to be frozen
2025-04-02 14:07 ` [PATCH v2 2/4] fs: allow all writers to be frozen Christian Brauner
@ 2025-04-02 15:32 ` Christian Brauner
2025-04-02 16:03 ` James Bottomley
2025-04-03 14:59 ` Jan Kara
1 sibling, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 15:32 UTC (permalink / raw)
To: jack, peterz
Cc: Ard Biesheuvel, linux-efi, linux-kernel, James Bottomley, mcgrof,
hch, david, rafael, djwong, pavel, mingo, will, boqun.feng,
linux-fsdevel
On Wed, Apr 02, 2025 at 04:07:32PM +0200, Christian Brauner wrote:
> During freeze/thaw we need to be able to freeze all writers during
> suspend/hibernate. Otherwise tasks such as systemd-journald that mmap a
> file and write to it will not be frozen after we've already frozen the
> filesystem.
>
> This has some risk of not being able to freeze processes in case a
> process has acquired SB_FREEZE_PAGEFAULT under mmap_sem or
> SB_FREEZE_INTERNAL under some other filesytem specific lock. If the
> filesystem is frozen, a task can block on the frozen filesystem with
> e.g., mmap_sem held. If some other task then blocks on grabbing that
> mmap_sem, hibernation ill fail because it is unable to hibernate a task
> holding mmap_sem. This could be fixed by making a range of filesystem
> related locks use freezable sleeping. That's impractical and not
> warranted just for suspend/hibernate. Assume that this is an infrequent
> problem and we've given userspace a way to skip filesystem freezing
> through a sysfs file.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---
> include/linux/fs.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b379a46b5576..1edcba3cd68e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1781,8 +1781,7 @@ static inline void __sb_end_write(struct super_block *sb, int level)
>
> static inline void __sb_start_write(struct super_block *sb, int level)
> {
> - percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1,
> - level == SB_FREEZE_WRITE);
> + percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1, true);
> }
Jan, one more thought about freezability here. We know that there will
can be at least one process during hibernation that ends up generating
page faults and that's systemd-journald. When systemd-sleep requests
writing a hibernation image via /sys/power/ files it will inevitably end
up freezing systemd-journald and it may be generating a page fault with
->mmap_lock held. systemd-journald is now sleeping with
SB_FREEZE_PAGEFAULT and TASK_FREEZABLE. We know this can cause
hibernation to fail. That part is fine. What isn't is that we will very
likely always trigger:
#ifdef CONFIG_LOCKDEP
/*
* It's dangerous to freeze with locks held; there be dragons there.
*/
if (!(state & __TASK_FREEZABLE_UNSAFE))
WARN_ON_ONCE(debug_locks && p->lockdep_depth);
#endif
with lockdep enabled.
So we really actually need percpu_rswem_read_freezable_unsafe(), i.e.,
TASK_FREEZABLE_UNSAFE.
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 2/4] fs: allow all writers to be frozen
2025-04-02 15:32 ` Christian Brauner
@ 2025-04-02 16:03 ` James Bottomley
2025-04-02 16:13 ` Christian Brauner
0 siblings, 1 reply; 59+ messages in thread
From: James Bottomley @ 2025-04-02 16:03 UTC (permalink / raw)
To: Christian Brauner, jack, peterz
Cc: Ard Biesheuvel, linux-efi, linux-kernel, mcgrof, hch, david,
rafael, djwong, pavel, mingo, will, boqun.feng, linux-fsdevel
On Wed, 2025-04-02 at 17:32 +0200, Christian Brauner wrote:
[...]
> Jan, one more thought about freezability here. We know that there
> will can be at least one process during hibernation that ends up
> generating page faults and that's systemd-journald. When systemd-
> sleep requests writing a hibernation image via /sys/power/ files it
> will inevitably end up freezing systemd-journald and it may be
> generating a page fault with ->mmap_lock held. systemd-journald is
> now sleeping with SB_FREEZE_PAGEFAULT and TASK_FREEZABLE. We know
> this can cause hibernation to fail. That part is fine. What isn't is
> that we will very likely always trigger:
>
> #ifdef CONFIG_LOCKDEP
> /*
> * It's dangerous to freeze with locks held; there be dragons
> there.
> */
> if (!(state & __TASK_FREEZABLE_UNSAFE))
> WARN_ON_ONCE(debug_locks && p->lockdep_depth);
> #endif
>
> with lockdep enabled.
>
> So we really actually need percpu_rswem_read_freezable_unsafe(),
> i.e., TASK_FREEZABLE_UNSAFE.
The sched people have pretty strong views about people not doing this,
expressed in the comment in sched.h and commit f5d39b020809
("freezer,sched: Rewrite core freezer logic") where most of the _unsafe
variants got removed with prejudice.
If we do get into this situation the worst that can happen is that
another upper lock acquisition triggers a hibernate failure and we thaw
everything, thus we can never truly deadlock, which is the fear, so
perhaps they might be OK with this. Note that Rafael's solution to
this was to disable lockdep around hibernate/suspend and resume, which
is another possibility.
Regards,
James
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 2/4] fs: allow all writers to be frozen
2025-04-02 16:03 ` James Bottomley
@ 2025-04-02 16:13 ` Christian Brauner
0 siblings, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 16:13 UTC (permalink / raw)
To: James Bottomley
Cc: jack, peterz, Ard Biesheuvel, linux-efi, linux-kernel, mcgrof,
hch, david, rafael, djwong, pavel, mingo, will, boqun.feng,
linux-fsdevel
On Wed, Apr 02, 2025 at 12:03:24PM -0400, James Bottomley wrote:
> On Wed, 2025-04-02 at 17:32 +0200, Christian Brauner wrote:
> [...]
> > Jan, one more thought about freezability here. We know that there
> > will can be at least one process during hibernation that ends up
> > generating page faults and that's systemd-journald. When systemd-
> > sleep requests writing a hibernation image via /sys/power/ files it
> > will inevitably end up freezing systemd-journald and it may be
> > generating a page fault with ->mmap_lock held. systemd-journald is
> > now sleeping with SB_FREEZE_PAGEFAULT and TASK_FREEZABLE. We know
> > this can cause hibernation to fail. That part is fine. What isn't is
> > that we will very likely always trigger:
> >
> > #ifdef CONFIG_LOCKDEP
> > /*
> > * It's dangerous to freeze with locks held; there be dragons
> > there.
> > */
> > if (!(state & __TASK_FREEZABLE_UNSAFE))
> > WARN_ON_ONCE(debug_locks && p->lockdep_depth);
> > #endif
> >
> > with lockdep enabled.
> >
> > So we really actually need percpu_rswem_read_freezable_unsafe(),
> > i.e., TASK_FREEZABLE_UNSAFE.
>
> The sched people have pretty strong views about people not doing this,
> expressed in the comment in sched.h and commit f5d39b020809
> ("freezer,sched: Rewrite core freezer logic") where most of the _unsafe
> variants got removed with prejudice.
>
> If we do get into this situation the worst that can happen is that
> another upper lock acquisition triggers a hibernate failure and we thaw
> everything, thus we can never truly deadlock, which is the fear, so
Yes, I know that it's harmless but we need to not generate misleading
lockdep splats when lockdep is turned on and confuse users.
> perhaps they might be OK with this. Note that Rafael's solution to
> this was to disable lockdep around hibernate/suspend and resume, which
> is another possibility.
It can be done as a follow-up. I'm just saying it needs treatment.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 2/4] fs: allow all writers to be frozen
2025-04-02 14:07 ` [PATCH v2 2/4] fs: allow all writers to be frozen Christian Brauner
2025-04-02 15:32 ` Christian Brauner
@ 2025-04-03 14:59 ` Jan Kara
1 sibling, 0 replies; 59+ messages in thread
From: Jan Kara @ 2025-04-03 14:59 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Wed 02-04-25 16:07:32, Christian Brauner wrote:
> During freeze/thaw we need to be able to freeze all writers during
> suspend/hibernate. Otherwise tasks such as systemd-journald that mmap a
> file and write to it will not be frozen after we've already frozen the
> filesystem.
>
> This has some risk of not being able to freeze processes in case a
> process has acquired SB_FREEZE_PAGEFAULT under mmap_sem or
> SB_FREEZE_INTERNAL under some other filesytem specific lock. If the
> filesystem is frozen, a task can block on the frozen filesystem with
> e.g., mmap_sem held. If some other task then blocks on grabbing that
> mmap_sem, hibernation ill fail because it is unable to hibernate a task
> holding mmap_sem. This could be fixed by making a range of filesystem
> related locks use freezable sleeping. That's impractical and not
> warranted just for suspend/hibernate. Assume that this is an infrequent
> problem and we've given userspace a way to skip filesystem freezing
> through a sysfs file.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
BTW, I agree about the need to silence lockdep somehow.
Honza
> ---
> include/linux/fs.h | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b379a46b5576..1edcba3cd68e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1781,8 +1781,7 @@ static inline void __sb_end_write(struct super_block *sb, int level)
>
> static inline void __sb_start_write(struct super_block *sb, int level)
> {
> - percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1,
> - level == SB_FREEZE_WRITE);
> + percpu_down_read_freezable(sb->s_writers.rw_sem + level - 1, true);
> }
>
> static inline bool __sb_start_write_trylock(struct super_block *sb, int level)
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 3/4] power: freeze filesystems during suspend/resume
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
2025-04-02 14:07 ` [PATCH v2 1/4] fs: add owner of freeze/thaw Christian Brauner
2025-04-02 14:07 ` [PATCH v2 2/4] fs: allow all writers to be frozen Christian Brauner
@ 2025-04-02 14:07 ` Christian Brauner
2025-04-03 16:29 ` Jan Kara
2025-04-02 14:07 ` [PATCH v2 4/4] kernfs: add warning about implementing freeze/thaw Christian Brauner
2025-07-20 19:23 ` [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume Askar Safin
4 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 14:07 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
Now all the pieces are in place to actually allow the power subsystem
to freeze/thaw filesystems during suspend/resume. Filesystems are only
frozen and thawed if the power subsystem does actually own the freeze.
We could bubble up errors and fail suspend/resume if the error isn't
EBUSY (aka it's already frozen) but I don't think that this is worth it.
Filesystem freezing during suspend/resume is best-effort. If the user
has 500 ext4 filesystems mounted and 4 fail to freeze for whatever
reason then we simply skip them.
What we have now is already a big improvement and let's see how we fare
with it before making our lives even harder (and uglier) than we have
to.
We add a new sysctl know /sys/power/freeze_filesystems that will allow
userspace to freeze filesystems during suspend/hibernate. For now it
defaults to off. The thaw logic doesn't require checking whether
freezing is enabled because the power subsystem exclusively owns frozen
filesystems for the duration of suspend/hibernate and is able to skip
filesystems it doesn't need to freeze.
Also it is technically possible that filesystem
filesystem_freeze_enabled is true and power freezes the filesystems but
before freezing all processes another process disables
filesystem_freeze_enabled. If power were to place the filesystems_thaw()
call under filesystems_freeze_enabled it would fail to thaw the
fileystems it frozw. The exclusive holder mechanism makes it possible to
iterate through the list without any concern making sure that no
filesystems are left frozen.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/super.c | 14 ++++++++++----
kernel/power/hibernate.c | 16 +++++++++++++++-
kernel/power/main.c | 31 +++++++++++++++++++++++++++++++
kernel/power/power.h | 4 ++++
kernel/power/suspend.c | 7 +++++++
5 files changed, 67 insertions(+), 5 deletions(-)
diff --git a/fs/super.c b/fs/super.c
index 3ddded4360c6..b4bdbc509dba 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1187,6 +1187,8 @@ static inline bool get_active_super(struct super_block *sb)
return active;
}
+static const char *filesystems_freeze_ptr = "filesystems_freeze";
+
static void filesystems_freeze_callback(struct super_block *sb, void *unused)
{
if (!sb->s_op->freeze_fs && !sb->s_op->freeze_super)
@@ -1196,9 +1198,11 @@ static void filesystems_freeze_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->freeze_super)
- sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
+ sb->s_op->freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
else
- freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
+ freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
deactivate_super(sb);
}
@@ -1218,9 +1222,11 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
return;
if (sb->s_op->thaw_super)
- sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
+ sb->s_op->thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
else
- thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
+ thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
+ filesystems_freeze_ptr);
deactivate_super(sb);
}
diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
index 50ec26ea696b..37d733945c59 100644
--- a/kernel/power/hibernate.c
+++ b/kernel/power/hibernate.c
@@ -777,6 +777,8 @@ int hibernate(void)
goto Restore;
ksys_sync_helper();
+ if (filesystem_freeze_enabled)
+ filesystems_freeze();
error = freeze_processes();
if (error)
@@ -845,6 +847,7 @@ int hibernate(void)
/* Don't bother checking whether freezer_test_done is true */
freezer_test_done = false;
Exit:
+ filesystems_thaw();
pm_notifier_call_chain(PM_POST_HIBERNATION);
Restore:
pm_restore_console();
@@ -881,6 +884,9 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
if (error)
goto restore;
+ if (filesystem_freeze_enabled)
+ filesystems_freeze();
+
error = freeze_processes();
if (error)
goto exit;
@@ -940,6 +946,7 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
thaw_processes();
exit:
+ filesystems_thaw();
pm_notifier_call_chain(PM_POST_HIBERNATION);
restore:
@@ -1028,19 +1035,26 @@ static int software_resume(void)
if (error)
goto Restore;
+ if (filesystem_freeze_enabled)
+ filesystems_freeze();
+
pm_pr_dbg("Preparing processes for hibernation restore.\n");
error = freeze_processes();
- if (error)
+ if (error) {
+ filesystems_thaw();
goto Close_Finish;
+ }
error = freeze_kernel_threads();
if (error) {
thaw_processes();
+ filesystems_thaw();
goto Close_Finish;
}
error = load_image_and_restore();
thaw_processes();
+ filesystems_thaw();
Finish:
pm_notifier_call_chain(PM_POST_RESTORE);
Restore:
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 6254814d4817..0b0e76324c43 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -962,6 +962,34 @@ power_attr(pm_freeze_timeout);
#endif /* CONFIG_FREEZER*/
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
+bool filesystem_freeze_enabled = false;
+
+static ssize_t freeze_filesystems_show(struct kobject *kobj,
+ struct kobj_attribute *attr, char *buf)
+{
+ return sysfs_emit(buf, "%d\n", filesystem_freeze_enabled);
+}
+
+static ssize_t freeze_filesystems_store(struct kobject *kobj,
+ struct kobj_attribute *attr,
+ const char *buf, size_t n)
+{
+ unsigned long val;
+
+ if (kstrtoul(buf, 10, &val))
+ return -EINVAL;
+
+ if (val > 1)
+ return -EINVAL;
+
+ filesystem_freeze_enabled = !!val;
+ return n;
+}
+
+power_attr(freeze_filesystems);
+#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATION */
+
static struct attribute * g[] = {
&state_attr.attr,
#ifdef CONFIG_PM_TRACE
@@ -991,6 +1019,9 @@ static struct attribute * g[] = {
#endif
#ifdef CONFIG_FREEZER
&pm_freeze_timeout_attr.attr,
+#endif
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
+ &freeze_filesystems_attr.attr,
#endif
NULL,
};
diff --git a/kernel/power/power.h b/kernel/power/power.h
index c352dea2f67b..2eb81662b8fa 100644
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -18,6 +18,10 @@ struct swsusp_info {
unsigned long size;
} __aligned(PAGE_SIZE);
+#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
+extern bool filesystem_freeze_enabled;
+#endif
+
#ifdef CONFIG_HIBERNATION
/* kernel/power/snapshot.c */
extern void __init hibernate_reserved_size_init(void);
diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
index 8eaec4ab121d..76b141b9aac0 100644
--- a/kernel/power/suspend.c
+++ b/kernel/power/suspend.c
@@ -30,6 +30,7 @@
#include <trace/events/power.h>
#include <linux/compiler.h>
#include <linux/moduleparam.h>
+#include <linux/fs.h>
#include "power.h"
@@ -374,6 +375,8 @@ static int suspend_prepare(suspend_state_t state)
if (error)
goto Restore;
+ if (filesystem_freeze_enabled)
+ filesystems_freeze();
trace_suspend_resume(TPS("freeze_processes"), 0, true);
error = suspend_freeze_processes();
trace_suspend_resume(TPS("freeze_processes"), 0, false);
@@ -550,6 +553,7 @@ int suspend_devices_and_enter(suspend_state_t state)
static void suspend_finish(void)
{
suspend_thaw_processes();
+ filesystems_thaw();
pm_notifier_call_chain(PM_POST_SUSPEND);
pm_restore_console();
}
@@ -588,6 +592,8 @@ static int enter_state(suspend_state_t state)
ksys_sync_helper();
trace_suspend_resume(TPS("sync_filesystems"), 0, false);
}
+ if (filesystem_freeze_enabled)
+ filesystems_freeze();
pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
pm_suspend_clear_flags();
@@ -609,6 +615,7 @@ static int enter_state(suspend_state_t state)
pm_pr_dbg("Finishing wakeup.\n");
suspend_finish();
Unlock:
+ filesystems_thaw();
mutex_unlock(&system_transition_mutex);
return error;
}
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 3/4] power: freeze filesystems during suspend/resume
2025-04-02 14:07 ` [PATCH v2 3/4] power: freeze filesystems during suspend/resume Christian Brauner
@ 2025-04-03 16:29 ` Jan Kara
0 siblings, 0 replies; 59+ messages in thread
From: Jan Kara @ 2025-04-03 16:29 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Wed 02-04-25 16:07:33, Christian Brauner wrote:
> Now all the pieces are in place to actually allow the power subsystem
> to freeze/thaw filesystems during suspend/resume. Filesystems are only
> frozen and thawed if the power subsystem does actually own the freeze.
>
> We could bubble up errors and fail suspend/resume if the error isn't
> EBUSY (aka it's already frozen) but I don't think that this is worth it.
> Filesystem freezing during suspend/resume is best-effort. If the user
> has 500 ext4 filesystems mounted and 4 fail to freeze for whatever
> reason then we simply skip them.
>
> What we have now is already a big improvement and let's see how we fare
> with it before making our lives even harder (and uglier) than we have
> to.
>
> We add a new sysctl know /sys/power/freeze_filesystems that will allow
> userspace to freeze filesystems during suspend/hibernate. For now it
> defaults to off. The thaw logic doesn't require checking whether
> freezing is enabled because the power subsystem exclusively owns frozen
> filesystems for the duration of suspend/hibernate and is able to skip
> filesystems it doesn't need to freeze.
>
> Also it is technically possible that filesystem
> filesystem_freeze_enabled is true and power freezes the filesystems but
> before freezing all processes another process disables
> filesystem_freeze_enabled. If power were to place the filesystems_thaw()
> call under filesystems_freeze_enabled it would fail to thaw the
> fileystems it frozw. The exclusive holder mechanism makes it possible to
> iterate through the list without any concern making sure that no
> filesystems are left frozen.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
Looks good modulo the nesting issue I've mentioned in my comments to patch
1. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/super.c | 14 ++++++++++----
> kernel/power/hibernate.c | 16 +++++++++++++++-
> kernel/power/main.c | 31 +++++++++++++++++++++++++++++++
> kernel/power/power.h | 4 ++++
> kernel/power/suspend.c | 7 +++++++
> 5 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/fs/super.c b/fs/super.c
> index 3ddded4360c6..b4bdbc509dba 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1187,6 +1187,8 @@ static inline bool get_active_super(struct super_block *sb)
> return active;
> }
>
> +static const char *filesystems_freeze_ptr = "filesystems_freeze";
> +
> static void filesystems_freeze_callback(struct super_block *sb, void *unused)
> {
> if (!sb->s_op->freeze_fs && !sb->s_op->freeze_super)
> @@ -1196,9 +1198,11 @@ static void filesystems_freeze_callback(struct super_block *sb, void *unused)
> return;
>
> if (sb->s_op->freeze_super)
> - sb->s_op->freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> + sb->s_op->freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> + filesystems_freeze_ptr);
> else
> - freeze_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> + freeze_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> + filesystems_freeze_ptr);
>
> deactivate_super(sb);
> }
> @@ -1218,9 +1222,11 @@ static void filesystems_thaw_callback(struct super_block *sb, void *unused)
> return;
>
> if (sb->s_op->thaw_super)
> - sb->s_op->thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> + sb->s_op->thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> + filesystems_freeze_ptr);
> else
> - thaw_super(sb, FREEZE_MAY_NEST | FREEZE_HOLDER_KERNEL, NULL);
> + thaw_super(sb, FREEZE_EXCL | FREEZE_HOLDER_KERNEL,
> + filesystems_freeze_ptr);
>
> deactivate_super(sb);
> }
> diff --git a/kernel/power/hibernate.c b/kernel/power/hibernate.c
> index 50ec26ea696b..37d733945c59 100644
> --- a/kernel/power/hibernate.c
> +++ b/kernel/power/hibernate.c
> @@ -777,6 +777,8 @@ int hibernate(void)
> goto Restore;
>
> ksys_sync_helper();
> + if (filesystem_freeze_enabled)
> + filesystems_freeze();
>
> error = freeze_processes();
> if (error)
> @@ -845,6 +847,7 @@ int hibernate(void)
> /* Don't bother checking whether freezer_test_done is true */
> freezer_test_done = false;
> Exit:
> + filesystems_thaw();
> pm_notifier_call_chain(PM_POST_HIBERNATION);
> Restore:
> pm_restore_console();
> @@ -881,6 +884,9 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
> if (error)
> goto restore;
>
> + if (filesystem_freeze_enabled)
> + filesystems_freeze();
> +
> error = freeze_processes();
> if (error)
> goto exit;
> @@ -940,6 +946,7 @@ int hibernate_quiet_exec(int (*func)(void *data), void *data)
> thaw_processes();
>
> exit:
> + filesystems_thaw();
> pm_notifier_call_chain(PM_POST_HIBERNATION);
>
> restore:
> @@ -1028,19 +1035,26 @@ static int software_resume(void)
> if (error)
> goto Restore;
>
> + if (filesystem_freeze_enabled)
> + filesystems_freeze();
> +
> pm_pr_dbg("Preparing processes for hibernation restore.\n");
> error = freeze_processes();
> - if (error)
> + if (error) {
> + filesystems_thaw();
> goto Close_Finish;
> + }
>
> error = freeze_kernel_threads();
> if (error) {
> thaw_processes();
> + filesystems_thaw();
> goto Close_Finish;
> }
>
> error = load_image_and_restore();
> thaw_processes();
> + filesystems_thaw();
> Finish:
> pm_notifier_call_chain(PM_POST_RESTORE);
> Restore:
> diff --git a/kernel/power/main.c b/kernel/power/main.c
> index 6254814d4817..0b0e76324c43 100644
> --- a/kernel/power/main.c
> +++ b/kernel/power/main.c
> @@ -962,6 +962,34 @@ power_attr(pm_freeze_timeout);
>
> #endif /* CONFIG_FREEZER*/
>
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
> +bool filesystem_freeze_enabled = false;
> +
> +static ssize_t freeze_filesystems_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return sysfs_emit(buf, "%d\n", filesystem_freeze_enabled);
> +}
> +
> +static ssize_t freeze_filesystems_store(struct kobject *kobj,
> + struct kobj_attribute *attr,
> + const char *buf, size_t n)
> +{
> + unsigned long val;
> +
> + if (kstrtoul(buf, 10, &val))
> + return -EINVAL;
> +
> + if (val > 1)
> + return -EINVAL;
> +
> + filesystem_freeze_enabled = !!val;
> + return n;
> +}
> +
> +power_attr(freeze_filesystems);
> +#endif /* CONFIG_SUSPEND || CONFIG_HIBERNATION */
> +
> static struct attribute * g[] = {
> &state_attr.attr,
> #ifdef CONFIG_PM_TRACE
> @@ -991,6 +1019,9 @@ static struct attribute * g[] = {
> #endif
> #ifdef CONFIG_FREEZER
> &pm_freeze_timeout_attr.attr,
> +#endif
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
> + &freeze_filesystems_attr.attr,
> #endif
> NULL,
> };
> diff --git a/kernel/power/power.h b/kernel/power/power.h
> index c352dea2f67b..2eb81662b8fa 100644
> --- a/kernel/power/power.h
> +++ b/kernel/power/power.h
> @@ -18,6 +18,10 @@ struct swsusp_info {
> unsigned long size;
> } __aligned(PAGE_SIZE);
>
> +#if defined(CONFIG_SUSPEND) || defined(CONFIG_HIBERNATION)
> +extern bool filesystem_freeze_enabled;
> +#endif
> +
> #ifdef CONFIG_HIBERNATION
> /* kernel/power/snapshot.c */
> extern void __init hibernate_reserved_size_init(void);
> diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c
> index 8eaec4ab121d..76b141b9aac0 100644
> --- a/kernel/power/suspend.c
> +++ b/kernel/power/suspend.c
> @@ -30,6 +30,7 @@
> #include <trace/events/power.h>
> #include <linux/compiler.h>
> #include <linux/moduleparam.h>
> +#include <linux/fs.h>
>
> #include "power.h"
>
> @@ -374,6 +375,8 @@ static int suspend_prepare(suspend_state_t state)
> if (error)
> goto Restore;
>
> + if (filesystem_freeze_enabled)
> + filesystems_freeze();
> trace_suspend_resume(TPS("freeze_processes"), 0, true);
> error = suspend_freeze_processes();
> trace_suspend_resume(TPS("freeze_processes"), 0, false);
> @@ -550,6 +553,7 @@ int suspend_devices_and_enter(suspend_state_t state)
> static void suspend_finish(void)
> {
> suspend_thaw_processes();
> + filesystems_thaw();
> pm_notifier_call_chain(PM_POST_SUSPEND);
> pm_restore_console();
> }
> @@ -588,6 +592,8 @@ static int enter_state(suspend_state_t state)
> ksys_sync_helper();
> trace_suspend_resume(TPS("sync_filesystems"), 0, false);
> }
> + if (filesystem_freeze_enabled)
> + filesystems_freeze();
>
> pm_pr_dbg("Preparing system for sleep (%s)\n", mem_sleep_labels[state]);
> pm_suspend_clear_flags();
> @@ -609,6 +615,7 @@ static int enter_state(suspend_state_t state)
> pm_pr_dbg("Finishing wakeup.\n");
> suspend_finish();
> Unlock:
> + filesystems_thaw();
> mutex_unlock(&system_transition_mutex);
> return error;
> }
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* [PATCH v2 4/4] kernfs: add warning about implementing freeze/thaw
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
` (2 preceding siblings ...)
2025-04-02 14:07 ` [PATCH v2 3/4] power: freeze filesystems during suspend/resume Christian Brauner
@ 2025-04-02 14:07 ` Christian Brauner
2025-04-03 15:00 ` Jan Kara
2025-07-20 19:23 ` [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume Askar Safin
4 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 14:07 UTC (permalink / raw)
To: linux-fsdevel, jack
Cc: Christian Brauner, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
Sysfs is built on top of kernfs and sysfs provides the power management
infrastructure to support suspend/hibernate by writing to various files
in /sys/power/. As filesystems may be automatically frozen during
suspend/hibernate implementing freeze/thaw support for kernfs
generically will cause deadlocks as the suspending/hibernation
initiating task will hold a VFS lock that it will then wait upon to be
released. If freeze/thaw for kernfs is needed talk to the VFS.
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
fs/kernfs/mount.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
index 1358c21837f1..d2073bb2b633 100644
--- a/fs/kernfs/mount.c
+++ b/fs/kernfs/mount.c
@@ -62,6 +62,21 @@ const struct super_operations kernfs_sops = {
.show_options = kernfs_sop_show_options,
.show_path = kernfs_sop_show_path,
+
+ /*
+ * sysfs is built on top of kernfs and sysfs provides the power
+ * management infrastructure to support suspend/hibernate by
+ * writing to various files in /sys/power/. As filesystems may
+ * be automatically frozen during suspend/hibernate implementing
+ * freeze/thaw support for kernfs generically will cause
+ * deadlocks as the suspending/hibernation initiating task will
+ * hold a VFS lock that it will then wait upon to be released.
+ * If freeze/thaw for kernfs is needed talk to the VFS.
+ */
+ .freeze_fs = NULL,
+ .unfreeze_fs = NULL,
+ .freeze_super = NULL,
+ .thaw_super = NULL,
};
static int kernfs_encode_fh(struct inode *inode, __u32 *fh, int *max_len,
--
2.47.2
^ permalink raw reply related [flat|nested] 59+ messages in thread* Re: [PATCH v2 4/4] kernfs: add warning about implementing freeze/thaw
2025-04-02 14:07 ` [PATCH v2 4/4] kernfs: add warning about implementing freeze/thaw Christian Brauner
@ 2025-04-03 15:00 ` Jan Kara
0 siblings, 0 replies; 59+ messages in thread
From: Jan Kara @ 2025-04-03 15:00 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, Ard Biesheuvel, linux-efi, linux-kernel,
James Bottomley, mcgrof, hch, david, rafael, djwong, pavel,
peterz, mingo, will, boqun.feng
On Wed 02-04-25 16:07:34, Christian Brauner wrote:
> Sysfs is built on top of kernfs and sysfs provides the power management
> infrastructure to support suspend/hibernate by writing to various files
> in /sys/power/. As filesystems may be automatically frozen during
> suspend/hibernate implementing freeze/thaw support for kernfs
> generically will cause deadlocks as the suspending/hibernation
> initiating task will hold a VFS lock that it will then wait upon to be
> released. If freeze/thaw for kernfs is needed talk to the VFS.
>
> Signed-off-by: Christian Brauner <brauner@kernel.org>
Yeah, good idea. Feel free to add:
Reviewed-by: Jan Kara <jack@suse.cz>
Honza
> ---
> fs/kernfs/mount.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index 1358c21837f1..d2073bb2b633 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -62,6 +62,21 @@ const struct super_operations kernfs_sops = {
>
> .show_options = kernfs_sop_show_options,
> .show_path = kernfs_sop_show_path,
> +
> + /*
> + * sysfs is built on top of kernfs and sysfs provides the power
> + * management infrastructure to support suspend/hibernate by
> + * writing to various files in /sys/power/. As filesystems may
> + * be automatically frozen during suspend/hibernate implementing
> + * freeze/thaw support for kernfs generically will cause
> + * deadlocks as the suspending/hibernation initiating task will
> + * hold a VFS lock that it will then wait upon to be released.
> + * If freeze/thaw for kernfs is needed talk to the VFS.
> + */
> + .freeze_fs = NULL,
> + .unfreeze_fs = NULL,
> + .freeze_super = NULL,
> + .thaw_super = NULL,
> };
>
> static int kernfs_encode_fh(struct inode *inode, __u32 *fh, int *max_len,
>
> --
> 2.47.2
>
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-02 14:07 ` [PATCH v2 0/4] " Christian Brauner
` (3 preceding siblings ...)
2025-04-02 14:07 ` [PATCH v2 4/4] kernfs: add warning about implementing freeze/thaw Christian Brauner
@ 2025-07-20 19:23 ` Askar Safin
2025-07-21 12:09 ` Jan Kara
4 siblings, 1 reply; 59+ messages in thread
From: Askar Safin @ 2025-07-20 19:23 UTC (permalink / raw)
To: brauner
Cc: James.Bottomley, ardb, boqun.feng, david, djwong, hch, jack,
linux-efi, linux-fsdevel, linux-kernel, mcgrof, mingo, pavel,
peterz, rafael, will
Hi, Christian Brauner, Jan Kara and other contributors of this patchset.
I did experiments on my laptop, and these experiments show that this patchset does not solve various longstanding problems related to suspend and filesystems. (Even if I enable /sys/power/freeze_filesystems )
Now let me describe problems I had in the past (and still have!) and then experiments I did and their results.
So, I had these 3 problems:
- Suspend doesn't work if fstrim in progress (note that I use btrfs as root file system)
- Suspend doesn't work if scrub in progress
- Suspend doesn't work if we try to read from fuse-sshfs filesystem while network is down
Let me describe third problem in more detail. To reproduce you need to do this:
- Mount remote filesystem using sshfs (it is based on ssh and fuse)
- Disable internet
- Run command "ls" in that sshfs filesystem (this command will, of course, hang, because network is down)
- Then suspend
Suspend will not work.
Does your patchset supposed to fix these problems?
Okay, so just now I was able to reproduce all 3 problems on latest mainline ( f4a40a4282f467ec99745c6ba62cb84346e42139 ), which (as well as I understand) has this patchset applied.
I reproduced them with /sys/power/freeze_filesystems set to both 0 and 1 (thus I did 3 * 2 = 6 experiments).
I'm available for further testing.
--
Askar Safin
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-07-20 19:23 ` [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume Askar Safin
@ 2025-07-21 12:09 ` Jan Kara
2025-08-04 5:31 ` Miklos Szeredi
0 siblings, 1 reply; 59+ messages in thread
From: Jan Kara @ 2025-07-21 12:09 UTC (permalink / raw)
To: Askar Safin
Cc: brauner, James.Bottomley, ardb, boqun.feng, david, djwong, hch,
jack, linux-efi, linux-fsdevel, linux-kernel, mcgrof, mingo,
pavel, peterz, rafael, will
Hi!
On Sun 20-07-25 22:23:36, Askar Safin wrote:
> I did experiments on my laptop, and these experiments show that this
> patchset does not solve various longstanding problems related to suspend
> and filesystems. (Even if I enable /sys/power/freeze_filesystems )
>
> Now let me describe problems I had in the past (and still have!) and then
> experiments I did and their results.
>
> So, I had these 3 problems:
>
> - Suspend doesn't work if fstrim in progress (note that I use btrfs as
> root file system)
Right, this is expected because the FITRIM ioctl (syscall as any other)
likely takes too long and so the suspend code looses its patience. There's
nothing VFS can do about this. You can talk to btrfs developers to
periodically check for pending signal / freezing event like e.g. ext4 does
in ext4_trim_interrupted() to avoid suspend failures when FITRIM is
running.
> - Suspend doesn't work if scrub in progress
Similar situation as with FITRIM. This is fully in control of the
filesystem and unless the filesystem adds checks and early abort paths, VFS
cannot do anything.
> - Suspend doesn't work if we try to read from fuse-sshfs filesystem while
> network is down
On the surface the problem is the same as the above two but the details
here are subtly different. Here I expect (although I'm not 100% sure) the
blocked process is inside the FUSE filesystem waiting for the FUSE daemon
to reply (a /proc/<pid>/stack of the blocked process would be useful here).
In theory, FUSE filesystem should be able to make the wait for reply in
TASK_FREEZABLE state which would fix your issue. In any case this is very
likely work for FUSE developers.
So I'm sorry but the patch set you speak about isn't supposed to fix any of
the above issues you hit.
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-07-21 12:09 ` Jan Kara
@ 2025-08-04 5:31 ` Miklos Szeredi
2025-08-04 6:02 ` Askar Safin
0 siblings, 1 reply; 59+ messages in thread
From: Miklos Szeredi @ 2025-08-04 5:31 UTC (permalink / raw)
To: Jan Kara
Cc: Askar Safin, brauner, James.Bottomley, ardb, boqun.feng, david,
djwong, hch, linux-efi, linux-fsdevel, linux-kernel, mcgrof,
mingo, pavel, peterz, rafael, will
On Mon, 21 Jul 2025 at 14:09, Jan Kara <jack@suse.cz> wrote:
>
> Hi!
>
> On Sun 20-07-25 22:23:36, Askar Safin wrote:
> > - Suspend doesn't work if we try to read from fuse-sshfs filesystem while
> > network is down
>
> On the surface the problem is the same as the above two but the details
> here are subtly different. Here I expect (although I'm not 100% sure) the
> blocked process is inside the FUSE filesystem waiting for the FUSE daemon
> to reply (a /proc/<pid>/stack of the blocked process would be useful here).
> In theory, FUSE filesystem should be able to make the wait for reply in
> TASK_FREEZABLE state which would fix your issue. In any case this is very
> likely work for FUSE developers.
This is a known problem with an unknown solution.
We can fix some of the cases, but changing all filesystem locks to be
freezable is likely not workable.
Thanks,
Miklos
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-08-04 5:31 ` Miklos Szeredi
@ 2025-08-04 6:02 ` Askar Safin
2025-08-04 6:51 ` Sergey Senozhatsky
0 siblings, 1 reply; 59+ messages in thread
From: Askar Safin @ 2025-08-04 6:02 UTC (permalink / raw)
To: Miklos Szeredi
Cc: Jan Kara, brauner, James.Bottomley, ardb, boqun.feng, david,
djwong, hch, linux-efi, linux-fsdevel, linux-kernel, mcgrof,
mingo, pavel, peterz, rafael, will, Joanne Koong, linux-pm,
senozhatsky
---- On Mon, 04 Aug 2025 09:31:00 +0400 Miklos Szeredi <miklos@szeredi.hu> wrote ---
> This is a known problem with an unknown solution.
>
> We can fix some of the cases, but changing all filesystem locks to be
> freezable is likely not workable.
So what to do in case of networked FUSE, such as sshfs?
We should put workaround to other place?
Where? To libfuse or to each networked FUSE daemon, such as sshfs?
I hit this bug in the past, and I'm very angry.
A lot of other people hit this FUSE bug, too: https://github.com/systemd/systemd/issues/37590 .
What about this timeout solution: https://lore.kernel.org/linux-fsdevel/20250122215528.1270478-1-joannelkoong@gmail.com/ ?
Will it work? As well as I understand, currently kernel waits 20 seconds, when it tries to freeze processes when suspending.
So, what if we use this Joanne Koong's timeout patch, and set timeout to 19 seconds?
So, in short, if we cannot properly fix kernel, then where fix belongs? What should we do?
--
Askar Safin
https://types.pl/@safinaskar
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH v2 0/4] power: wire-up filesystem freeze/thaw with suspend/resume
2025-08-04 6:02 ` Askar Safin
@ 2025-08-04 6:51 ` Sergey Senozhatsky
0 siblings, 0 replies; 59+ messages in thread
From: Sergey Senozhatsky @ 2025-08-04 6:51 UTC (permalink / raw)
To: Askar Safin
Cc: Miklos Szeredi, Jan Kara, brauner, James.Bottomley, ardb,
boqun.feng, david, djwong, hch, linux-efi, linux-fsdevel,
linux-kernel, mcgrof, mingo, pavel, peterz, rafael, will,
Joanne Koong, linux-pm, senozhatsky
On (25/08/04 10:02), Askar Safin wrote:
> What about this timeout solution: https://lore.kernel.org/linux-fsdevel/20250122215528.1270478-1-joannelkoong@gmail.com/ ?
> Will it work? As well as I understand, currently kernel waits 20 seconds, when it tries to freeze processes when suspending.
> So, what if we use this Joanne Koong's timeout patch, and set timeout to 19 seconds?
I think the problem with this approach is that not all fuse connections
are remote (over the network). One can use fuse to mount vfat/ntfs or
even archives. Destroying those connections for suspend is unlikely to
be loved by users.
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (7 preceding siblings ...)
2025-04-01 9:32 ` Jan Kara
@ 2025-04-01 14:14 ` Peter Zijlstra
2025-04-01 14:40 ` Christian Brauner
2025-04-01 17:02 ` James Bottomley
9 siblings, 1 reply; 59+ messages in thread
From: Peter Zijlstra @ 2025-04-01 14:14 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, djwong, pavel,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 02:32:45AM +0200, Christian Brauner wrote:
> The whole shebang can also be found at:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
>
> I know nothing about power or hibernation. I've tested it as best as I
> could. Works for me (TM).
>
> I need to catch some actual sleep now...
>
> ---
>
> Now all the pieces are in place to actually allow the power subsystem to
> freeze/thaw filesystems during suspend/resume. Filesystems are only
> frozen and thawed if the power subsystem does actually own the freeze.
Urgh, I was relying on all kthreads to be freezable for live-patching:
https://lkml.kernel.org/r/20250324134909.GA14718@noisy.programming.kicks-ass.net
So I understand the problem with freezing filesystems, but can't we
leave the TASK_FREEZABLE in the kthreads? The way I understand it, the
power subsystem will first freeze the filesystems before it goes freeze
threads anyway. So them remaining freezable should not affect anything,
right?
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 14:14 ` [PATCH 0/6] " Peter Zijlstra
@ 2025-04-01 14:40 ` Christian Brauner
2025-04-01 14:59 ` Peter Zijlstra
0 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-01 14:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, djwong, pavel,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 04:14:07PM +0200, Peter Zijlstra wrote:
> On Tue, Apr 01, 2025 at 02:32:45AM +0200, Christian Brauner wrote:
> > The whole shebang can also be found at:
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> >
> > I know nothing about power or hibernation. I've tested it as best as I
> > could. Works for me (TM).
> >
> > I need to catch some actual sleep now...
> >
> > ---
> >
> > Now all the pieces are in place to actually allow the power subsystem to
> > freeze/thaw filesystems during suspend/resume. Filesystems are only
> > frozen and thawed if the power subsystem does actually own the freeze.
>
> Urgh, I was relying on all kthreads to be freezable for live-patching:
>
> https://lkml.kernel.org/r/20250324134909.GA14718@noisy.programming.kicks-ass.net
>
> So I understand the problem with freezing filesystems, but can't we
> leave the TASK_FREEZABLE in the kthreads? The way I understand it, the
Yeah, we can.
> power subsystem will first freeze the filesystems before it goes freeze
> threads anyway. So them remaining freezable should not affect anything,
> right?
Yes. I've dropped the other patches. I've discussed this later
downthread with Jan.
>
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 14:40 ` Christian Brauner
@ 2025-04-01 14:59 ` Peter Zijlstra
0 siblings, 0 replies; 59+ messages in thread
From: Peter Zijlstra @ 2025-04-01 14:59 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, James Bottomley, mcgrof, hch, david, djwong, pavel,
mingo, will, boqun.feng
On Tue, Apr 01, 2025 at 04:40:33PM +0200, Christian Brauner wrote:
> On Tue, Apr 01, 2025 at 04:14:07PM +0200, Peter Zijlstra wrote:
> > On Tue, Apr 01, 2025 at 02:32:45AM +0200, Christian Brauner wrote:
> > > The whole shebang can also be found at:
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> > >
> > > I know nothing about power or hibernation. I've tested it as best as I
> > > could. Works for me (TM).
> > >
> > > I need to catch some actual sleep now...
> > >
> > > ---
> > >
> > > Now all the pieces are in place to actually allow the power subsystem to
> > > freeze/thaw filesystems during suspend/resume. Filesystems are only
> > > frozen and thawed if the power subsystem does actually own the freeze.
> >
> > Urgh, I was relying on all kthreads to be freezable for live-patching:
> >
> > https://lkml.kernel.org/r/20250324134909.GA14718@noisy.programming.kicks-ass.net
> >
> > So I understand the problem with freezing filesystems, but can't we
> > leave the TASK_FREEZABLE in the kthreads? The way I understand it, the
>
> Yeah, we can.
>
> > power subsystem will first freeze the filesystems before it goes freeze
> > threads anyway. So them remaining freezable should not affect anything,
> > right?
>
> Yes. I've dropped the other patches. I've discussed this later
> downthread with Jan.
Thanks!
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 0:32 ` [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume Christian Brauner
` (8 preceding siblings ...)
2025-04-01 14:14 ` [PATCH 0/6] " Peter Zijlstra
@ 2025-04-01 17:02 ` James Bottomley
2025-04-02 7:46 ` Christian Brauner
9 siblings, 1 reply; 59+ messages in thread
From: James Bottomley @ 2025-04-01 17:02 UTC (permalink / raw)
To: Christian Brauner, linux-fsdevel, jack, rafael
Cc: Ard Biesheuvel, linux-efi, linux-kernel, mcgrof, hch, david,
djwong, pavel, peterz, mingo, will, boqun.feng
On Tue, 2025-04-01 at 02:32 +0200, Christian Brauner wrote:
> The whole shebang can also be found at:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
>
> I know nothing about power or hibernation. I've tested it as best as
> I could. Works for me (TM).
I'm testing the latest you have in work.freeze and it doesn't currently
work for me. Patch 7b315c39b67d ("power: freeze filesystems during
suspend/resume") doesn't set filesystems_freeze_ptr so it ends up being
NULL and tripping over this check
+static inline bool may_unfreeze(struct super_block *sb, enum
freeze_holder who,
+ const void *freeze_owner)
+{
+ WARN_ON_ONCE((who & ~FREEZE_FLAGS));
+ WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
+
+ if (who & FREEZE_EXCL) {
+ if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
+ return false;
in f15a9ae05a71 ("fs: add owner of freeze/thaw") and failing to resume
from hibernate. Setting it to __builtin_return_address(0) in
filesystems_freeze() makes everything work as expected, so that's what
I'm testing now.
I suppose one minor, minor nit is that the vagaries of English grammar
mean that the verbs fail and succeed don't take the same grammatical
construction, so failed can take the infinitive (failed to thaw)
perfectly well, but succeeded takes a prepositional gerund construction
instead: "succeeded at/in thawing" instead of the infinitive "succeeded
to thaw" ... I've no idea why, but I'd probably blame the Victorians
...
Regards,
James
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-01 17:02 ` James Bottomley
@ 2025-04-02 7:46 ` Christian Brauner
2025-04-08 15:43 ` James Bottomley
0 siblings, 1 reply; 59+ messages in thread
From: Christian Brauner @ 2025-04-02 7:46 UTC (permalink / raw)
To: James Bottomley
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, mcgrof, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, Apr 01, 2025 at 01:02:07PM -0400, James Bottomley wrote:
> On Tue, 2025-04-01 at 02:32 +0200, Christian Brauner wrote:
> > The whole shebang can also be found at:
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> >
> > I know nothing about power or hibernation. I've tested it as best as
> > I could. Works for me (TM).
>
> I'm testing the latest you have in work.freeze and it doesn't currently
> work for me. Patch 7b315c39b67d ("power: freeze filesystems during
> suspend/resume") doesn't set filesystems_freeze_ptr so it ends up being
> NULL and tripping over this check
I haven't pushed the new version there. Sorry about that. I only have it
locally.
>
> +static inline bool may_unfreeze(struct super_block *sb, enum
> freeze_holder who,
> + const void *freeze_owner)
> +{
> + WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> + WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
> +
> + if (who & FREEZE_EXCL) {
> + if (WARN_ON_ONCE(sb->s_writers.freeze_owner == NULL))
> + return false;
>
>
> in f15a9ae05a71 ("fs: add owner of freeze/thaw") and failing to resume
> from hibernate. Setting it to __builtin_return_address(0) in
> filesystems_freeze() makes everything work as expected, so that's what
> I'm testing now.
+1
I'll send the final version out in a bit.
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-02 7:46 ` Christian Brauner
@ 2025-04-08 15:43 ` James Bottomley
2025-04-08 17:09 ` Luis Chamberlain
2025-04-09 7:17 ` Christian Brauner
0 siblings, 2 replies; 59+ messages in thread
From: James Bottomley @ 2025-04-08 15:43 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, mcgrof, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Wed, 2025-04-02 at 09:46 +0200, Christian Brauner wrote:
> On Tue, Apr 01, 2025 at 01:02:07PM -0400, James Bottomley wrote:
> > On Tue, 2025-04-01 at 02:32 +0200, Christian Brauner wrote:
> > > The whole shebang can also be found at:
> > > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> > >
> > > I know nothing about power or hibernation. I've tested it as best
> > > as I could. Works for me (TM).
> >
> > I'm testing the latest you have in work.freeze and it doesn't
> > currently work for me. Patch 7b315c39b67d ("power: freeze
> > filesystems during suspend/resume") doesn't set
> > filesystems_freeze_ptr so it ends up being NULL and tripping over
> > this check
>
> I haven't pushed the new version there. Sorry about that. I only have
> it locally.
>
> >
> > +static inline bool may_unfreeze(struct super_block *sb, enum
> > freeze_holder who,
> > + const void *freeze_owner)
> > +{
> > + WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> > + WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
> > +
> > + if (who & FREEZE_EXCL) {
> > + if (WARN_ON_ONCE(sb->s_writers.freeze_owner ==
> > NULL))
> > + return false;
> >
> >
> > in f15a9ae05a71 ("fs: add owner of freeze/thaw") and failing to
> > resume from hibernate. Setting it to __builtin_return_address(0)
> > in filesystems_freeze() makes everything work as expected, so
> > that's what I'm testing now.
>
> +1
>
> I'll send the final version out in a bit.
I've now done some extensive testing on loop nested filesystems with
fio load on the upper. I've tried xfs on ext4 and ext4 on ext4.
Hibernate/Resume has currently worked on these without a hitch (and the
fio load burps a bit but then starts running at full speed within a few
seconds). What I'm doing is a single round of hibernate/resume followed
by a reboot. I'm relying on the fschecks to detect any filesystem
corruption. I've also tried doing a couple of fresh starts of the
hibernated image to check that we did correctly freeze the filesystems.
The problems I've noticed are:
1. I'm using 9p to push host directories throught and that
completely hangs after a resume. This is expected because the
virtio server is out of sync, but it does indicate a need to
address Jeff's question of what we should be doing for network
filesystems (and is also the reason I have to reboot after
resuming).
2. Top doesn't show any CPU activity after resume even though fio is
definitely running. This seems to be a suspend issue and
unrelated to filesystems, but I'll continue investigating.
Regards,
James
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-08 15:43 ` James Bottomley
@ 2025-04-08 17:09 ` Luis Chamberlain
2025-04-08 17:20 ` Luis Chamberlain
2025-04-08 17:24 ` James Bottomley
2025-04-09 7:17 ` Christian Brauner
1 sibling, 2 replies; 59+ messages in thread
From: Luis Chamberlain @ 2025-04-08 17:09 UTC (permalink / raw)
To: James Bottomley
Cc: Christian Brauner, linux-fsdevel, jack, rafael, Ard Biesheuvel,
linux-efi, linux-kernel, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, Apr 08, 2025 at 11:43:46AM -0400, James Bottomley wrote:
> On Wed, 2025-04-02 at 09:46 +0200, Christian Brauner wrote:
> > On Tue, Apr 01, 2025 at 01:02:07PM -0400, James Bottomley wrote:
> > > On Tue, 2025-04-01 at 02:32 +0200, Christian Brauner wrote:
> > > > The whole shebang can also be found at:
> > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> > > >
> > > > I know nothing about power or hibernation. I've tested it as best
> > > > as I could. Works for me (TM).
> > >
> > > I'm testing the latest you have in work.freeze and it doesn't
> > > currently work for me. Patch 7b315c39b67d ("power: freeze
> > > filesystems during suspend/resume") doesn't set
> > > filesystems_freeze_ptr so it ends up being NULL and tripping over
> > > this check
> >
> > I haven't pushed the new version there. Sorry about that. I only have
> > it locally.
> >
> > >
> > > +static inline bool may_unfreeze(struct super_block *sb, enum
> > > freeze_holder who,
> > > + const void *freeze_owner)
> > > +{
> > > + WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> > > + WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
> > > +
> > > + if (who & FREEZE_EXCL) {
> > > + if (WARN_ON_ONCE(sb->s_writers.freeze_owner ==
> > > NULL))
> > > + return false;
> > >
> > >
> > > in f15a9ae05a71 ("fs: add owner of freeze/thaw") and failing to
> > > resume from hibernate. Setting it to __builtin_return_address(0)
> > > in filesystems_freeze() makes everything work as expected, so
> > > that's what I'm testing now.
> >
> > +1
> >
> > I'll send the final version out in a bit.
>
> I've now done some extensive testing on loop nested filesystems with
> fio load on the upper. I've tried xfs on ext4 and ext4 on ext4.
> Hibernate/Resume has currently worked on these without a hitch (and the
> fio load burps a bit but then starts running at full speed within a few
> seconds). What I'm doing is a single round of hibernate/resume followed
> by a reboot. I'm relying on the fschecks to detect any filesystem
> corruption. I've also tried doing a couple of fresh starts of the
> hibernated image to check that we did correctly freeze the filesystems.
>
> The problems I've noticed are:
>
> 1. I'm using 9p to push host directories throught and that
> completely hangs after a resume. This is expected because the
> virtio server is out of sync, but it does indicate a need to
> address Jeff's question of what we should be doing for network
> filesystems (and is also the reason I have to reboot after
> resuming).
> 2. Top doesn't show any CPU activity after resume even though fio is
> definitely running. This seems to be a suspend issue and
> unrelated to filesystems, but I'll continue investigating.
To be clear, on the fio run -- are you running fio *while*
suspend/resume cycle on XFS? That used to stall / break suspend
resume. We may want to test dd against a drive too, that will use
the block device cache, and I forget if we have a freeze/thaw for it.
Luis
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-08 17:09 ` Luis Chamberlain
@ 2025-04-08 17:20 ` Luis Chamberlain
2025-04-08 17:26 ` James Bottomley
2025-04-08 17:24 ` James Bottomley
1 sibling, 1 reply; 59+ messages in thread
From: Luis Chamberlain @ 2025-04-08 17:20 UTC (permalink / raw)
To: James Bottomley
Cc: Christian Brauner, linux-fsdevel, jack, rafael, Ard Biesheuvel,
linux-efi, linux-kernel, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
And in case its useful, to test this on a VM you'll need on libvirt:
</os>
<pm>
<suspend-to-mem enabled='yes'/>
<suspend-to-disk enabled='yes'/>
</pm>
The litmus test which used to stall without ever returning was:
while true; do
dd if=/dev/zero of=/path/to/xfs/foo bs=1M count=1024 &> /dev/null
done
To suspend you can use one of these two:
echo mem > /sys/power/state
systemctl suspend
Verify you can resume from suspend:
virsh qemu-monitor-command guest_foo 'query-current-machine'
{"return":{"wakeup-suspend-support":true},"id":"libvirt-415"}
To resume the guest:
virsh dompmwakeup guest_foo
Luis
^ permalink raw reply [flat|nested] 59+ messages in thread* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-08 17:20 ` Luis Chamberlain
@ 2025-04-08 17:26 ` James Bottomley
0 siblings, 0 replies; 59+ messages in thread
From: James Bottomley @ 2025-04-08 17:26 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christian Brauner, linux-fsdevel, jack, rafael, Ard Biesheuvel,
linux-efi, linux-kernel, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, 2025-04-08 at 10:20 -0700, Luis Chamberlain wrote:
> And in case its useful, to test this on a VM you'll need on libvirt:
Just so we're clear, I'm only doing hibernate, not suspend tests. I
figure they should be a bit harsher, but you never know. The reason is
I set up my test rig for efivarfs, which only has a testable problem on
hibernate.
Regards,
James
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-08 17:09 ` Luis Chamberlain
2025-04-08 17:20 ` Luis Chamberlain
@ 2025-04-08 17:24 ` James Bottomley
1 sibling, 0 replies; 59+ messages in thread
From: James Bottomley @ 2025-04-08 17:24 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Christian Brauner, linux-fsdevel, jack, rafael, Ard Biesheuvel,
linux-efi, linux-kernel, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, 2025-04-08 at 10:09 -0700, Luis Chamberlain wrote:
> On Tue, Apr 08, 2025 at 11:43:46AM -0400, James Bottomley wrote:
[...]
> > I've now done some extensive testing on loop nested filesystems
> > with fio load on the upper. I've tried xfs on ext4 and ext4 on
> > ext4. Hibernate/Resume has currently worked on these without a
> > hitch (and the fio load burps a bit but then starts running at full
> > speed within a few seconds). What I'm doing is a single round of
> > hibernate/resume followed by a reboot. I'm relying on the fschecks
> > to detect any filesystem corruption. I've also tried doing a couple
> > of fresh starts of the hibernated image to check that we did
> > correctly freeze the filesystems.
> >
> > The problems I've noticed are:
> >
> > 1. I'm using 9p to push host directories throught and that
> > completely hangs after a resume. This is expected because the
> > virtio server is out of sync, but it does indicate a need to
> > address Jeff's question of what we should be doing for
> > network
> > filesystems (and is also the reason I have to reboot after
> > resuming).
> > 2. Top doesn't show any CPU activity after resume even though
> > fio is
> > definitely running. This seems to be a suspend issue and
> > unrelated to filesystems, but I'll continue investigating.
>
> To be clear, on the fio run -- are you running fio *while*
> suspend/resume cycle on XFS?
Yes, that's why I said "the fio load burps a bit" (as in after resume)
"but then starts running full speed after a few seconds".
> That used to stall / break suspend resume. We may want to test dd
> against a drive too, that will use the block device cache, and I
> forget if we have a freeze/thaw for it.
fio is running a read/write test, but I think all my caches are write
through for safety (although I have verified that the device cache
flush is sent as the last sequence of hibernate).
Regards,
James
^ permalink raw reply [flat|nested] 59+ messages in thread
* Re: [PATCH 0/6] power: wire-up filesystem freeze/thaw with suspend/resume
2025-04-08 15:43 ` James Bottomley
2025-04-08 17:09 ` Luis Chamberlain
@ 2025-04-09 7:17 ` Christian Brauner
1 sibling, 0 replies; 59+ messages in thread
From: Christian Brauner @ 2025-04-09 7:17 UTC (permalink / raw)
To: James Bottomley
Cc: linux-fsdevel, jack, rafael, Ard Biesheuvel, linux-efi,
linux-kernel, mcgrof, hch, david, djwong, pavel, peterz, mingo,
will, boqun.feng
On Tue, Apr 08, 2025 at 11:43:46AM -0400, James Bottomley wrote:
> On Wed, 2025-04-02 at 09:46 +0200, Christian Brauner wrote:
> > On Tue, Apr 01, 2025 at 01:02:07PM -0400, James Bottomley wrote:
> > > On Tue, 2025-04-01 at 02:32 +0200, Christian Brauner wrote:
> > > > The whole shebang can also be found at:
> > > > https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=work.freeze
> > > >
> > > > I know nothing about power or hibernation. I've tested it as best
> > > > as I could. Works for me (TM).
> > >
> > > I'm testing the latest you have in work.freeze and it doesn't
> > > currently work for me. Patch 7b315c39b67d ("power: freeze
> > > filesystems during suspend/resume") doesn't set
> > > filesystems_freeze_ptr so it ends up being NULL and tripping over
> > > this check
> >
> > I haven't pushed the new version there. Sorry about that. I only have
> > it locally.
> >
> > >
> > > +static inline bool may_unfreeze(struct super_block *sb, enum
> > > freeze_holder who,
> > > + const void *freeze_owner)
> > > +{
> > > + WARN_ON_ONCE((who & ~FREEZE_FLAGS));
> > > + WARN_ON_ONCE(hweight32(who & FREEZE_HOLDERS) > 1);
> > > +
> > > + if (who & FREEZE_EXCL) {
> > > + if (WARN_ON_ONCE(sb->s_writers.freeze_owner ==
> > > NULL))
> > > + return false;
> > >
> > >
> > > in f15a9ae05a71 ("fs: add owner of freeze/thaw") and failing to
> > > resume from hibernate. Setting it to __builtin_return_address(0)
> > > in filesystems_freeze() makes everything work as expected, so
> > > that's what I'm testing now.
> >
> > +1
> >
> > I'll send the final version out in a bit.
>
> I've now done some extensive testing on loop nested filesystems with
> fio load on the upper. I've tried xfs on ext4 and ext4 on ext4.
> Hibernate/Resume has currently worked on these without a hitch (and the
> fio load burps a bit but then starts running at full speed within a few
> seconds). What I'm doing is a single round of hibernate/resume followed
> by a reboot. I'm relying on the fschecks to detect any filesystem
> corruption. I've also tried doing a couple of fresh starts of the
> hibernated image to check that we did correctly freeze the filesystems.
>
> The problems I've noticed are:
>
> 1. I'm using 9p to push host directories throught and that
> completely hangs after a resume. This is expected because the
> virtio server is out of sync, but it does indicate a need to
> address Jeff's question of what we should be doing for network
> filesystems (and is also the reason I have to reboot after
> resuming).
No network filesystem supports freeze/thaw so they're not frozen/thawed
during hibernation.
virtiofs doesn't support filesystem freezing either and it warns about
virtio_driver based freezing not being implemented.
^ permalink raw reply [flat|nested] 59+ messages in thread