[RFC 0/3] Shut down frozen filesystems on last unmount

[RFC 0/3] Shut down frozen filesystems on last unmount

[Andreas Gruenbacher]

Hello,

currently, when a frozen filesystem is unmouted, it turns into a zombie
rather than being shut down; it can only be shut down after remounting
and thawing it.  That's silly for local filesystems, but it's worse for
filesystems like gfs2 which freeze the filesystem on all nodes when
fsfreeze is called on any of the nodes: there, the nodes that didn't
initiate the freeze cannot shut down the filesystem at all.

This is a non-working, first shot at allowing filesystems to shut down
on the last unmount.  Could you please have a look to let me know if
something like this makes sense?

The three patches in this series can be found at the tail of this tree:

https://git.kernel.org/pub/scm/linux/kernel/git/gfs2/linux-gfs2.git/log/?h=freeze%2bumount

The vfs patches apply directly on top of v6.1-rc5 -ish kernels.

The gfs2 patch depends on previous patches in the above tree, so please
grab that if you want the full context.

Thanks a lot,
Andreas

Andreas Gruenbacher (3):
  fs: Add activate_super function
  fs: Introduce { freeze, thaw }_active_super functions
  gfs2: Shut down frozen filesystem on last unmount

 fs/gfs2/glops.c    | 17 ++-------
 fs/gfs2/super.c    | 27 ++++++++++----
 fs/super.c         | 89 +++++++++++++++++++++++++++++++++++++++++-----
 include/linux/fs.h |  3 ++
 4 files changed, 108 insertions(+), 28 deletions(-)

-- 
2.38.1

[PATCH 1/3] fs: Add activate_super function

[Andreas Gruenbacher]

Add function activate_super() for grabbing an active reference on a
super block.  Fails if the filesystem is already shutting down.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/super.c         | 19 +++++++++++++++++++
 include/linux/fs.h |  1 +
 2 files changed, 20 insertions(+)

diff --git a/fs/super.c b/fs/super.c
index 8d39e4f11cfa..051241cf408b 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -393,6 +393,25 @@ static int grab_super(struct super_block *s) __releases(sb_lock)
 	return 0;
 }
 
+/**
+ * activate_super - try to grab an active reference on the superblock
+ * @sb: reference we are trying to grab
+ *
+ * Try to grab an active reference on the superblock to prevent filesystem
+ * shutdown.  Fails if the filesystem is already shutting down (see
+ * deactivate_locked_super()).
+ */
+bool activate_super(struct super_block *sb)
+{
+	if (atomic_inc_not_zero(&sb->s_active)) {
+		smp_mb__after_atomic();
+		BUG_ON(!(sb->s_flags & SB_BORN));
+		return true;
+	}
+	return false;
+}
+EXPORT_SYMBOL(activate_super);
+
 /*
  *	trylock_super - try to grab ->s_umount shared
  *	@sb: reference we are trying to grab
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e654435f1651..84c609123a25 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2573,6 +2573,7 @@ void generic_shutdown_super(struct super_block *sb);
 void kill_block_super(struct super_block *sb);
 void kill_anon_super(struct super_block *sb);
 void kill_litter_super(struct super_block *sb);
+bool activate_super(struct super_block *sb);
 void deactivate_super(struct super_block *sb);
 void deactivate_locked_super(struct super_block *sb);
 int set_anon_super(struct super_block *s, void *data);
-- 
2.38.1

[PATCH 2/3] fs: Introduce { freeze, thaw }_active_super functions

[Andreas Gruenbacher]

Introduce functions freeze_active_super() and thaw_active_super(), which
are like freeze_super() and thaw_super() but don't keep an active super
block reference between freeze_super() and the following thaw_super().
This allows filesystem shutdown to occur while a filesystem is frozen.

In places in the filesystem code where a super block may or may not be
active anymore (i.e., places that may race with ->put_super()), function
activate_super() can be used for grabbing an active super block
reference.  In that case,

freeze_super(sb) turns into:

	if (activate_super(sb)) {
		ret = freeze_active_super(sb);
		deactivate_super(sb);
	}

and thaw_super(sb) turns into:

	if (activate_super(sb)) {
		ret = thaw_active_super(sb);
		deactivate_super(sb);
	}

For obvious reaons, the filesystem is responsible for making sure that
no such asynchronous code outlives put_super().

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/super.c         | 70 ++++++++++++++++++++++++++++++++++++++++------
 include/linux/fs.h |  2 ++
 2 files changed, 64 insertions(+), 8 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 051241cf408b..cba55ca89c09 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -39,6 +39,7 @@
 #include <uapi/linux/mount.h>
 #include "internal.h"
 
+static int thaw_active_super_locked(struct super_block *sb);
 static int thaw_super_locked(struct super_block *sb);
 
 static LIST_HEAD(super_blocks);
@@ -510,6 +511,8 @@ void generic_shutdown_super(struct super_block *sb)
 		if (sop->put_super)
 			sop->put_super(sb);
 
+		thaw_active_super_locked(sb);
+
 		if (!list_empty(&sb->s_inodes)) {
 			printk("VFS: Busy inodes after unmount of %s. "
 			   "Self-destruct in 5 seconds.  Have a nice day...\n",
@@ -1676,7 +1679,7 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
 }
 
 /**
- * freeze_super - lock the filesystem and force it into a consistent state
+ * freeze_active_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
@@ -1708,11 +1711,10 @@ static void sb_freeze_unlock(struct super_block *sb, int level)
  *
  * sb->s_writers.frozen is protected by sb->s_umount.
  */
-int freeze_super(struct super_block *sb)
+int freeze_active_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);
@@ -1776,16 +1778,38 @@ int freeze_super(struct super_block *sb)
 	up_write(&sb->s_umount);
 	return 0;
 }
+EXPORT_SYMBOL(freeze_active_super);
+
+/**
+ * freeze_super - lock the filesystem and force it into a consistent state
+ * @sb: the super to lock
+ *
+ * Like freeze_active_super(), but takes an active reference on @sb so
+ * that it cannot be shut down while frozen.
+ */
+int freeze_super(struct super_block *sb)
+{
+	atomic_inc(&sb->s_active);
+	return freeze_active_super(sb);
+}
 EXPORT_SYMBOL(freeze_super);
 
-static int thaw_super_locked(struct super_block *sb)
+/**
+ * thaw_active_super_locked -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ *
+ * Like thaw_super(), but takes a locked super block, leaves it locked, and
+ * doesn't drop an active reference.  For use in ->put_super by filesystems
+ * that use freeze_active_super() and thaw_active_super().
+ */
+static int thaw_active_super_locked(struct super_block *sb)
 {
 	int error;
 
-	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;
@@ -1800,7 +1824,6 @@ static int thaw_super_locked(struct super_block *sb)
 			printk(KERN_ERR
 				"VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			up_write(&sb->s_umount);
 			return error;
 		}
 	}
@@ -1809,6 +1832,18 @@ static int thaw_super_locked(struct super_block *sb)
 	sb_freeze_unlock(sb, SB_FREEZE_FS);
 out:
 	wake_up(&sb->s_writers.wait_unfrozen);
+	return 0;
+}
+
+static int thaw_super_locked(struct super_block *sb)
+{
+	int ret;
+
+	ret = thaw_active_super_locked(sb);
+	if (ret) {
+		up_write(&sb->s_umount);
+		return ret;
+	}
 	deactivate_locked_super(sb);
 	return 0;
 }
@@ -1825,3 +1860,22 @@ int thaw_super(struct super_block *sb)
 	return thaw_super_locked(sb);
 }
 EXPORT_SYMBOL(thaw_super);
+
+/**
+ * thaw_active_super -- unlock filesystem
+ * @sb: the super to thaw
+ *
+ * Unlocks the filesystem and marks it writeable again after freeze_super().
+ *
+ * Like thaw_super(), but doesn't drop an active reference.
+ */
+int thaw_active_super(struct super_block *sb)
+{
+	int ret;
+
+	down_write(&sb->s_umount);
+	ret = thaw_active_super_locked(sb);
+	up_write(&sb->s_umount);
+	return ret;
+}
+EXPORT_SYMBOL(thaw_active_super);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 84c609123a25..fe869102cd85 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2612,6 +2612,8 @@ extern int user_statfs(const char __user *, struct kstatfs *);
 extern int fd_statfs(int, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern int freeze_active_super(struct super_block *super);
+extern int thaw_active_super(struct super_block *super);
 extern __printf(2, 3)
 int super_setup_bdi_name(struct super_block *sb, char *fmt, ...);
 extern int super_setup_bdi(struct super_block *sb);
-- 
2.38.1

Re: [PATCH 2/3] fs: Introduce { freeze, thaw }_active_super functions

[Al Viro]

On Wed, Nov 30, 2022 at 12:07:34AM +0100, Andreas Gruenbacher wrote:

> -int freeze_super(struct super_block *sb)
> +int freeze_active_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);

Not fond of the calling conventions, to be honest...  "On success return 0;
on failure return -E... *and* drop an active reference passed by the caller"?

If you go that way, at least take the deactivate_locked_super() into the
caller and I would argue that ->s_umount handling also belongs in the caller,
both grabbing and dropping it.

[PATCH 3/3] gfs2: Shut down frozen filesystem on last unmount

[Andreas Gruenbacher]

So far, when a frozen filesystem is last unmouted, it turns into a
zombie rather than being shut down; to shut it down, it needs to be
remounted and thawed first.

That's silly for local filesystems, but it's worse for filesystems like
gfs2 which freeze a filesystem cluster-wide when fsfreeze is called on
one of the nodes.  Only the node that initiated a freeze is allowed to
thaw the filesystem it again.  On the other nodes, the only way to shut
down the remotely frozen filesystem is to power off.

Change that on gfs2 so that frozen filesystems are immediately shut down
when they are last unmounted and removed from the filesystem namespace.
This doesn't require writing to the filesystem, so the remaining cluster
nodes remain undisturbed.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 fs/gfs2/glops.c | 17 +++--------------
 fs/gfs2/super.c | 27 +++++++++++++++++++++------
 2 files changed, 24 insertions(+), 20 deletions(-)

diff --git a/fs/gfs2/glops.c b/fs/gfs2/glops.c
index ceadb2d2d948..d73d1944543f 100644
--- a/fs/gfs2/glops.c
+++ b/fs/gfs2/glops.c
@@ -555,25 +555,13 @@ static void inode_go_dump(struct seq_file *seq, struct gfs2_glock *gl,
 static void freeze_go_callback(struct gfs2_glock *gl, bool remote)
 {
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
-	struct super_block *sb = sdp->sd_vfs;
 
 	if (!remote ||
 	    gl->gl_state != LM_ST_SHARED ||
 	    gl->gl_demote_state != LM_ST_UNLOCKED)
 		return;
 
-	/*
-	 * Try to get an active super block reference to prevent racing with
-	 * unmount (see trylock_super()).  But note that unmount isn't the only
-	 * place where a write lock on s_umount is taken, and we can fail here
-	 * because of things like remount as well.
-	 */
-	if (down_read_trylock(&sb->s_umount)) {
-		atomic_inc(&sb->s_active);
-		up_read(&sb->s_umount);
-		if (!queue_work(gfs2_freeze_wq, &sdp->sd_freeze_work))
-			deactivate_super(sb);
-	}
+	queue_work(gfs2_freeze_wq, &sdp->sd_freeze_work);
 }
 
 /**
@@ -588,7 +576,8 @@ static int freeze_go_xmote_bh(struct gfs2_glock *gl)
 	struct gfs2_log_header_host head;
 	int error;
 
-	if (test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags)) {
+	if (test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags) &&
+	    !test_bit(SDF_FROZEN, &sdp->sd_flags)) {
 		j_gl->gl_ops->go_inval(j_gl, DIO_METADATA);
 
 		error = gfs2_find_jhead(sdp->sd_jdesc, &head, false);
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index ce0e6d5e0e47..0c6a9e09b5cb 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -533,7 +533,8 @@ static void gfs2_dirty_inode(struct inode *inode, int flags)
 
 void gfs2_make_fs_ro(struct gfs2_sbd *sdp)
 {
-	int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
+	int log_write_allowed = test_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags) &&
+				!test_bit(SDF_FROZEN, &sdp->sd_flags);
 
 	gfs2_flush_delete_work(sdp);
 	if (!log_write_allowed && current == sdp->sd_quotad_process)
@@ -606,6 +607,7 @@ static void gfs2_put_super(struct super_block *sb)
 
 	/*  Release stuff  */
 
+	flush_work(&sdp->sd_freeze_work);
 	gfs2_freeze_unlock(&sdp->sd_freeze_gh);
 
 	iput(sdp->sd_jindex);
@@ -667,7 +669,10 @@ static int gfs2_freeze_locally(struct gfs2_sbd *sdp)
 	struct super_block *sb = sdp->sd_vfs;
 	int error;
 
-	error = freeze_super(sb);
+	if (!activate_super(sb))
+		return -EBUSY;
+	error = freeze_active_super(sb);
+	deactivate_super(sb);
 	if (error)
 		return error;
 
@@ -685,10 +690,22 @@ static int gfs2_enforce_thaw(struct gfs2_sbd *sdp)
 	struct super_block *sb = sdp->sd_vfs;
 	int error;
 
-	error = gfs2_freeze_lock_shared(sdp, 0);
+	error = gfs2_freeze_lock_shared(sdp, GL_ASYNC);
 	if (error)
 		goto fail;
-	error = thaw_super(sb);
+wait_longer:
+	error = gfs2_glock_async_wait(1, &sdp->sd_freeze_gh, 5 * HZ);
+	if (error && error != -ESTALE)
+		goto fail;
+	if (!activate_super(sb))
+		return -EBUSY;
+	if (error != 0) {
+		/* We don't have the lock, yet. */
+		deactivate_super(sb);
+		goto wait_longer;
+	}
+	error = thaw_active_super(sb);
+	deactivate_super(sb);
 	if (!error)
 		return 0;
 
@@ -701,7 +718,6 @@ static int gfs2_enforce_thaw(struct gfs2_sbd *sdp)
 void gfs2_freeze_func(struct work_struct *work)
 {
 	struct gfs2_sbd *sdp = container_of(work, struct gfs2_sbd, sd_freeze_work);
-	struct super_block *sb = sdp->sd_vfs;
 	int error;
 
 	mutex_lock(&sdp->sd_freeze_mutex);
@@ -724,7 +740,6 @@ void gfs2_freeze_func(struct work_struct *work)
 
 out_unlock:
 	mutex_unlock(&sdp->sd_freeze_mutex);
-	deactivate_super(sb);
 out:
 	if (error)
 		fs_info(sdp, "GFS2: couldn't freeze filesystem: %d\n", error);
-- 
2.38.1

Re: [PATCH 3/3] gfs2: Shut down frozen filesystem on last unmount

[Al Viro]

On Wed, Nov 30, 2022 at 12:07:35AM +0100, Andreas Gruenbacher wrote:
> So far, when a frozen filesystem is last unmouted, it turns into a
> zombie rather than being shut down; to shut it down, it needs to be
> remounted and thawed first.
> 
> That's silly for local filesystems, but it's worse for filesystems like
> gfs2 which freeze a filesystem cluster-wide when fsfreeze is called on
> one of the nodes.  Only the node that initiated a freeze is allowed to
> thaw the filesystem it again.  On the other nodes, the only way to shut
> down the remotely frozen filesystem is to power off.
> 
> Change that on gfs2 so that frozen filesystems are immediately shut down
> when they are last unmounted and removed from the filesystem namespace.
> This doesn't require writing to the filesystem, so the remaining cluster
> nodes remain undisturbed.

	So what are your preferred rules wrt active references vs. bdev
freeze depth vs. actual frozen state of fs?  The current situation is
already headache-inducing (and I wouldn't bet on correctness - e.g.
FITHAW vs. XFS_IOC_GOINDOWN is interesting).  And the variety of callbacks
(->thaw_super() vs. ->unfreeze_fs(), etc.) also doesn't help, to put it
mildly...

	Sure, gfs2 has unusual needs in that area, but it would be really
nice to get that thing regularized to something that would work for
all users of that machinery.

Re: [RFC 0/3] Shut down frozen filesystems on last unmount

[Jan Kara]

Hi Andreas!

On Wed 30-11-22 00:07:32, Andreas Gruenbacher wrote:
> currently, when a frozen filesystem is unmouted, it turns into a zombie
> rather than being shut down; it can only be shut down after remounting
> and thawing it.  That's silly for local filesystems, but it's worse for
> filesystems like gfs2 which freeze the filesystem on all nodes when
> fsfreeze is called on any of the nodes: there, the nodes that didn't
> initiate the freeze cannot shut down the filesystem at all.

I agree this situation is suboptimal ;)

> This is a non-working, first shot at allowing filesystems to shut down
> on the last unmount.  Could you please have a look to let me know if
> something like this makes sense?

So I had a look at the patches and I have to admit I'm not a huge fan of
this approach. For example if there's a utility doing disk image copy and
the filesystem gets unmounted, it could result in an inconsistent copy
AFAICT. Not for GFS2 as you argue but it seems a bit dangerous to provide
API that makes it easy to screw up. Also I dislike the fact that different
filesystem would behave differently wrt umount & freezing. Why cannot we
just block unmount when the filesystem is frozen like any other write
operation? I understand locking-wise it is a bit challenging because we
have to block in a place where we don't hold s_umount semaphore but
logically it would make sense to me. What do you think?

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR