fs: use freeze_fs on suspend/hibernate

freeze_super() holds a write lock, however we wish to also enable callers which already hold the write lock. To do this provide a helper and make freeze_super() use it. This way, all that freeze_super() does now is lock handling and active count management.

This change has no functional changes.

Suggested-by Dave Chinner <[email protected]>

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

---
 fs/super.c | 100 +++++++++++++++++++++++++++++++++----------------------------
 1 file changed, 55 insertions(+), 45 deletions(-)

fs/super.c +55 -45

index d4e33e8f1e6f..a7650ff22f0e 100644
@@ -1387,59 +1387,20 @@ static void sb_freeze_unlock(struct super_block *sb)
 		percpu_up_write(sb->s_writers.rw_sem + level);
 }
 
-/**
- * freeze_super - lock the filesystem and force it into a consistent state
- * @sb: the super to lock
- *
- * 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 will return
- * -EBUSY.
- *
- * During this function, sb->s_writers.frozen goes through these values:
- *
- * SB_UNFROZEN: File system is normal, all writes progress as usual.
- *
- * SB_FREEZE_WRITE: The file system is in the process of being frozen.  New
- * writes should be blocked, though page faults are still allowed. We wait for
- * all writes to complete and then proceed to the next stage.
- *
- * SB_FREEZE_PAGEFAULT: Freezing continues. Now also page faults are blocked
- * but internal fs threads can still modify the filesystem (although they
- * should not dirty new pages or inodes), writeback can run etc. After waiting
- * for all running page faults we sync the filesystem which will clean all
- * dirty pages and inodes (no new dirty pages or inodes can be created when
- * sync is running).
- *
- * SB_FREEZE_FS: The file system is frozen. Now all internal sources of fs
- * modification are blocked (e.g. XFS preallocation truncation on inode
- * reclaim). This is usually implemented by blocking new transactions for
- * filesystems that have them and need this additional guard. After all
- * internal writers are finished we call ->freeze_fs() to finish filesystem
- * freezing. Then we transition to SB_FREEZE_COMPLETE state. This state is
- * mostly auxiliary for filesystems to verify they do not modify frozen fs.
- *
- * sb->s_writers.frozen is protected by sb->s_umount.
- */
-int freeze_super(struct super_block *sb)
+/* Caller takes lock and handles active count */
+static int freeze_locked_super(struct super_block *sb)
 {
 	int ret;
 
-	atomic_inc(&sb->s_active);
-	down_write(&sb->s_umount);
-	if (sb->s_writers.frozen != SB_UNFROZEN) {
-		deactivate_locked_super(sb);
+	if (sb->s_writers.frozen != SB_UNFROZEN)
 		return -EBUSY;
-	}
 
-	if (!(sb->s_flags & SB_BORN)) {
-		up_write(&sb->s_umount);
+	if (!(sb->s_flags & SB_BORN))
 		return 0;	/* sic - it's "nothing to do" */
-	}
 
 	if (sb_rdonly(sb)) {
 		/* Nothing to do really... */
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
-		up_write(&sb->s_umount);
 		return 0;
 	}
 

@@ -1468,7 +1429,6 @@ int freeze_super(struct super_block *sb)
 			sb->s_writers.frozen = SB_UNFROZEN;
 			sb_freeze_unlock(sb);
 			wake_up(&sb->s_writers.wait_unfrozen);
-			deactivate_locked_super(sb);
 			return ret;
 		}
 	}

@@ -1478,9 +1438,59 @@ int freeze_super(struct super_block *sb)
 	 */
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 	lockdep_sb_freeze_release(sb);
-	up_write(&sb->s_umount);
 	return 0;
 }
+
+/**
+ * freeze_super - lock the filesystem and force it into a consistent state
+ * @sb: the super to lock
+ *
+ * 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 will return
+ * -EBUSY.
+ *
+ * During this function, sb->s_writers.frozen goes through these values:
+ *
+ * SB_UNFROZEN: File system is normal, all writes progress as usual.
+ *
+ * SB_FREEZE_WRITE: The file system is in the process of being frozen.  New
+ * writes should be blocked, though page faults are still allowed. We wait for
+ * all writes to complete and then proceed to the next stage.
+ *
+ * SB_FREEZE_PAGEFAULT: Freezing continues. Now also page faults are blocked
+ * but internal fs threads can still modify the filesystem (although they
+ * should not dirty new pages or inodes), writeback can run etc. After waiting
+ * for all running page faults we sync the filesystem which will clean all
+ * dirty pages and inodes (no new dirty pages or inodes can be created when
+ * sync is running).
+ *
+ * SB_FREEZE_FS: The file system is frozen. Now all internal sources of fs
+ * modification are blocked (e.g. XFS preallocation truncation on inode
+ * reclaim). This is usually implemented by blocking new transactions for
+ * filesystems that have them and need this additional guard. After all
+ * internal writers are finished we call ->freeze_fs() to finish filesystem
+ * freezing. Then we transition to SB_FREEZE_COMPLETE state. This state is
+ * mostly auxiliary for filesystems to verify they do not modify frozen fs.
+ *
+ * sb->s_writers.frozen is protected by sb->s_umount.
+ */
+int freeze_super(struct super_block *sb)
+{
+	int error;
+
+	atomic_inc(&sb->s_active);
+
+	down_write(&sb->s_umount);
+	error = freeze_locked_super(sb);
+	if (error) {
+		deactivate_locked_super(sb);
+		goto out;
+	}
+	up_write(&sb->s_umount);
+
+out:
+	return error;
+}
 EXPORT_SYMBOL(freeze_super);
 
 /**

Signature · 1 line

2.15.0

thaw_super() hold a write lock, however we wish to also enable callers which already hold the write lock. To do this provide a helper and make thaw_super() use it. This way, all that thaw_super() does now is lock handling and active count management.

This change has no functional changes.

Suggested-by Dave Chinner <[email protected]>

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

---
 fs/super.c | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

fs/super.c +26 -13

index a7650ff22f0e..cecc279beecd 100644
@@ -1493,21 +1493,13 @@ int freeze_super(struct super_block *sb)
 }
 EXPORT_SYMBOL(freeze_super);
 
-/**
- * thaw_super -- unlock filesystem
- * @sb: the super to thaw
- *
- * Unlocks the filesystem and marks it writeable again after freeze_super().
- */
-int thaw_super(struct super_block *sb)
+/* Caller takes lock and handles active count */
+static int thaw_locked_super(struct super_block *sb)
 {
 	int error;
 
-	down_write(&sb->s_umount);
-	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE) {
-		up_write(&sb->s_umount);
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
 		return -EINVAL;
-	}
 
 	if (sb_rdonly(sb)) {
 		sb->s_writers.frozen = SB_UNFROZEN;

@@ -1522,7 +1514,6 @@ int thaw_super(struct super_block *sb)
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			up_write(&sb->s_umount);
 			return error;
 		}
 	}

@@ -1531,7 +1522,29 @@ int thaw_super(struct super_block *sb)
 	sb_freeze_unlock(sb);
 out:
 	wake_up(&sb->s_writers.wait_unfrozen);
-	deactivate_locked_super(sb);
 	return 0;
 }
+
+/**
+ * thaw_super -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ */
+int thaw_super(struct super_block *sb)
+{
+	int error;
+
+	down_write(&sb->s_umount);
+	error = thaw_locked_super(sb);
+	if (error) {
+		up_write(&sb->s_umount);
+		goto out;
+	}
+
+	deactivate_locked_super(sb);
+
+out:
+	return error;
+}
 EXPORT_SYMBOL(thaw_super);

Signature · 1 line

2.15.0

The question of whether or not a superblock is frozen needs to be augmented in the future to account for differences between a user initiated freeze and a kernel initiated freeze done automatically on behalf of the kernel.

Provide helpers so that these can be used instead so that we don't have to expand checks later in these same call sites as we expand the definition of a frozen superblock.

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

... I dislike the _by_user() suffix as there may be different places that call freeze_super() (e.g. device mapper does this during some operations). Clearly we need to distinguish "by system suspend" and "the other" cases. So please make this clear in the naming.

In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration.

Quoted 4 lines · Jan Kara

... I dislike the _by_user() suffix as there may be different places that call freeze_super() (e.g. device mapper does this during some operations). Clearly we need to distinguish "by system suspend" and "the other" cases. So please make this clear in the naming.

Ah. How about sb_frozen_by_cb() ?

Quoted 6 lines

In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration.

Ah, there are three important reasons for doing it the way I did it which are easy to miss, unless you read the commit log message very carefully.

0) The ioctl interface causes a failure to be sent back to userspace if you issue two consecutive freezes, or two thaws. Ie, once a filesystem is frozen, a secondary call will result in an error. Likewise for thaw.

1) The new iterate supers stuff I added bail on the first error and return that error. If we kept the ioctl() interface error scheme we'd be erroring out if on suspend if userspace had already frozen a filesystem. Clearly that'd be silly so we need to distinguish between the automatic kernel freezing and the old userspace ioctl initiated interface, so that we can keep the old behaviour but allow in-kernel auto freeze on suspend to work properly.

2) If we fail to suspend we need to then thaw up all filesystems. The order in which we try to freeze is in reverse order on the super_block list. If we fail though we iterate in proper order on the super_block list and thaw. If you had two filesystems this means that if a failure happened on freezing the first filesystem, we'd first thaw the other filesystem -- and because of
0) if we don't distinguish between the ioctl interface or auto freezing, we'd also fail on thaw'ing given the other superblock wouldn't have been frozen.

So we need to keep two separate approaches. The count stuff would not suffice to distinguish origin of source for freeze call.

Come to think of it, I think I forgot to avoid thaw if the freeze was ioctl initiated..

thaw_unlocked(bool cb_call)
{
if (sb_frozen_by_cb(sb) && !cb_call)
return 0; /* skip as the user wanted to keep this fs frozen */
...
}

Even though the kernel auto call is new I think we need to keep ioctl initiated frozen filesystems frozen to not break old userspace assumptions.

So, keeping all this in mind, does a count method still suffice?

Quoted 7 lines · Luis R. Rodriguez

Quoted 4 lines · Jan Kara

... I dislike the _by_user() suffix as there may be different places that call freeze_super() (e.g. device mapper does this during some operations). Clearly we need to distinguish "by system suspend" and "the other" cases. So please make this clear in the naming.

Ah. How about sb_frozen_by_cb() ?

And what does 'cb' stand for? :)

Quoted 10 lines

Quoted 6 lines

In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration.

Ah, there are three important reasons for doing it the way I did it which are easy to miss, unless you read the commit log message very carefully.

Quoted 3 lines

0) The ioctl interface causes a failure to be sent back to userspace if you issue two consecutive freezes, or two thaws. Ie, once a filesystem is frozen, a secondary call will result in an error. Likewise for thaw.

Yep. But also note that there's *another* interface to filesystem freezing which behaves differently - freeze_bdev() (used internally by dm). That interface uses the counter and freezing of already frozen device succeeds.

IOW it is a mess.

We cannot change the behavior of the ioctl but we could
still provide an in-kernel interface to freeze_super() with the same semantics as freeze_bdev() which might be easier to use by suspend - maybe we could call it 'exclusive' (for the current freeze_super() semantics) and 'non-exclusive' (for the freeze_bdev() semantics) since this is very much like O_EXCL open of block devices...

Quoted 6 lines

1) The new iterate supers stuff I added bail on the first error and return that error. If we kept the ioctl() interface error scheme we'd be erroring out if on suspend if userspace had already frozen a filesystem. Clearly that'd be silly so we need to distinguish between the automatic kernel freezing and the old userspace ioctl initiated interface, so that we can keep the old behaviour but allow in-kernel auto freeze on suspend to work properly.

This would work fine with the non-exclusive semantics I believe.

Quoted 21 lines

2) If we fail to suspend we need to then thaw up all filesystems. The order in which we try to freeze is in reverse order on the super_block list. If we fail though we iterate in proper order on the super_block list and thaw. If you had two filesystems this means that if a failure happened on freezing the first filesystem, we'd first thaw the other filesystem -- and because of
0) if we don't distinguish between the ioctl interface or auto freezing, we'd also fail on thaw'ing given the other superblock wouldn't have been frozen.

So we need to keep two separate approaches. The count stuff would not suffice to distinguish origin of source for freeze call.

Come to think of it, I think I forgot to avoid thaw if the freeze was ioctl initiated..

thaw_unlocked(bool cb_call)
{
if (sb_frozen_by_cb(sb) && !cb_call)
return 0; /* skip as the user wanted to keep this fs frozen */
...
}

Quoted 4 lines

Even though the kernel auto call is new I think we need to keep ioctl initiated frozen filesystems frozen to not break old userspace assumptions.

So, keeping all this in mind, does a count method still suffice?

The count method would need a different error recovery method - i.e. if you fail freezing filesystems somewhere in the middle of iteration through superblock list, you'd need to iterate from that point on to the superblock where you've started. This is somewhat more complicated than your approach but it seems cleaner to me:

1) Function freezing all superblocks either succeeds and all superblocks are frozen or fails and no superblocks are (additionally) frozen.

2) It is not that normal users + one special user (who owns the "flag" in the superblock in form of a special freeze state) setup. We'd simply have exclusive and non-exclusive users of superblock freezing and there can be arbitrary numbers of them.

Quoted 11 lines · Jan Kara

Quoted 10 lines · Luis R. Rodriguez

Quoted 6 lines

In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration.

Ah, there are three important reasons for doing it the way I did it which are easy to miss, unless you read the commit log message very carefully.

Quoted 7 lines

Quoted 3 lines

0) The ioctl interface causes a failure to be sent back to userspace if you issue two consecutive freezes, or two thaws. Ie, once a filesystem is frozen, a secondary call will result in an error. Likewise for thaw.

Yep. But also note that there's *another* interface to filesystem freezing which behaves differently - freeze_bdev() (used internally by dm). That interface uses the counter and freezing of already frozen device succeeds.

Ah... so freeze_bdev() semantics matches the same semantics I was looking for.

Quoted 1 line

IOW it is a mess.

To say the least.

Quoted 6 lines

We cannot change the behavior of the ioctl but we could
still provide an in-kernel interface to freeze_super() with the same semantics as freeze_bdev() which might be easier to use by suspend - maybe we could call it 'exclusive' (for the current freeze_super() semantics) and 'non-exclusive' (for the freeze_bdev() semantics) since this is very much like O_EXCL open of block devices...

Sure, now typically I see we make exclusive calls with the postfix _excl() so I take it you'd be OK in renaming freeze_super() freeze_super_excl() eventually then?

I totally missed freeze_bdev() otherwise I think I would have picked up on the shared semantics stuff and I would have just made a helper out of what freeze_bdev() uses, and then have both in-kernel and freeze_bdev() use it.

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

Quoted 4 lines

2) It is not that normal users + one special user (who owns the "flag" in the superblock in form of a special freeze state) setup. We'd simply have exclusive and non-exclusive users of superblock freezing and there can be arbitrary numbers of them.

Sorry I did not understand this point. Can you rephrase perhaps a bit?

Anyway, I just tried implementing this and it seemed rather easy to use a pivot, however note that then freeze_processes() which calls fs_suspend_freeze() would somehow need to pass the failed sb... do we want to have let fs_suspend_freeze() pass a parameter to be set to the failed sb of it failed? Locking-wise this seems racy.

Hello,

I think I owe you a reply here... Sorry that it took so long.

Quoted 8 lines

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

I believe nobody really thought about mixing those two interfaces to fs freezing and so the behavior is basically defined by the implementation. That is:

freeze_bdev() on sb frozen by ioctl_fsfreeze() -> EBUSY

freeze_bdev() on sb frozen by freeze_bdev() -> success ioctl_fsfreeze() on sb frozen by freeze_bdev() -> EBUSY ioctl_fsfreeze() on sb frozen by ioctl_fsfreeze() -> EBUSY

thaw_bdev() on sb frozen by ioctl_fsfreeze() -> EINVAL

ioctl_fsthaw() on sb frozen by freeze_bdev() -> success

What I propose is the following API:

freeze_super_excl()
- freezes superblock, returns EBUSY if the superblock is already frozen (either by another freeze_super_excl() or by freeze_super())
freeze_super()
- this function will make sure superblock is frozen when the function returns with success.

It can be nested with other freeze_super() or
freeze_super_excl() calls

(this second part is different from how
freeze_bdev() behaves currently but AFAICT this behavior is actually what all current users of freeze_bdev() really want - just make sure fs cannot be written to)

Quoted 10 lines · Jan Kara

Quoted 8 lines · Luis R. Rodriguez

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

Quoted 5 lines

I believe nobody really thought about mixing those two interfaces to fs freezing and so the behavior is basically defined by the implementation. That is:

freeze_bdev() on sb frozen by ioctl_fsfreeze() -> EBUSY

Note below as well on your *future* freeze_super() implementation.

Quoted 5 lines

freeze_bdev() on sb frozen by freeze_bdev() -> success ioctl_fsfreeze() on sb frozen by freeze_bdev() -> EBUSY ioctl_fsfreeze() on sb frozen by ioctl_fsfreeze() -> EBUSY

thaw_bdev() on sb frozen by ioctl_fsfreeze() -> EINVAL

Phew, so this is what we want for the in-kernel freezing so we're good and *can* combine these then.

Quoted 10 lines

ioctl_fsthaw() on sb frozen by freeze_bdev() -> success

What I propose is the following API:

freeze_super_excl()
- freezes superblock, returns EBUSY if the superblock is already frozen (either by another freeze_super_excl() or by freeze_super())
freeze_super()
- this function will make sure superblock is frozen when the function returns with success.

That's straight forward.

Quoted 2 lines

It can be nested with other freeze_super() or
freeze_super_excl() calls

This is where it can get hairy. More below.

Quoted 4 lines

(this second part is different from how
freeze_bdev() behaves currently but AFAICT this behavior is actually what all current users of freeze_bdev() really want - just make sure fs cannot be written to)

If we can agree to this, then sure. However there are two types of possible nested calls to consider, one where the sb was already frozen by an IOCTL, and the other where it was initiated by either another freeze_super_excl() or another freeze_super() call which is currently being processed. For the first type, its easy to say the device is already frozen as such return success. If the freezing is ongoing, we may want to wait or not wait, and this will depend on our current use cases for freeze_bdev().

As you noted above, freeze_bdev() currently returns EBUSY if we had the sb already frozen by ioctl_fsfreeze(). It may be a welcomed enhancement to correct the semantics first to address the first case, but keep the EBUSY for the other case. A secondary patch could then add a completion mechanism and let callers decide to either wait or not. *Iff* the caller did not opt-in to wait we keep the EBUSY return.

Quoted 3 lines · Jan Kara

Hello,

I think I owe you a reply here... Sorry that it took so long.

Took me just as long :)

Quoted 19 lines · Luis R. Rodriguez

Quoted 8 lines

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

I believe nobody really thought about mixing those two interfaces to fs freezing and so the behavior is basically defined by the implementation. That is:

freeze_bdev() on sb frozen by ioctl_fsfreeze() -> EBUSY freeze_bdev() on sb frozen by freeze_bdev() -> success ioctl_fsfreeze() on sb frozen by freeze_bdev() -> EBUSY ioctl_fsfreeze() on sb frozen by ioctl_fsfreeze() -> EBUSY

Quoted 1 line

thaw_bdev() on sb frozen by ioctl_fsfreeze() -> EINVAL

Phew, so this is what we want for the in-kernel freezing so we're good and *can* combine these then.

Quoted 26 lines · Luis R. Rodriguez

Quoted 3 lines · Jan Kara

Hello,

I think I owe you a reply here... Sorry that it took so long.

Took me just as long :)

Quoted 19 lines · Luis R. Rodriguez

Quoted 8 lines

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

I believe nobody really thought about mixing those two interfaces to fs freezing and so the behavior is basically defined by the implementation. That is:

freeze_bdev() on sb frozen by ioctl_fsfreeze() -> EBUSY freeze_bdev() on sb frozen by freeze_bdev() -> success ioctl_fsfreeze() on sb frozen by freeze_bdev() -> EBUSY ioctl_fsfreeze() on sb frozen by ioctl_fsfreeze() -> EBUSY

Quoted 4 lines

Quoted 1 line

thaw_bdev() on sb frozen by ioctl_fsfreeze() -> EINVAL

Phew, so this is what we want for the in-kernel freezing so we're good and *can* combine these then.

I double checked, and I don't see where you get EINVAL for this case. We *do* keep the sb frozen though, which is good, and the worst fear I had was that we did not. However we return 0 if there was already a prior freeze_bdev() or ioctl_fsfreeze() other than the context that started the prior freeze (--bdev->bd_fsfreeze_count > 0).

The -EINVAL is only returned currently if there were no freezers.

int thaw_bdev(struct block_device *bdev, struct super_block *sb)

{
	int error = -EINVAL;

	mutex_lock(&bdev->bd_fsfreeze_mutex);
	if (!bdev->bd_fsfreeze_count)
		goto out;

Quoted 44 lines · Luis R. Rodriguez

Quoted 26 lines · Luis R. Rodriguez

Quoted 3 lines · Jan Kara

Hello,

I think I owe you a reply here... Sorry that it took so long.

Took me just as long :)

Quoted 19 lines · Luis R. Rodriguez

Quoted 8 lines

I'll note that its still not perfectly clear if really the semantics behind freeze_bdev() match what I described above fully. That still needs to be vetted for. For instance, does thaw_bdev() keep a superblock frozen if we an ioctl initiated freeze had occurred before? If so then great. Otherwise I think we'll need to distinguish the ioctl interface. Worst possible case is that bdev semantics and in-kernel semantics differ somehow, then that will really create a holy fucking mess.

I believe nobody really thought about mixing those two interfaces to fs freezing and so the behavior is basically defined by the implementation. That is:

freeze_bdev() on sb frozen by ioctl_fsfreeze() -> EBUSY freeze_bdev() on sb frozen by freeze_bdev() -> success ioctl_fsfreeze() on sb frozen by freeze_bdev() -> EBUSY ioctl_fsfreeze() on sb frozen by ioctl_fsfreeze() -> EBUSY

Quoted 4 lines

Quoted 1 line

thaw_bdev() on sb frozen by ioctl_fsfreeze() -> EINVAL

Phew, so this is what we want for the in-kernel freezing so we're good and *can* combine these then.

I double checked, and I don't see where you get EINVAL for this case. We *do* keep the sb frozen though, which is good, and the worst fear I had was that we did not. However we return 0 if there was already a prior freeze_bdev() or ioctl_fsfreeze() other than the context that started the prior freeze (--bdev->bd_fsfreeze_count > 0).

The -EINVAL is only returned currently if there were no freezers.

int thaw_bdev(struct block_device *bdev, struct super_block *sb)

{
	int error = -EINVAL;

Quoted 3 lines

	mutex_lock(&bdev->bd_fsfreeze_mutex);
	if (!bdev->bd_fsfreeze_count)
		goto out;

But this is precisely where we'd bail if we freeze sb by ioctl_fsfreeze() but try to thaw by thaw_bdev(). ioctl_fsfreeze() does not touch bd_fsfreeze_count...

Quoted 10 lines · Luis R. Rodriguez

The question of whether or not a superblock is frozen needs to be augmented in the future to account for differences between a user initiated freeze and a kernel initiated freeze done automatically on behalf of the kernel.

Provide helpers so that these can be used instead so that we don't have to expand checks later in these same call sites as we expand the definition of a frozen superblock.

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

So helpers are fine but...

include/linux/fs.h static inline void sb_start_intwrite(struct super_block *sb) · line 1602

Quoted 11 lines

include/linux/fs.h +11 -0

+/**
+ * sb_is_frozen_by_user - is superblock frozen by a user call
+ * @sb: the super to check
+ *
+ * Returns true if the super freeze was initiated by userspace, for instance,
+ * an ioctl call.
+ */
+static inline bool sb_is_frozen_by_user(struct super_block *sb)
+{
+	return sb->s_writers.frozen == SB_FREEZE_COMPLETE;
+}

... I dislike the _by_user() suffix as there may be different places that call freeze_super() (e.g. device mapper does this during some operations). Clearly we need to distinguish "by system suspend" and "the other" cases. So please make this clear in the naming.

In fact, what might be a cleaner solution is to introduce a 'freeze_count' for superblock freezing (we already do have this for block devices). Then you don't need to differentiate these two cases - but you'd still need to properly handle cleanup if freezing of all superblocks fails in the middle. So I'm not 100% this works out nicely in the end. But it's certainly worth a consideration.

There are use cases where we wish to traverse the superblock list but also capture errors, and in which case we want to avoid having our callers issue a lock themselves since we can do the locking for the callers. Provide a iterate_supers_excl() which calls a function with the write lock held. If an error occurs we capture it and propagate it.

Likewise there are use cases where we wish to traverse the superblock list but in reverse order. The new iterate_supers_reverse_excl() helpers does this but also also captures any errors encountered.

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

---
 fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 93 insertions(+)

fs/super.c +10 -0

index a63513d187e8..885711c1d35b 100644
@@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
        spin_unlock(&sb_lock);
 }
 
+/**
+ *     iterate_supers_excl - exclusively call func for all active superblocks
+ *     @f: function to call
+ *     @arg: argument to pass to it
+ *
+ *     Scans the superblock list and calls given function, passing it
+ *     locked superblock and given argument. Returns 0 unless an error
+ *     occurred on calling the function on any superblock.
+ */
+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)

fs/super.c +25 -0

+
+               down_write(&sb->s_umount);
+               if (sb->s_root && (sb->s_flags & SB_BORN)) {
+                       error = f(sb, arg);
+                       if (error) {
+                               up_write(&sb->s_umount);
+                               spin_lock(&sb_lock);
+                               __put_super(sb);
+                               break;
+                       }
+               }
+               up_write(&sb->s_umount);
+
+               spin_lock(&sb_lock);
+               if (p)
+                       __put_super(p);
+               p = sb;
+       }
+       if (p)
+               __put_super(p);
+       spin_unlock(&sb_lock);
+
+       return error;
+}
+

fs/super.c void iterate_supers(void (*f)(struct super_block *, void *), void *arg) · line 627

Quoted 11 lines · Luis R. Rodriguez

fs/super.c +11 -0

+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
+{
+       struct super_block *sb, *p = NULL;
+       int error = 0;
+
+       spin_lock(&sb_lock);
+       list_for_each_entry(sb, &super_blocks, s_list) {
+               if (hlist_unhashed(&sb->s_instances))
+                       continue;
+               sb->s_count++;
+               spin_unlock(&sb_lock);

Can anything bad happen if the list is modified at this point by a concurrent thread?

Quoted 40 lines · Luis R. Rodriguez

There are use cases where we wish to traverse the superblock list but also capture errors, and in which case we want to avoid having our callers issue a lock themselves since we can do the locking for the callers. Provide a iterate_supers_excl() which calls a function with the write lock held. If an error occurs we capture it and propagate it.

Likewise there are use cases where we wish to traverse the superblock list but in reverse order. The new iterate_supers_reverse_excl() helpers does this but also also captures any errors encountered.

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

---
 fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 93 insertions(+)

fs/super.c +15 -0

index a63513d187e8..885711c1d35b 100644
@@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
        spin_unlock(&sb_lock);
 }
 
+/**
+ *     iterate_supers_excl - exclusively call func for all active superblocks
+ *     @f: function to call
+ *     @arg: argument to pass to it
+ *
+ *     Scans the superblock list and calls given function, passing it
+ *     locked superblock and given argument. Returns 0 unless an error
+ *     occurred on calling the function on any superblock.
+ */
+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
+{
+       struct super_block *sb, *p = NULL;
+       int error = 0;
+
+       spin_lock(&sb_lock);

fs/super.c void iterate_supers(void (*f)(struct super_block *, void *), void *arg) · line 627

Quoted 5 lines

fs/super.c +5 -0

+       list_for_each_entry(sb, &super_blocks, s_list) {
+               if (hlist_unhashed(&sb->s_instances))
+                       continue;
+               sb->s_count++;
+               spin_unlock(&sb_lock);

Can anything bad happen if the list is modified at this point by a concurrent thread?

Quoted 9 lines

fs/super.c +9 -0

+
+               down_write(&sb->s_umount);
+               if (sb->s_root && (sb->s_flags & SB_BORN)) {
+                       error = f(sb, arg);
+                       if (error) {
+                               up_write(&sb->s_umount);
+                               spin_lock(&sb_lock);
+                               __put_super(sb);
+                               break;

fs/super.c void iterate_supers(void (*f)(struct super_block *, void *), void *arg) · line 644

Quoted 8 lines

fs/super.c +8 -0

+                       }
+               }
+               up_write(&sb->s_umount);
+
+               spin_lock(&sb_lock);
+               if (p)
+                       __put_super(p);
+               p = sb;

Quoted 41 lines · Rafael J. Wysocki

Quoted 40 lines · Luis R. Rodriguez

There are use cases where we wish to traverse the superblock list but also capture errors, and in which case we want to avoid having our callers issue a lock themselves since we can do the locking for the callers. Provide a iterate_supers_excl() which calls a function with the write lock held. If an error occurs we capture it and propagate it.

Likewise there are use cases where we wish to traverse the superblock list but in reverse order. The new iterate_supers_reverse_excl() helpers does this but also also captures any errors encountered.

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

---
 fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 93 insertions(+)

fs/super.c +15 -0

index a63513d187e8..885711c1d35b 100644
@@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
        spin_unlock(&sb_lock);
 }
 
+/**
+ *     iterate_supers_excl - exclusively call func for all active superblocks
+ *     @f: function to call
+ *     @arg: argument to pass to it
+ *
+ *     Scans the superblock list and calls given function, passing it
+ *     locked superblock and given argument. Returns 0 unless an error
+ *     occurred on calling the function on any superblock.
+ */
+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
+{
+       struct super_block *sb, *p = NULL;
+       int error = 0;
+
+       spin_lock(&sb_lock);

Quoted 8 lines

fs/super.c void iterate_supers(void (*f)(struct super_block *, void *), void *arg) · line 627

Quoted 5 lines

fs/super.c +5 -0

+       list_for_each_entry(sb, &super_blocks, s_list) {
+               if (hlist_unhashed(&sb->s_instances))
+                       continue;
+               sb->s_count++;
+               spin_unlock(&sb_lock);

Can anything bad happen if the list is modified at this point by a concurrent thread?

No. We have a valid reference to sb->s_count and that keeps it on the list while we have the lock dropped. The sb reference isn't dropped until we've iterated to the next sb on the list and taken a reference to that, hence it's safe to drop and regain the list lock without needing to restart the iteration.

Quoted 9 lines

Quoted 9 lines

fs/super.c +9 -0

+
+               down_write(&sb->s_umount);
+               if (sb->s_root && (sb->s_flags & SB_BORN)) {
+                       error = f(sb, arg);
+                       if (error) {
+                               up_write(&sb->s_umount);
+                               spin_lock(&sb_lock);
+                               __put_super(sb);
+                               break;

Quoted 8 lines

fs/super.c void iterate_supers(void (*f)(struct super_block *, void *), void *arg) · line 644

Quoted 8 lines

fs/super.c +8 -0

+                       }
+               }
+               up_write(&sb->s_umount);
+
+               spin_lock(&sb_lock);
+               if (p)
+                       __put_super(p);
+               p = sb;

This code here is what drops the reference to the previous sb we've iterated past.

FWIW, this "hold until next is held" iteration pattern is used frequently for inodes, dentries, and other reference counted VFS objects so we can iterate the list without needing to hold the list lock for the entire iteration....

There are use cases where we wish to traverse the superblock list but also capture errors, and in which case we want to avoid having our callers issue a lock themselves since we can do the locking for the callers. Provide a iterate_supers_excl() which calls a function with the write lock held. If an error occurs we capture it and propagate it.

Likewise there are use cases where we wish to traverse the superblock list but in reverse order. The new iterate_supers_reverse_excl() helpers does this but also also captures any errors encountered.

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

---
 fs/super.c         | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h |  2 ++
 2 files changed, 93 insertions(+)

fs/super.c +91 -0

index a63513d187e8..885711c1d35b 100644
@@ -605,6 +605,97 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 	spin_unlock(&sb_lock);
 }
 
+/**
+ *	iterate_supers_excl - exclusively call func for all active superblocks
+ *	@f: function to call
+ *	@arg: argument to pass to it
+ *
+ *	Scans the superblock list and calls given function, passing it
+ *	locked superblock and given argument. Returns 0 unless an error
+ *	occurred on calling the function on any superblock.
+ */
+int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		down_write(&sb->s_umount);
+		if (sb->s_root && (sb->s_flags & SB_BORN)) {
+			error = f(sb, arg);
+			if (error) {
+				up_write(&sb->s_umount);
+				spin_lock(&sb_lock);
+				__put_super(sb);
+				break;
+			}
+		}
+		up_write(&sb->s_umount);
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	return error;
+}
+
+/**
+ *	iterate_supers_reverse_excl - exclusively calls func in reverse order
+ *	@f: function to call
+ *	@arg: argument to pass to it
+ *
+ *	Scans the superblock list and calls given function, passing it
+ *	locked superblock and given argument, in reverse order, and holding
+ *	the s_umount write lock. Returns if an error occurred.
+ */
+int iterate_supers_reverse_excl(int (*f)(struct super_block *, void *),
+					 void *arg)
+{
+	struct super_block *sb, *p = NULL;
+	int error = 0;
+
+	spin_lock(&sb_lock);
+	list_for_each_entry_reverse(sb, &super_blocks, s_list) {
+		if (hlist_unhashed(&sb->s_instances))
+			continue;
+		sb->s_count++;
+		spin_unlock(&sb_lock);
+
+		down_write(&sb->s_umount);
+		if (sb->s_root && (sb->s_flags & SB_BORN)) {
+			error = f(sb, arg);
+			if (error) {
+				up_write(&sb->s_umount);
+				spin_lock(&sb_lock);
+				__put_super(sb);
+				break;
+			}
+		}
+		up_write(&sb->s_umount);
+
+		spin_lock(&sb_lock);
+		if (p)
+			__put_super(p);
+		p = sb;
+	}
+	if (p)
+		__put_super(p);
+	spin_unlock(&sb_lock);
+
+	return error;
+}
+
 /**
  *	iterate_supers_type - call function for superblocks of given type
  *	@type: fs type

include/linux/fs.h +2 -0

index 107725b20fad..fe90b6542697 100644
@@ -3164,6 +3164,8 @@ extern struct super_block *get_active_super(struct block_device *bdev);
 extern void drop_super(struct super_block *sb);
 extern void drop_super_exclusive(struct super_block *sb);
 extern void iterate_supers(void (*)(struct super_block *, void *), void *);
+extern int iterate_supers_excl(int (*f)(struct super_block *, void *), void *arg);
+extern int iterate_supers_reverse_excl(int (*)(struct super_block *, void *), void *);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);
 

Signature · 1 line

2.15.0

This removes superflous freezer calls as they are no longer needed as the VFS now performs filesystem freezing/thaw if the filesystem has support for it.

The following Coccinelle rule was used as follows:

spatch --sp-file fs-freeze-cleanup.cocci --in-place fs/$FS/

@ has_freeze_fs @
identifier super_ops;
expression freeze_op;
@@

struct super_operations super_ops = {
.freeze_fs = freeze_op,
};

@ remove_set_freezable depends on has_freeze_fs @
expression time;
statement S, S2, S3;
expression task;
@@

(

fs/xfs/xfs_trans_ail.c +0 -1

-	set_freezable();

|

(quoted diff) +0 -2

-	if (try_to_freeze())
-		continue;

|

fs/xfs/xfs_trans_ail.c +0 -1

-	try_to_freeze();

|

fs/xfs/xfs_trans_ail.c +1 -1

-	freezable_schedule();
+	schedule();

|

(quoted diff) +1 -1

-	freezable_schedule_timeout(time);
+	schedule_timeout(time);

|

(quoted diff) +0 -1

-	if (freezing(task)) { S }

|

(quoted diff) +0 -2

-	if (freezing(task)) { S }
-	else

	{ S2 }

|

(quoted diff) +0 -1

-	freezing(current)

)

Generated-by Coccinelle SmPL

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

---
 fs/xfs/xfs_trans_ail.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

fs/xfs/xfs_trans_ail.c +2 -5

index cef89f7127d3..1f3dd10a9d00 100644
@@ -513,7 +513,6 @@ xfsaild(
 	long		tout = 0;	/* milliseconds */
 
 	current->flags |= PF_MEMALLOC;
-	set_freezable();
 
 	while (1) {
 		if (tout && tout <= 20)

@@ -551,19 +550,17 @@ xfsaild(
 		if (!xfs_ail_min(ailp) &&
 		    ailp->xa_target == ailp->xa_target_prev) {
 			spin_unlock(&ailp->xa_lock);
-			freezable_schedule();
+			schedule();
 			tout = 0;
 			continue;
 		}
 		spin_unlock(&ailp->xa_lock);
 
 		if (tout)
-			freezable_schedule_timeout(msecs_to_jiffies(tout));
+			schedule_timeout(msecs_to_jiffies(tout));
 
 		__set_current_state(TASK_RUNNING);
 
-		try_to_freeze();
-
 		tout = xfsaild_push(ailp);
 	}
 

Signature · 1 line

2.15.0

Quoted 7 lines · Luis R. Rodriguez

This removes superflous freezer calls as they are no longer needed as the VFS now performs filesystem freezing/thaw if the filesystem has support for it.

The following Coccinelle rule was used as follows:

spatch --sp-file fs-freeze-cleanup.cocci --in-place fs/$FS/

I think your rule misses WQ_FREEZABLE flag for workqueues? That would be also good to get rid of...

Quoted 5 lines · Jan Kara

Quoted 4 lines · Luis R. Rodriguez

This removes superflous freezer calls as they are no longer needed as the VFS now performs filesystem freezing/thaw if the filesystem has support for it.

Quoted 6 lines

Quoted 3 lines

The following Coccinelle rule was used as follows:

spatch --sp-file fs-freeze-cleanup.cocci --in-place fs/$FS/

I think your rule misses WQ_FREEZABLE flag for workqueues? That would be also good to get rid of...

We need that one (or equivalent) for the runtime PM workqueue at least.

Quoted 10 lines · Rafael J. Wysocki

Quoted 9 lines · Jan Kara

Quoted 7 lines · Luis R. Rodriguez

This removes superflous freezer calls as they are no longer needed as the VFS now performs filesystem freezing/thaw if the filesystem has support for it.

The following Coccinelle rule was used as follows:

spatch --sp-file fs-freeze-cleanup.cocci --in-place fs/$FS/

Quoted 4 lines

Quoted 2 lines

I think your rule misses WQ_FREEZABLE flag for workqueues? That would be also good to get rid of...

We need that one (or equivalent) for the runtime PM workqueue at least.

I think Jan was talking about all the WQ_FREEZABLE flags in filesystem workqueues (such as all the XFS wqs) that are no longer necessary once filesystems are frozen appropriately.

This is a followup from the original RFC which proposed to start to kill kthread freezing all together [0]. Instead of going straight out to the jugular for kthread freezing this series only addresses killing freezer calls on filesystems which implement freeze_fs, after we let the kernel freeze these filesystems for us on suspend.

o xfs
o reiserfs
o nilfs2
o jfs
o f2fs
o ext4
o ext2
o btrfs

BTW, is nfs able to be included in this set? I also encountered a freeze() failure due to nfs access during that stage recently.

Quoted 2 lines · Yu Chen

BTW, is nfs able to be included in this set? I also encountered a freeze() failure due to nfs access during that stage recently.

The freezer usage in NFS is magnitudes more complicated, so it makes sense to first go after the lower hanging fruit to figure out the viability of the whole aproach in practice.

Quoted 2 lines · Yu Chen

BTW, is nfs able to be included in this set? I also encountered a freeze() failure due to nfs access during that stage recently.

The freezer usage in NFS is magnitudes more complicated, so it makes sense to first go after the lower hanging fruit to figure out the viability of the whole aproach in practice.

Quoted 9 lines · Jiri Kosina

Quoted 2 lines · Yu Chen

BTW, is nfs able to be included in this set? I also encountered a freeze() failure due to nfs access during that stage recently.

The freezer usage in NFS is magnitudes more complicated, so it makes sense to first go after the lower hanging fruit to figure out the viability of the whole aproach in practice.

Agreed that we should do this in stages. It doesn't help that freezer handling in the client is a bit of a mess at this point...

At a high level for NFS, I think we need to have freeze_fs make the RPC engine "park" newly issued RPCs for that fs' client onto a rpc_wait_queue. Any RPC that has already been sent however, we need to wait for a reply.

Once everything is quiesced we can return and call it frozen. unfreeze_fs can then just have the engine stop parking RPCs and wake up the waitq.

That should be enough to make suspend and resume work more reliably. If, however, you're interested in making the cgroup freezer also work, then we may need to do a bit more work to ensure that we don't end up with frozen tasks squatting on VFS locks.

This is a followup from the original RFC which proposed to start to kill kthread freezing all together [0]. Instead of going straight out to the jugular for kthread freezing this series only addresses killing freezer calls on filesystems which implement freeze_fs, after we let the kernel freeze these filesystems for us on suspend.

This approach puts on a slow but steady path towards the original goal though. Each subsystem could look for similar solutions. Even with filesystems we're not all done yet, after this we'll still have to decide what to do about filesystems which do not implement freeze_fs().

Motivation and problem:

kthreads have some semantics for freezing, which helps the kernel freeze them when a system is going to suspend or hibernation. These semantics are not well defined though, and it actually turns out pretty hard to get it right.

Without a proper solution suspend and hibernation are fragile on filesystems, it can easily break suspend and fixing such issues are in no way trivial [1] [2].

Proposed solution:

Instead of fixing such semantics and trying to get all filesystems to do it right, we can easily do away with all freezing calls if the filesystem implements a proper freeze_fs() callback. The following 9 filesystems have freeze_fs() implemented as such we can let the kernel issue the callback upon suspend and thaw on resume automatically on our behalf.

o xfs
o reiserfs
o nilfs2
o jfs
o f2fs
o ext4
o ext2
o btrfs

Of these, the following have freezer helpers, which can then be removed after the kernel automaticaly calls freeze_fs for us on suspend:

o xfs
o nilfs2
o jfs
o f2fs
o ext4

I've tested this on a system with ext4 and XFS, and have let 0-day go at without issues. I have branches availabe for linux-next [3] and Linus' latest tree [4].

Further testing, thoughts, reviews, flames are all equally appreciated.

It's great to see that you are making progress with this work :-) However, what's not clear to me is what your (long-term) plan is for freezing filesystems that e.g. exist on top of a md RAID1 block device?

The md resync
thread must be stopped before a system is frozen. Today the md driver uses the kthread freezing mechanism for that purpose. Do you have a plan for handling the more complicated scenarios, e.g. a filesystem that exists on top of an md device where the md device uses one or more files as backing store and with the loop driver between the md device and the files?

How about filesystems implemented in user space using the FUSE driver?

Patch 6/11 of this series freezes user space processes before freezing filesystems. Will that work for FUSE filesystems?

Quoted 14 lines · Luis R. Rodriguez

Quoted 6 lines · Bart Van Assche

The md resync
thread must be stopped before a system is frozen. Today the md driver uses the kthread freezing mechanism for that purpose. Do you have a plan for handling the more complicated scenarios, e.g. a filesystem that exists on top of an md device where the md device uses one or more files as backing store and with the loop driver between the md device and the files?

Nope not yet. It seems you have given this some thought though so you're help here is greatly appreciated. In fact the way we should see the long term 'kill kthread freezing' effort should be a collaborative one. I've never touched md, so folks more familiar with md should give this some thought.

Quoted 4 lines

Can for instance md register_pm_notifier() and register_syscore_ops() and do handy work to pause some work to replace kthread freezing? Note that I believe a pm notifier is needed in case syscore_suspend() is not called, say on a suspend fail.

Sorry but I don't think that a solution can be based on a notifier mechanism. Freezing has to happen in the order in which drivers and filesystems have been stacked (filesystem > md device > filesystem for the above example).

Proposed solution:

Instead of fixing such semantics and trying to get all filesystems to do it right, we can easily do away with all freezing calls if the filesystem implements a proper freeze_fs() callback. The following 9 filesystems have freeze_fs() implemented as such we can let the kernel issue the callback upon suspend and thaw on resume automatically on our behalf.

o xfs
o reiserfs
o nilfs2
o jfs
o f2fs
o ext4
o ext2
o btrfs

Of these, the following have freezer helpers, which can then be removed after the kernel automaticaly calls freeze_fs for us on suspend:

o xfs
o nilfs2
o jfs
o f2fs
o ext4

I've tested this on a system with ext4 and XFS, and have let 0-day go at without issues. I have branches availabe for linux-next [3] and Linus' latest tree [4].

Hi!

Quoted 26 lines

Proposed solution:

Instead of fixing such semantics and trying to get all filesystems to do it right, we can easily do away with all freezing calls if the filesystem implements a proper freeze_fs() callback. The following 9 filesystems have freeze_fs() implemented as such we can let the kernel issue the callback upon suspend and thaw on resume automatically on our behalf.

o xfs
o reiserfs
o nilfs2
o jfs
o f2fs
o ext4
o ext2
o btrfs

Of these, the following have freezer helpers, which can then be removed after the kernel automaticaly calls freeze_fs for us on suspend:

o xfs
o nilfs2
o jfs
o f2fs
o ext4

Quoted 3 lines

I've tested this on a system with ext4 and XFS, and have let 0-day go at without issues. I have branches availabe for linux-next [3] and Linus' latest tree [4].

Was hibernation tested? uswsusp?

This is a followup from the original RFC which proposed to start to kill kthread freezing all together [0]. Instead of going straight out to the jugular for kthread freezing this series only addresses killing freezer calls on filesystems which implement freeze_fs, after we let the kernel freeze these filesystems for us on suspend.

This approach puts on a slow but steady path towards the original goal though. Each subsystem could look for similar solutions. Even with filesystems we're not all done yet, after this we'll still have to decide what to do about filesystems which do not implement freeze_fs().

Motivation and problem:

kthreads have some semantics for freezing, which helps the kernel freeze them when a system is going to suspend or hibernation. These semantics are not well defined though, and it actually turns out pretty hard to get it right.

Without a proper solution suspend and hibernation are fragile on filesystems, it can easily break suspend and fixing such issues are in no way trivial [1] [2].

Proposed solution:

Instead of fixing such semantics and trying to get all filesystems to do it right, we can easily do away with all freezing calls if the filesystem implements a proper freeze_fs() callback. The following 9 filesystems have freeze_fs() implemented as such we can let the kernel issue the callback upon suspend and thaw on resume automatically on our behalf.

o xfs
o reiserfs
o nilfs2
o jfs
o f2fs
o ext4
o ext2
o btrfs

Of these, the following have freezer helpers, which can then be removed after the kernel automaticaly calls freeze_fs for us on suspend:

o xfs
o nilfs2
o jfs
o f2fs
o ext4

I've tested this on a system with ext4 and XFS, and have let 0-day go at without issues. I have branches availabe for linux-next [3] and Linus' latest tree [4].

Further testing, thoughts, reviews, flames are all equally appreciated.

[0] https://lkml.kernel.org/r/[email protected]
[1] https://bugzilla.suse.com/show_bug.cgi?id=1043449
[2] https://lkml.kernel.org/r/[email protected]
[3] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20171129-fs-freeze-cleanup
[4] https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20171129-fs-freeze-cleanup

Luis R. Rodriguez (11):
fs: provide unlocked helper for freeze_super()
fs: provide unlocked helper thaw_super()
fs: add frozen sb state helpers
fs: distinguish between user initiated freeze and kernel initiated
freeze
fs: add iterate_supers_excl() and iterate_supers_reverse_excl()
fs: freeze on suspend and thaw on resume
xfs: remove not needed freezing calls
ext4: remove not needed freezing calls
f2fs: remove not needed freezing calls
nilfs2: remove not needed freezing calls
jfs: remove not needed freezing calls

 fs/ext4/ext4_jbd2.c    |   2 +-
 fs/ext4/super.c        |   2 -
 fs/f2fs/gc.c           |   5 +-
 fs/f2fs/segment.c      |   6 +-
 fs/jfs/jfs_logmgr.c    |  11 +-
 fs/jfs/jfs_txnmgr.c    |  31 ++---
 fs/nilfs2/segment.c    |  48 ++++----
 fs/super.c             | 320 ++++++++++++++++++++++++++++++++++++++++---------
 fs/xfs/xfs_trans.c     |   2 +-
 fs/xfs/xfs_trans_ail.c |   7 +-
 include/linux/fs.h     |  63 +++++++++-
 kernel/power/process.c |  15 ++-
 12 files changed, 378 insertions(+), 134 deletions(-)

I'm assuming an update of this to be posted due to the comments from Jan on patch [3/11] and patch [7/11] probably.

Is there anything else that needs to be addressed?