fs: replace kthread freezing with filesystem freeze/thaw

fs/super.c int thaw_super(struct super_block *sb) · line 1580

Quoted 4 lines · Luis R. Rodriguez

fs/super.c +4 -0

+static bool super_allows_freeze(struct super_block *sb)
+{
+	return !!(sb->s_type->fs_flags & FS_FREEZE_ON_SUSPEND);
+}

A minor comment: if "!!" would be left out the compiler will perform the conversion from int to bool implicitly

so I propose to leave out the "!!" and parentheses.

Anyway, I agree with the approach of this patch and I think that freezing filesystems before processes are frozen would be a big step forward.

Quoted 11 lines · Luis R. Rodriguez

Quoted 8 lines · Bart Van Assche

fs/super.c int thaw_super(struct super_block *sb) · line 1580

Quoted 4 lines · Luis R. Rodriguez

fs/super.c +4 -0

+static bool super_allows_freeze(struct super_block *sb)
+{
+	return !!(sb->s_type->fs_flags & FS_FREEZE_ON_SUSPEND);
+}

A minor comment: if "!!" would be left out the compiler will perform the conversion from int to bool implicitly

For all compilers?

Let's have a look at the output of the following commands:

$ PAGER= git grep 'typedef.*[[:blank:]]bool;' include
include/linux/types.h:typedef _Bool                     bool;
$ PAGER= git grep std= Makefile
Makefile:               -fomit-frame-pointer -std=gnu89 $(HOST_LFS_CFLAGS)
Makefile:                  -std=gnu89 $(call cc-option,-fno-PIE)

From https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/C-Dialect-Options.html#C-Dialect-Options:
‘gnu89’
GNU dialect of ISO C90 (including some C99 features).

I think this means that the Linux kernel tree can only be compiled correctly by compilers that support the C11 type _Bool.

Quoted 7 lines

Quoted 3 lines

Anyway, I agree with the approach of this patch and I think that freezing filesystems before processes are frozen would be a big step forward.

Great! But please note, the current implementation calls fs_suspend_freeze() *after* try_to_freeze_tasks(), ie: this implementation freezes userspace and only after then filesystems.

What will the impact be of that choice on filesystems implemented in userspace?

This uses the existing filesystem freeze and thaw callbacks to freeze each filesystem on suspend/hibernation and thaw upon resume.

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. For instance, even though a kthread may use try_to_freeze() and end up being frozen we have no way of being sure that everything that has been spawned asynchronously from it (such as timers) have also been stopped as well.

A long term advantage of also adding filesystem freeze / thawing supporting durign suspend / hibernation is that long term we may be able to eventually drop the kernel's thread freezing completely as it was originally added to stop disk IO in flight as we hibernate or suspend.

This also implies that many kthread users exist which have been adding freezer semantics onto its kthreads without need. These also will need to be reviewed later.

This is based on prior work originally by Rafael Wysocki and later by Jiri Kosina.

Signed-off-by Luis R. Rodriguez <[email protected]>

This uses the existing filesystem freeze and thaw callbacks to freeze each filesystem on suspend/hibernation and thaw upon resume.

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. For instance, even though a kthread may use try_to_freeze() and end up being frozen we have no way of being sure that everything that has been spawned asynchronously from it (such as timers) have also been stopped as well.

A long term advantage of also adding filesystem freeze / thawing supporting durign suspend / hibernation is that long term we may be able to eventually drop the kernel's thread freezing completely as it was originally added to stop disk IO in flight as we hibernate or suspend.

This also implies that many kthread users exist which have been adding freezer semantics onto its kthreads without need. These also will need to be reviewed later.

This is based on prior work originally by Rafael Wysocki and later by Jiri Kosina.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 fs/super.c             | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h     | 13 +++++++++
 kernel/power/process.c | 14 ++++++++-
 3 files changed, 105 insertions(+), 1 deletion(-)

fs/super.c +0 -0

index d45e92d9a38f..ce8da8b187b1 100644

fs/super.c +31 -0

+
+static bool super_should_freeze(struct super_block *sb)
+{
+	if (!sb->s_root)
+		return false;
+	if (!(sb->s_flags & MS_BORN))
+		return false;
+	/*
+	 * We don't freeze virtual filesystems, we skip those filesystems with
+	 * no backing device.
+	 */
+	if (sb->s_bdi == &noop_backing_dev_info)
+		return false;
+	/* No need to freeze read-only filesystems */
+	if (sb->s_flags & MS_RDONLY)
+		return false;
+	if (!super_allows_freeze(sb))
+		return false;
+
+	return true;
+}
 
+
+int fs_suspend_freeze_sb(struct super_block *sb, void *priv)
+{
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	if (!super_should_freeze(sb))
+		goto out;
+
+	up_read(&sb->s_umount);

Quoted 11 lines · Luis R. Rodriguez

fs/super.c +3 -0

index d45e92d9a38f..ce8da8b187b1 100644
@@ -1572,3 +1572,82 @@ int thaw_super(struct super_block *sb)
 	return 0;
 }
 EXPORT_SYMBOL(thaw_super);
+
+#ifdef CONFIG_PM_SLEEP
+static bool super_allows_freeze(struct super_block *sb)

fs/super.c int thaw_super(struct super_block *sb) · line 1580

Quoted 3 lines

fs/super.c +3 -0

+{
+	return !!(sb->s_type->fs_flags & FS_FREEZE_ON_SUSPEND);
+}

That's a completely misleading function name. All superblocks can be frozen - freeze_super() is filesystem independent. And given that, I don't see why these super_should_freeze() hoops need to be jumped through...

Quoted 15 lines

fs/super.c +15 -0

+int fs_suspend_freeze_sb(struct super_block *sb, void *priv)
+{
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	if (!super_should_freeze(sb))
+		goto out;
+
+	up_read(&sb->s_umount);
+	pr_info("%s (%s): freezing\n", sb->s_type->name, sb->s_id);
+	error = freeze_super(sb);
+	down_read(&sb->s_umount);
+out:
+	if (error && error != -EBUSY)
+		pr_notice("%s (%s): Unable to freeze, error=%d",

fs/super.c int thaw_super(struct super_block *sb) · line 1622

Quoted 4 lines

fs/super.c +4 -0

+			  sb->s_type->name, sb->s_id, error);
+	spin_unlock(&sb_lock);
+	return error;
+}

I don't think this was ever tested. Calling freeze_super() with a spinlock held with through "sleeping in atomic" errors all over the place.

Also, the s_umount lock juggling is nasty. Your new copy+pasted iterate_supers_reverse() takes the lock in read mode, yet all the freeze/thaw callers here want to take it in write mode.

So, really,
iterate_supers_reverse() needs to be iterate_supers_reverse_excl() and take the write lock, and freeze_super/thaw_super need to be factored into locked and unlocked versions.

In which case, we end up with:

int fs_suspend_freeze_sb(struct super_block *sb, void *priv)

{
	return freeze_locked_super(sb);
}

int fs_suspend_thaw_sb(struct super_block *sb, void *priv)

{
	return thaw_locked_super(sb);
}

Quoted 4 lines · Luis R. Rodriguez

fs/ext4/super.c +0 -0

@@ -4926,7 +4926,7 @@ static int ext4_unfreeze(struct super_block *sb)
 		ext4_set_feature_journal_needs_recovery(sb);
 	}
 

fs/ext4/super.c static int ext4_unfreeze(struct super_block *sb) · line 4932

Quoted 5 lines

fs/ext4/super.c +1 -1

-	ext4_commit_super(sb, 1);
+	ext4_commit_super(sb, 0);
 	return 0;
 }
 

After we remove add the NEEDS_RECOVERY flag, we need to make sure recovery flag is pushed out to disk before any other changes are allowed to be pushed out to disk. That's why we originally did the update synchronously.

There are other ways we could fulfill this requirements, but doing a synchronous update is the simplest way to handle this. Was it necessary to change this given the other changes you are making the fs freeze implementation?

Quoted 4 lines

After we remove add the NEEDS_RECOVERY flag, we need to make sure recovery flag is pushed out to disk before any other changes are allowed to be pushed out to disk. That's why we originally did the update synchronously.

I see. I had to try to dig through linux-history to see why this was done, and I actually could not find an exact commit which explained what you just did. Thanks!

Hrm, so on freeze we issue the same commit as well, so is a sync *really* needed on thaw?

No, it was am empirical evaluation done at testing, I observed bio_submit() stalls, we never get completion from the device. Even if we call thaw at the very end of resume, after the devices should be up, we still end up in the same situation. Given how I order freezing filesystems after freezing userspace I do believe we should thaw filesystems prior unfreezing userspace, its why I placed the call where it is now.

Quoted 11 lines · Luis R. Rodriguez

Quoted 4 lines

After we remove add the NEEDS_RECOVERY flag, we need to make sure recovery flag is pushed out to disk before any other changes are allowed to be pushed out to disk. That's why we originally did the update synchronously.

I see. I had to try to dig through linux-history to see why this was done, and I actually could not find an exact commit which explained what you just did. Thanks!

Hrm, so on freeze we issue the same commit as well, so is a sync *really* needed on thaw?

So let me explain what's going on. When we do a freeze, we make sure all of the blocks that are written to the journal are writen to the final location on disk, and the journal is is truncated. (This is called a "journal checkpoint".) Then we clear the NEEDS RECOVERY feature flag and set the EXT4_VALID_FS flags in the superblock. In the no journal case, we flush out all metadata writes, and we set the EXT4_VALID_FS flag. In both cases, we call ext4_commit_super(sb, 1) to make sure the flags update is pushed out to disk. This puts the file system into a "quiscent" state; in fact, it looks like the file system has been unmounted, so that it becomes safe to run dump/restore, run fsck -n on the file system, etc.

The problem on the thaw side is that we need to make sure that EXT4_VALID_FS flag is removed (and if journaling is enabled, the NEEDS RECOVERY feature flag is set) and the superblock is flushed out to the storage device before any other writes are persisted on the disk. In the case where we have journalling enabled, we could wait until the first journal commit to write out the superblock, since in journal mode all metadata updates to the disk are suppressed until the journal commit. We don't do that today, but it is a change we could make.

However, in the no journal node we need to make sure the EXT4_VALID_FS flag is cleared on disk before any other metadata operations can take place, and calling ext4_commit_super(sb, 1) is the only real way to do that.

Quoted 6 lines

No, it was am empirical evaluation done at testing, I observed bio_submit() stalls, we never get completion from the device. Even if we call thaw at the very end of resume, after the devices should be up, we still end up in the same situation. Given how I order freezing filesystems after freezing userspace I do believe we should thaw filesystems prior unfreezing userspace, its why I placed the call where it is now.

So when we call ext4_commit_super(sb, 1), we issue the bio, and then we block waiting for the I/O to complete. That's a blocking call. Is the thaw context one which allows blocking?

If userspace is still
frozen, does that imply that the scheduler isn't allow to run? If that's the case, then that's probably the problem.

More generally, if the thawing code needs to allocate memory, or do

any number of things that could potentially block, this could potentially be an issue.

Or if we have a network block device, or
something else in the storage stack that needs to run a kernel thread context (or a workqueue, etc.) --- is the fact that userspace is frozen mean the scheduler is going to refuse to schedule()?

Basically, before thawing filesystems the rest of the kernel infrastructure needs to have been restarted. i.e. the order needs to be:

freeze userspace
freeze filesystems
freeze kernel threads
freeze workqueues

thaw workqueues
thaw kernel threads
thaw filesystems
thaw userspace

and it should end up that way.

Basically, before thawing filesystems the rest of the kernel infrastructure needs to have been restarted. i.e. the order needs to be:

freeze userspace
freeze filesystems
freeze kernel threads
freeze workqueues

thaw workqueues
thaw kernel threads
thaw filesystems
thaw userspace

and it should end up that way.

Basically, before thawing filesystems the rest of the kernel infrastructure needs to have been restarted. i.e. the order needs to be:

freeze userspace
freeze filesystems
freeze kernel threads
freeze workqueues

thaw workqueues
thaw kernel threads
thaw filesystems
thaw userspace

and it should end up that way.

Quoted 4 lines

Or if we have a network block device, or
something else in the storage stack that needs to run a kernel thread context (or a workqueue, etc.) --- is the fact that userspace is frozen mean the scheduler is going to refuse to schedule()?

No.

Quoted 21 lines · Dave Chinner

Basically, before thawing filesystems the rest of the kernel infrastructure needs to have been restarted. i.e. the order needs to be:

freeze userspace
freeze filesystems
freeze kernel threads
freeze workqueues

thaw workqueues
thaw kernel threads
thaw filesystems
thaw userspace

and it should end up that way.

Quoted 4 lines

Or if we have a network block device, or
something else in the storage stack that needs to run a kernel thread context (or a workqueue, etc.) --- is the fact that userspace is frozen mean the scheduler is going to refuse to schedule()?

Quoted 1 line

No.

Well, that's what the answer *should* be. I was asking what this patch series does, and given that Luis reported that with this patch series ext4_commit_super(sb, 1) is hanging, I have my suspicions about what the answer might be with this patch set. (Especially since the claimed goal of the patch set is, "kthread freezing with filesystem freeze/thaw".

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Signed-off-by Luis R. Rodriguez <[email protected]>

Quoted 5 lines · CEST Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Signed-off-by Luis R. Rodriguez <[email protected]>

I like this one. :-)

Quoted 5 lines · CEST Rafael J. Wysocki

Quoted 4 lines · CEST Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Quoted 3 lines

Quoted 1 line

Signed-off-by Luis R. Rodriguez <[email protected]>

I like this one. :-)

However, suspend_freeze/thaw_processes() require some more work.

In particular, the freezing of workqueues is being removed here without a replacement.

3) Lookup for kthreads which seem to generate IO -- address / review if removal of the freezer API can be done somehow with a quescing. This is currently for example being done on SCSI / md.
4) Only after all the above is done should we consider this patch or some form of it.

Quoted 5 lines · Luis R. Rodriguez

3) Lookup for kthreads which seem to generate IO -- address / review if removal of the freezer API can be done somehow with a quescing. This is currently for example being done on SCSI / md.
4) Only after all the above is done should we consider this patch or some form of it.

After having given this more thought, I think we should omit these last two steps. Modifying the md driver such that it does not submit I/O requests while processes are frozen requires either to use the freezer API or to open-code it. I think there is general agreement in the kernel community that open-coding a single mechanism in multiple drivers is wrong. Does this mean that instead of removing the freezer API we should keep it and review all its users instead?

Quoted 10 lines · Rafael J. Wysocki

Quoted 8 lines · CEST Rafael J. Wysocki

Quoted 5 lines · CEST Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Signed-off-by Luis R. Rodriguez <[email protected]>

I like this one. :-)

Quoted 4 lines

However, suspend_freeze/thaw_processes() require some more work.

In particular, the freezing of workqueues is being removed here without a replacement.

Hrm, where do you see that? freeze_workqueues_busy() is still called on try_to_freeze_tasks(). Likewise thaw_processes() also calls thaw_workqueues(). I did forget to nuke pm_nosig_freezing though.

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Signed-off-by Luis R. Rodriguez <[email protected]>

Can knfsd be a problem?

Can memory management doing background writes be a problem?

Quoted 1 line

Can memory management doing background writes be a problem?

This point I don't understand. What exactly do you mean?

Quoted 1 line

This point I don't understand. What exactly do you mean?

Hibernation:

We freeze, do system snapshot, unfreeze, and write image to disk.

Quoted 6 lines · Pavel Machek

Quoted 1 line

This point I don't understand. What exactly do you mean?

Hibernation:

We freeze, do system snapshot, unfreeze, and write image to disk.

Where/why do you unfreeze between creating the snapshot and writing it out?

Quoted 6 lines · Pavel Machek

Quoted 1 line

This point I don't understand. What exactly do you mean?

Hibernation:

We freeze, do system snapshot, unfreeze, and write image to disk.

Where/why do you unfreeze between creating the snapshot and writing it out?

Again, I agree that the (rare) kthreads that are actually "creating" new I/O have to be somehow frozen and require special care.

Quoted 2 lines

Again, I agree that the (rare) kthreads that are actually "creating" new I/O have to be somehow frozen and require special care.

Agreed. Was any effort made to identify those special kernel threads?

Quoted 4 lines · Pavel Machek

Quoted 2 lines

Again, I agree that the (rare) kthreads that are actually "creating" new I/O have to be somehow frozen and require special care.

Agreed. Was any effort made to identify those special kernel threads?

I don't think there is any other way than just inspecting all the try_to_freeze() instances in the kernel, and understanding what that particular kthread is doing.

I've cleaned up most of the low-hanging fruit already, where the try_to_freeze() was obviously completely pointless, but a lot more time needs to be invested into this.

Quoted 7 lines · Jiri Kosina

Quoted 4 lines · Pavel Machek

Quoted 2 lines

Again, I agree that the (rare) kthreads that are actually "creating" new I/O have to be somehow frozen and require special care.

Agreed. Was any effort made to identify those special kernel threads?

Quoted 8 lines

I don't think there is any other way than just inspecting all the try_to_freeze() instances in the kernel, and understanding what that particular kthread is doing.

I've cleaned up most of the low-hanging fruit already, where the try_to_freeze() was obviously completely pointless, but a lot more time needs to be invested into this.

There are about 36 drivers that call try_to_freeze() and half (18 ) of those are media drivers. Maybe it is easier handle sub-system by sub-system basis for a review of which one of these usages could be removed. cc'ing Mauro and linux-media

Quoted 3 lines · Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Are you surely-sure? You mentioned other in kernel sources of writes; what about those?

Quoted 5 lines · Pavel Machek

Quoted 3 lines · Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Quoted 2 lines

Are you surely-sure? You mentioned other in kernel sources of writes; what about those?

You perhaps did not read the cover letter, it describes this patch will very likely take time to *ever* apply. We must cover tons of grounds first, to address precisely what you say.

Yeah, I was not careful enough reading cover letter. Having series where 1-4/5 are ready to go, and 5/5 not-good-idea for years to come is quite confusing.

What about the many drivers outside filesystems that use the set_freezable() / try_to_freeze() / wait_event_freezable() API?

Quoted 2 lines · Bart Van Assche

What about the many drivers outside filesystems that use the set_freezable() / try_to_freeze() / wait_event_freezable() API?

Many/most of them are just completely bogus and pointless. I've killed a lot of those in the past, but the copy/paste programming is just too strong enemy to fight against.

If just a single driver would use that API to prevent that I/O occurs while processes are frozen then this patch will break that driver. I would like to know your opinion about the following patch in particular: "[PATCH v4 1/7] md:
Make md resync and reshape threads freezable"
(https://www.spinics.net/lists/linux-block/msg17854.html).

Quoted 7 lines · Jiri Kosina

Quoted 2 lines · Bart Van Assche

What about the many drivers outside filesystems that use the set_freezable() / try_to_freeze() / wait_event_freezable() API?

Many/most of them are just completely bogus and pointless. I've killed a lot of those in the past, but the copy/paste programming is just too strong enemy to fight against.

If just a single driver would use that API to prevent that I/O occurs while processes are frozen then this patch will break that driver.

Quoted 2 lines

What about the many drivers outside filesystems that use the set_freezable() / try_to_freeze() / wait_event_freezable() API?

Many/most of them are just completely bogus and pointless.

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

kernel/power/process.c +0 -6

-/**
- * freeze_kernel_threads - Make freezable kernel threads go to the refrigerator.
- *
- * On success, returns 0.  On failure, -errno and only the kernel threads are
- * thawed, so as to give a chance to the caller to do additional cleanups
- * (if any) before thawing the userspace tasks. So, it is the responsibility

kernel/power/process.c +0 -11

-	if (!error)
-		pr_cont("done.");
-
-	pr_cont("\n");
-	BUG_ON(in_atomic());
-
-	if (error)
-		thaw_kernel_threads();
-	return error;
-}
-
 void thaw_processes(void)
 {
 	struct task_struct *g, *p;

@@ -234,23 +207,3 @@ void thaw_processes(void)
 	pr_cont("done.\n");
 	trace_suspend_resume(TPS("thaw_processes"), 0, false);

Quoted 3 lines · Luis R. Rodriguez

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Really? There's no other subsystem that relies on kernel thread and workqueue freezing to function correctly on suspend?

Now that all filesystems which used to rely on kthread freezing have been converted to filesystem freeze/thawing we can remove the kernel kthread freezer.

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 Documentation/power/freezing-of-tasks.txt |  9 ------
 drivers/xen/manage.c                      |  6 ----
 include/linux/freezer.h                   |  4 ---
 kernel/power/hibernate.c                  | 10 ++-----
 kernel/power/power.h                      | 20 +------------
 kernel/power/process.c                    | 47 -------------------------------
 kernel/power/user.c                       |  9 ------
 tools/power/pm-graph/analyze_suspend.py   |  1 -
 8 files changed, 3 insertions(+), 103 deletions(-)

At the 2015 South Korea kernel summit Jiri Kosina had pointd out the issue of the sloppy semantics of the kthread freezer, lwn covered this pretty well [0]. In short, he explained how the original purpose of the freezer was to aid in going to suspend to ensure no uwanted IO activity would cause filesystem corruption. Kernel kthreads require special freezer handling though, the call try_to_freeze() is often sprinkled at strategic places, but sometimes this is done without set_freezable() making try_to_freeze() useless. Other helpers such as freezable_schedule_timeout() exist, and very likely they are not used in any consistent and proper way either all over the kernel. Dealing with these helpers alone also does not and cannot ensure that everything that has been spawned asynchronously from a kthread (such as timers) are stopped as well, these are left to the kthread user implementation, and chances are pretty high there are many bugs lurking here. There are even non-IO bounds kthreads now using the freezer APIs, where this is not even needed!

Jiri suggested we can easily replace the complexities of the kthread freezer by just using the existing filesystem freeze/thaw callbacks on hibernation and suspend.

I've picked up Jiri's work given there are bugs which after inspection don't see like real bugs, but just random IO loosely waiting to be completed and the kernel not really being able to tell me who the culprit is. In fact even if one plugs a fix, one ends up in another random place and its really unclear who is to blaim for next.

For instance, to reproduce a simple suspend bug on what may at first seem to be an XFS bug, one can issue a dd onto disk prior to suspend, and we'll get a stall on our way to suspend, claiming the issue was the SCSI layer not being able to quiesce the disk. This was reported on OpenSUSE and reproduced on linux-next easily [1]. The following script can be run while we loop on systemctl suspend and qemu system_wakeup calls to resume:

	while true; do
		dd if=/dev/zero of=crap bs=1M count=1024 &> /dev/null
	done

You end up with with a hung suspend attempt, and eventually a splat as follows with a hunk task notification:

INFO: task kworker/u8:8:1320 blocked for more than 10 seconds.
      Tainted: G            E   4.13.0-next-20170907+ #88
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u8:8    D    0  1320      2 0x80000000
Workqueue: events_unbound async_run_entry_fn
Call Trace:
 __schedule+0x2ec/0x7a0
 schedule+0x36/0x80
 io_schedule+0x16/0x40
 get_request+0x278/0x780
 ? remove_wait_queue+0x70/0x70
 blk_get_request+0x9c/0x110
 scsi_execute+0x7a/0x310 [scsi_mod]
 sd_sync_cache+0xa3/0x190 [sd_mod]
 ? blk_run_queue+0x3f/0x50
 sd_suspend_common+0x7b/0x130 [sd_mod]
 ? scsi_print_result+0x270/0x270 [scsi_mod]
 sd_suspend_system+0x13/0x20 [sd_mod]
 do_scsi_suspend+0x1b/0x30 [scsi_mod]
 scsi_bus_suspend_common+0xb1/0xd0 [scsi_mod]
 ? device_for_each_child+0x69/0x90
 scsi_bus_suspend+0x15/0x20 [scsi_mod]
 dpm_run_callback+0x56/0x140
 ? scsi_bus_freeze+0x20/0x20 [scsi_mod]
 __device_suspend+0xf1/0x340

 async_suspend+0x1f/0xa0
 async_run_entry_fn+0x38/0x160
 process_one_work+0x191/0x380
 worker_thread+0x4e/0x3c0
 kthread+0x109/0x140
 ? process_one_work+0x380/0x380
 ? kthread_create_on_node+0x70/0x70
 ret_from_fork+0x25/0x30

Quoted 25 lines · Luis R. Rodriguez

INFO: task kworker/u8:8:1320 blocked for more than 10 seconds.
      Tainted: G            E   4.13.0-next-20170907+ #88
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u8:8    D    0  1320      2 0x80000000
Workqueue: events_unbound async_run_entry_fn
Call Trace:
 __schedule+0x2ec/0x7a0
 schedule+0x36/0x80
 io_schedule+0x16/0x40
 get_request+0x278/0x780
 ? remove_wait_queue+0x70/0x70
 blk_get_request+0x9c/0x110
 scsi_execute+0x7a/0x310 [scsi_mod]
 sd_sync_cache+0xa3/0x190 [sd_mod]
 ? blk_run_queue+0x3f/0x50
 sd_suspend_common+0x7b/0x130 [sd_mod]
 ? scsi_print_result+0x270/0x270 [scsi_mod]
 sd_suspend_system+0x13/0x20 [sd_mod]
 do_scsi_suspend+0x1b/0x30 [scsi_mod]
 scsi_bus_suspend_common+0xb1/0xd0 [scsi_mod]
 ? device_for_each_child+0x69/0x90
 scsi_bus_suspend+0x15/0x20 [scsi_mod]
 dpm_run_callback+0x56/0x140
 ? scsi_bus_freeze+0x20/0x20 [scsi_mod]
 __device_suspend+0xf1/0x340

Quoted 8 lines

 async_suspend+0x1f/0xa0
 async_run_entry_fn+0x38/0x160
 process_one_work+0x191/0x380
 worker_thread+0x4e/0x3c0
 kthread+0x109/0x140
 ? process_one_work+0x380/0x380
 ? kthread_create_on_node+0x70/0x70
 ret_from_fork+0x25/0x30

Actually we are trying to fix this issue inside block layer/SCSI, please see the following link:

https://marc.info/?l=linux-scsi&m=150703947029304&w=2

Even though this patch can make kthread to not do I/O during suspend/resume, the SCSI quiesce still can cause similar issue in other case, like when sending SCSI domain validation to transport_spi, which happens in revalidate path, nothing to do with suspend/resume.

So IMO the root cause is in SCSI's quiesce.

You can find the similar description in above link:

Once SCSI device is put into QUIESCE, no new request except for RQF_PREEMPT can be dispatched to SCSI successfully, and scsi_device_quiesce() just simply waits for completion of I/Os dispatched to SCSI stack. It isn't enough at all.

Because new request still can be coming, but all the allocated requests can't be dispatched successfully, so request pool can be consumed up easily. Then RQF_PREEMPT can't be allocated, and hang forever, just like the stack trace you posted.

Quoted 40 lines · Ming Lei

Quoted 25 lines · Luis R. Rodriguez

INFO: task kworker/u8:8:1320 blocked for more than 10 seconds.
      Tainted: G            E   4.13.0-next-20170907+ #88
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u8:8    D    0  1320      2 0x80000000
Workqueue: events_unbound async_run_entry_fn
Call Trace:
 __schedule+0x2ec/0x7a0
 schedule+0x36/0x80
 io_schedule+0x16/0x40
 get_request+0x278/0x780
 ? remove_wait_queue+0x70/0x70
 blk_get_request+0x9c/0x110
 scsi_execute+0x7a/0x310 [scsi_mod]
 sd_sync_cache+0xa3/0x190 [sd_mod]
 ? blk_run_queue+0x3f/0x50
 sd_suspend_common+0x7b/0x130 [sd_mod]
 ? scsi_print_result+0x270/0x270 [scsi_mod]
 sd_suspend_system+0x13/0x20 [sd_mod]
 do_scsi_suspend+0x1b/0x30 [scsi_mod]
 scsi_bus_suspend_common+0xb1/0xd0 [scsi_mod]
 ? device_for_each_child+0x69/0x90
 scsi_bus_suspend+0x15/0x20 [scsi_mod]
 dpm_run_callback+0x56/0x140
 ? scsi_bus_freeze+0x20/0x20 [scsi_mod]
 __device_suspend+0xf1/0x340

Quoted 8 lines

 async_suspend+0x1f/0xa0
 async_run_entry_fn+0x38/0x160
 process_one_work+0x191/0x380
 worker_thread+0x4e/0x3c0
 kthread+0x109/0x140
 ? process_one_work+0x380/0x380
 ? kthread_create_on_node+0x70/0x70
 ret_from_fork+0x25/0x30

Actually we are trying to fix this issue inside block layer/SCSI, please see the following link:

https://marc.info/?l=linux-scsi&m=150703947029304&w=2

Quoted 5 lines

Even though this patch can make kthread to not do I/O during suspend/resume, the SCSI quiesce still can cause similar issue in other case, like when sending SCSI domain validation to transport_spi, which happens in revalidate path, nothing to do with suspend/resume.

Are you saying that the SCSI layer can generate IO even without the filesystem triggering it?

Quoted 7 lines · Luis R. Rodriguez

Quoted 5 lines · Ming Lei

Even though this patch can make kthread to not do I/O during suspend/resume, the SCSI quiesce still can cause similar issue in other case, like when sending SCSI domain validation to transport_spi, which happens in revalidate path, nothing to do with suspend/resume.

Quoted 2 lines

Are you saying that the SCSI layer can generate IO even without the filesystem triggering it?

The SCSI layer can send SCSI commands; they aren't I/Os in the sense that they do reads and writes to media,

but they are block requests. Maybe those should be allowed even to frozen devices?

Quoted 40 lines · Ming Lei

Quoted 25 lines · Luis R. Rodriguez

INFO: task kworker/u8:8:1320 blocked for more than 10 seconds.
      Tainted: G            E   4.13.0-next-20170907+ #88
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
kworker/u8:8    D    0  1320      2 0x80000000
Workqueue: events_unbound async_run_entry_fn
Call Trace:
 __schedule+0x2ec/0x7a0
 schedule+0x36/0x80
 io_schedule+0x16/0x40
 get_request+0x278/0x780
 ? remove_wait_queue+0x70/0x70
 blk_get_request+0x9c/0x110
 scsi_execute+0x7a/0x310 [scsi_mod]
 sd_sync_cache+0xa3/0x190 [sd_mod]
 ? blk_run_queue+0x3f/0x50
 sd_suspend_common+0x7b/0x130 [sd_mod]
 ? scsi_print_result+0x270/0x270 [scsi_mod]
 sd_suspend_system+0x13/0x20 [sd_mod]
 do_scsi_suspend+0x1b/0x30 [scsi_mod]
 scsi_bus_suspend_common+0xb1/0xd0 [scsi_mod]
 ? device_for_each_child+0x69/0x90
 scsi_bus_suspend+0x15/0x20 [scsi_mod]
 dpm_run_callback+0x56/0x140
 ? scsi_bus_freeze+0x20/0x20 [scsi_mod]
 __device_suspend+0xf1/0x340

Quoted 8 lines

 async_suspend+0x1f/0xa0
 async_run_entry_fn+0x38/0x160
 process_one_work+0x191/0x380
 worker_thread+0x4e/0x3c0
 kthread+0x109/0x140
 ? process_one_work+0x380/0x380
 ? kthread_create_on_node+0x70/0x70
 ret_from_fork+0x25/0x30

Actually we are trying to fix this issue inside block layer/SCSI, please see the following link:

https://marc.info/?l=linux-scsi&m=150703947029304&w=2

Quoted 5 lines

Even though this patch can make kthread to not do I/O during suspend/resume, the SCSI quiesce still can cause similar issue in other case, like when sending SCSI domain validation to transport_spi, which happens in revalidate path, nothing to do with suspend/resume.

Are you saying that the SCSI layer can generate IO even without the filesystem triggering it?

If so then by all means these are certainly other areas we should address quiescing as I noted in my email.

Also, *iff* the generated IO is triggered on the SCSI suspend callback, then clearly the next question is if this is truly needed. If so then yes, it should be quiesced and all restrictions should be considered.

Note that device pm ops get called first, then later the notifiers are processed, and only later is userspace frozen. Its this gap this patch set addresses, and its also where where I saw the issue creep in. Depending on the questions above we may or not need more work in other layers.

So I am not saying this patch set is sufficient to address all IO quiescing, quite the contrary I acknowledged that each subsystem should vet if they have non-FS generated IO (seems you and Bart are doing great job at doing this analysis on SCSI). This patchset however should help with odd corner cases which *are* triggered by the FS and the spaghetti code requirements of the kthread freezing clearly does not suffice.

Quoted 9 lines

So IMO the root cause is in SCSI's quiesce.

You can find the similar description in above link:

Once SCSI device is put into QUIESCE, no new request except for RQF_PREEMPT can be dispatched to SCSI successfully, and scsi_device_quiesce() just simply waits for completion of I/Os dispatched to SCSI stack. It isn't enough at all.

I see so the race here is *on* the pm ops of SCSI we have generated IO to QUIESCE.

Quoted 6 lines

Because new request still can be coming, but all the allocated requests can't be dispatched successfully, so request pool can be consumed up easily. Then RQF_PREEMPT can't be allocated, and hang forever, just like the stack trace you posted.

I see. Makes sense. So SCSI quiesce has restrictions and they're being violated.

Anyway, don't think of this as a replacement for yours or Bart's work then, but rather supplemental.

Are you saying we should not move forward with this patch set, or simply that the above splat is rather properly fixed with SCSI quiescing? Given you're explanation I'd have to agree. But even with this considered and accepted, from a theoretical perspective -- why would this patch set actually seem to fix the same issue? Is it, that it just *seems* to fix it?