[PATCH v2 00/10] Implement freeze and thaw as holder operations

[PATCH v2 00/10] Implement freeze and thaw as holder operations

[Christian Brauner]

Hey Christoph,
Hey Jan,
Hey Darrick,

This is v2 and based on vfs.super. I'm sending this out right now
because frankly, shortly before the merge window is the time when I have
time to do something. Otherwise, I would've waited a bit.

This implements block device freezing and thawing as holder operations.

This allows us to extend block device freezing to all devices associated
with a superblock and not just the main device.

It also allows us to remove get_active_super() and thus another function
that scans the global list of superblocks.

Freezing via additional block devices only works if the filesystem
chooses to use @fs_holder_ops for these additional devices as well. That
currently only includes ext4 and xfs.

Earlier releases switched get_tree_bdev() and mount_bdev() to use
@fs_holder_ops. The remaining nilfs2 open-coded version of mount_bdev()
has been converted to rely on @fs_holder_ops as well. So block device
freezing for the main block device will continue to work as before.

There should be no regressions in functionality. The only special case
is btrfs where block device freezing for the main block device never
worked because sb->s_bdev isn't set. Block device freezing for btrfs can
be fixed once they can switch to @fs_holder_ops but that can happen
whenever they're ready.

This survives

(1) xfstests: check -g quick
(2) xfstests: check -g freeze
(3) blktests: check

Thanks!
Christian

Signed-off-by: Christian Brauner <brauner@kernel.org>

Changes in v2:
- Call sync_blockdev() consistently under bd_holder_lock.
- Return early with error in fs_bdev_freeze() if we can't get an active
  reference and don't call sync_blockdev().
- Keep bd_fsfreeze_mutex now that we don't hold bd_holder_lock over
  freeze and thaw anymore.
- Link to v1: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-0-ecc36d9ab4d9@kernel.org

---



---
base-commit: 79ac81458fb58e1bac836450d6c68da1da9911d9
change-id: 20230927-vfs-super-freeze-eff650f66b06

[PATCH v2 01/10] fs: massage locking helpers

[Christian Brauner]

Multiple people have balked at the the fact that
super_lock{_shared,_excluse}() return booleans and even if they return
false hold s_umount. So let's change them to only hold s_umount when
true is returned and change the code accordingly.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 105 +++++++++++++++++++++++++++++++++++--------------------------
 1 file changed, 61 insertions(+), 44 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index c7b452e12e4c..9cf3ee50cecd 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -105,8 +105,9 @@ static inline bool wait_born(struct super_block *sb)
  *
  * The caller must have acquired a temporary reference on @sb->s_count.
  *
- * Return: This returns true if SB_BORN was set, false if SB_DYING was
- *         set. The function acquires s_umount and returns with it held.
+ * Return: The function returns true if SB_BORN was set and with
+ *         s_umount held. The function returns false if SB_DYING was
+ *         set and without s_umount held.
  */
 static __must_check bool super_lock(struct super_block *sb, bool excl)
 {
@@ -121,8 +122,10 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
 	 * @sb->s_root is NULL and @sb->s_active is 0. No one needs to
 	 * grab a reference to this. Tell them so.
 	 */
-	if (sb->s_flags & SB_DYING)
+	if (sb->s_flags & SB_DYING) {
+		super_unlock(sb, excl);
 		return false;
+	}
 
 	/* Has called ->get_tree() successfully. */
 	if (sb->s_flags & SB_BORN)
@@ -140,13 +143,13 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
 	goto relock;
 }
 
-/* wait and acquire read-side of @sb->s_umount */
+/* wait and try to acquire read-side of @sb->s_umount */
 static inline bool super_lock_shared(struct super_block *sb)
 {
 	return super_lock(sb, false);
 }
 
-/* wait and acquire write-side of @sb->s_umount */
+/* wait and try to acquire write-side of @sb->s_umount */
 static inline bool super_lock_excl(struct super_block *sb)
 {
 	return super_lock(sb, true);
@@ -532,16 +535,18 @@ EXPORT_SYMBOL(deactivate_super);
  */
 static int grab_super(struct super_block *s) __releases(sb_lock)
 {
-	bool born;
+	bool locked;
 
 	s->s_count++;
 	spin_unlock(&sb_lock);
-	born = super_lock_excl(s);
-	if (born && atomic_inc_not_zero(&s->s_active)) {
-		put_super(s);
-		return 1;
+	locked = super_lock_excl(s);
+	if (locked) {
+		if (atomic_inc_not_zero(&s->s_active)) {
+			put_super(s);
+			return 1;
+		}
+		super_unlock_excl(s);
 	}
-	super_unlock_excl(s);
 	put_super(s);
 	return 0;
 }
@@ -572,7 +577,6 @@ static inline bool wait_dead(struct super_block *sb)
  */
 static bool grab_super_dead(struct super_block *sb)
 {
-
 	sb->s_count++;
 	if (grab_super(sb)) {
 		put_super(sb);
@@ -958,15 +962,17 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 
 	spin_lock(&sb_lock);
 	list_for_each_entry(sb, &super_blocks, s_list) {
-		bool born;
+		bool locked;
 
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		born = super_lock_shared(sb);
-		if (born && sb->s_root)
-			f(sb, arg);
-		super_unlock_shared(sb);
+		locked = super_lock_shared(sb);
+		if (locked) {
+			if (sb->s_root)
+				f(sb, arg);
+			super_unlock_shared(sb);
+		}
 
 		spin_lock(&sb_lock);
 		if (p)
@@ -994,15 +1000,17 @@ void iterate_supers_type(struct file_system_type *type,
 
 	spin_lock(&sb_lock);
 	hlist_for_each_entry(sb, &type->fs_supers, s_instances) {
-		bool born;
+		bool locked;
 
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		born = super_lock_shared(sb);
-		if (born && sb->s_root)
-			f(sb, arg);
-		super_unlock_shared(sb);
+		locked = super_lock_shared(sb);
+		if (locked) {
+			if (sb->s_root)
+				f(sb, arg);
+			super_unlock_shared(sb);
+		}
 
 		spin_lock(&sb_lock);
 		if (p)
@@ -1051,15 +1059,17 @@ struct super_block *user_get_super(dev_t dev, bool excl)
 	spin_lock(&sb_lock);
 	list_for_each_entry(sb, &super_blocks, s_list) {
 		if (sb->s_dev ==  dev) {
-			bool born;
+			bool locked;
 
 			sb->s_count++;
 			spin_unlock(&sb_lock);
 			/* still alive? */
-			born = super_lock(sb, excl);
-			if (born && sb->s_root)
-				return sb;
-			super_unlock(sb, excl);
+			locked = super_lock(sb, excl);
+			if (locked) {
+				if (sb->s_root)
+					return sb;
+				super_unlock(sb, excl);
+			}
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
 			__put_super(sb);
@@ -1170,9 +1180,9 @@ int reconfigure_super(struct fs_context *fc)
 
 static void do_emergency_remount_callback(struct super_block *sb)
 {
-	bool born = super_lock_excl(sb);
+	bool locked = super_lock_excl(sb);
 
-	if (born && sb->s_root && sb->s_bdev && !sb_rdonly(sb)) {
+	if (locked && sb->s_root && sb->s_bdev && !sb_rdonly(sb)) {
 		struct fs_context *fc;
 
 		fc = fs_context_for_reconfigure(sb->s_root,
@@ -1183,7 +1193,8 @@ static void do_emergency_remount_callback(struct super_block *sb)
 			put_fs_context(fc);
 		}
 	}
-	super_unlock_excl(sb);
+	if (locked)
+		super_unlock_excl(sb);
 }
 
 static void do_emergency_remount(struct work_struct *work)
@@ -1206,16 +1217,17 @@ void emergency_remount(void)
 
 static void do_thaw_all_callback(struct super_block *sb)
 {
-	bool born = super_lock_excl(sb);
+	bool locked = super_lock_excl(sb);
 
-	if (born && sb->s_root) {
+	if (locked && sb->s_root) {
 		if (IS_ENABLED(CONFIG_BLOCK))
 			while (sb->s_bdev && !thaw_bdev(sb->s_bdev))
 				pr_warn("Emergency Thaw on %pg\n", sb->s_bdev);
 		thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
-	} else {
-		super_unlock_excl(sb);
+		return;
 	}
+	if (locked)
+		super_unlock_excl(sb);
 }
 
 static void do_thaw_all(struct work_struct *work)
@@ -1429,7 +1441,7 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 	__releases(&bdev->bd_holder_lock)
 {
 	struct super_block *sb = bdev->bd_holder;
-	bool born;
+	bool locked;
 
 	lockdep_assert_held(&bdev->bd_holder_lock);
 	lockdep_assert_not_held(&sb->s_umount);
@@ -1441,9 +1453,8 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 	spin_unlock(&sb_lock);
 	mutex_unlock(&bdev->bd_holder_lock);
 
-	born = super_lock_shared(sb);
-	if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
-		super_unlock_shared(sb);
+	locked = super_lock_shared(sb);
+	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
 		put_super(sb);
 		return NULL;
 	}
@@ -1959,9 +1970,11 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 {
 	int ret;
 
+	if (!super_lock_excl(sb)) {
+		WARN_ON_ONCE("Dying superblock while freezing!");
+		return -EINVAL;
+	}
 	atomic_inc(&sb->s_active);
-	if (!super_lock_excl(sb))
-		WARN(1, "Dying superblock while freezing!");
 
 retry:
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
@@ -2009,8 +2022,10 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
 	super_unlock_excl(sb);
 	sb_wait_write(sb, SB_FREEZE_WRITE);
-	if (!super_lock_excl(sb))
-		WARN(1, "Dying superblock while freezing!");
+	if (!super_lock_excl(sb)) {
+		WARN_ON_ONCE("Dying superblock while freezing!");
+		return -EINVAL;
+	}
 
 	/* Now we go and block page faults... */
 	sb->s_writers.frozen = SB_FREEZE_PAGEFAULT;
@@ -2128,8 +2143,10 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who)
 {
-	if (!super_lock_excl(sb))
-		WARN(1, "Dying superblock while thawing!");
+	if (!super_lock_excl(sb)) {
+		WARN_ON_ONCE("Dying superblock while thawing!");
+		return -EINVAL;
+	}
 	return thaw_super_locked(sb, who);
 }
 EXPORT_SYMBOL(thaw_super);

-- 
2.34.1

[PATCH v2 02/10] bdev: rename freeze and thaw helpers

[Christian Brauner]

We have bdev_mark_dead() etc and we're going to move block device
freezing to holder ops in the next patch. Make the naming consistent:

* freeze_bdev() -> bdev_freeze()
* thaw_bdev()   -> bdev_thaw()

Also document the return code.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-1-ecc36d9ab4d9@kernel.org
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c           | 22 +++++++++++++---------
 drivers/md/dm.c        |  4 ++--
 fs/ext4/ioctl.c        |  4 ++--
 fs/f2fs/file.c         |  4 ++--
 fs/super.c             |  4 ++--
 fs/xfs/xfs_fsops.c     |  4 ++--
 include/linux/blkdev.h |  4 ++--
 7 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index 2018d250e131..d674ad381c52 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -207,18 +207,20 @@ int sync_blockdev_range(struct block_device *bdev, loff_t lstart, loff_t lend)
 EXPORT_SYMBOL(sync_blockdev_range);
 
 /**
- * freeze_bdev - lock a filesystem and force it into a consistent state
+ * bdev_freeze - lock a filesystem and force it into a consistent state
  * @bdev:	blockdevice to lock
  *
  * If a superblock is found on this device, we take the s_umount semaphore
  * on it to make sure nobody unmounts until the snapshot creation is done.
  * The reference counter (bd_fsfreeze_count) guarantees that only the last
  * unfreeze process can unfreeze the frozen filesystem actually when multiple
- * freeze requests arrive simultaneously. It counts up in freeze_bdev() and
- * count down in thaw_bdev(). When it becomes 0, thaw_bdev() will unfreeze
+ * freeze requests arrive simultaneously. It counts up in bdev_freeze() and
+ * count down in bdev_thaw(). When it becomes 0, thaw_bdev() will unfreeze
  * actually.
+ *
+ * Return: On success zero is returned, negative error code on failure.
  */
-int freeze_bdev(struct block_device *bdev)
+int bdev_freeze(struct block_device *bdev)
 {
 	struct super_block *sb;
 	int error = 0;
@@ -248,15 +250,17 @@ int freeze_bdev(struct block_device *bdev)
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
 }
-EXPORT_SYMBOL(freeze_bdev);
+EXPORT_SYMBOL(bdev_freeze);
 
 /**
- * thaw_bdev - unlock filesystem
+ * bdev_thaw - unlock filesystem
  * @bdev:	blockdevice to unlock
  *
- * Unlocks the filesystem and marks it writeable again after freeze_bdev().
+ * Unlocks the filesystem and marks it writeable again after bdev_freeze().
+ *
+ * Return: On success zero is returned, negative error code on failure.
  */
-int thaw_bdev(struct block_device *bdev)
+int bdev_thaw(struct block_device *bdev)
 {
 	struct super_block *sb;
 	int error = -EINVAL;
@@ -285,7 +289,7 @@ int thaw_bdev(struct block_device *bdev)
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
 }
-EXPORT_SYMBOL(thaw_bdev);
+EXPORT_SYMBOL(bdev_thaw);
 
 /*
  * pseudo-fs
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index f7212e8fc27f..c14dc6db0810 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -2650,7 +2650,7 @@ static int lock_fs(struct mapped_device *md)
 
 	WARN_ON(test_bit(DMF_FROZEN, &md->flags));
 
-	r = freeze_bdev(md->disk->part0);
+	r = bdev_freeze(md->disk->part0);
 	if (!r)
 		set_bit(DMF_FROZEN, &md->flags);
 	return r;
@@ -2660,7 +2660,7 @@ static void unlock_fs(struct mapped_device *md)
 {
 	if (!test_bit(DMF_FROZEN, &md->flags))
 		return;
-	thaw_bdev(md->disk->part0);
+	bdev_thaw(md->disk->part0);
 	clear_bit(DMF_FROZEN, &md->flags);
 }
 
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index 0bfe2ce589e2..c1390219c945 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -810,11 +810,11 @@ int ext4_force_shutdown(struct super_block *sb, u32 flags)
 
 	switch (flags) {
 	case EXT4_GOING_FLAGS_DEFAULT:
-		ret = freeze_bdev(sb->s_bdev);
+		ret = bdev_freeze(sb->s_bdev);
 		if (ret)
 			return ret;
 		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
-		thaw_bdev(sb->s_bdev);
+		bdev_thaw(sb->s_bdev);
 		break;
 	case EXT4_GOING_FLAGS_LOGFLUSH:
 		set_bit(EXT4_FLAGS_SHUTDOWN, &sbi->s_ext4_flags);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index ca5904129b16..c22aeb9ffb61 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -2239,11 +2239,11 @@ static int f2fs_ioc_shutdown(struct file *filp, unsigned long arg)
 
 	switch (in) {
 	case F2FS_GOING_DOWN_FULLSYNC:
-		ret = freeze_bdev(sb->s_bdev);
+		ret = bdev_freeze(sb->s_bdev);
 		if (ret)
 			goto out;
 		f2fs_stop_checkpoint(sbi, false, STOP_CP_REASON_SHUTDOWN);
-		thaw_bdev(sb->s_bdev);
+		bdev_thaw(sb->s_bdev);
 		break;
 	case F2FS_GOING_DOWN_METASYNC:
 		/* do checkpoint only */
diff --git a/fs/super.c b/fs/super.c
index 9cf3ee50cecd..b224182f2440 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1221,7 +1221,7 @@ static void do_thaw_all_callback(struct super_block *sb)
 
 	if (locked && sb->s_root) {
 		if (IS_ENABLED(CONFIG_BLOCK))
-			while (sb->s_bdev && !thaw_bdev(sb->s_bdev))
+			while (sb->s_bdev && !bdev_thaw(sb->s_bdev))
 				pr_warn("Emergency Thaw on %pg\n", sb->s_bdev);
 		thaw_super_locked(sb, FREEZE_HOLDER_USERSPACE);
 		return;
@@ -1529,7 +1529,7 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 	/*
 	 * Until SB_BORN flag is set, there can be no active superblock
 	 * references and thus no filesystem freezing. get_active_super() will
-	 * just loop waiting for SB_BORN so even freeze_bdev() cannot proceed.
+	 * just loop waiting for SB_BORN so even bdev_freeze() cannot proceed.
 	 *
 	 * It is enough to check bdev was not frozen before we set s_bdev.
 	 */
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 7cb75cb6b8e9..57076a25f17d 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -482,9 +482,9 @@ xfs_fs_goingdown(
 {
 	switch (inflags) {
 	case XFS_FSOP_GOING_FLAGS_DEFAULT: {
-		if (!freeze_bdev(mp->m_super->s_bdev)) {
+		if (!bdev_freeze(mp->m_super->s_bdev)) {
 			xfs_force_shutdown(mp, SHUTDOWN_FORCE_UMOUNT);
-			thaw_bdev(mp->m_super->s_bdev);
+			bdev_thaw(mp->m_super->s_bdev);
 		}
 		break;
 	}
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 51fa7ffdee83..7a3da7f44afb 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1541,8 +1541,8 @@ static inline int early_lookup_bdev(const char *pathname, dev_t *dev)
 }
 #endif /* CONFIG_BLOCK */
 
-int freeze_bdev(struct block_device *bdev);
-int thaw_bdev(struct block_device *bdev);
+int bdev_freeze(struct block_device *bdev);
+int bdev_thaw(struct block_device *bdev);
 
 struct io_comp_batch {
 	struct request *req_list;

-- 
2.34.1

[PATCH v2 03/10] bdev: surface the error from sync_blockdev()

[Christian Brauner]

When freeze_super() is called, sync_filesystem() will be called which
calls sync_blockdev() and already surfaces any errors. Do the same for
block devices that aren't owned by a superblock and also for filesystems
that don't call sync_blockdev() internally but implicitly rely on
bdev_freeze() to do it.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/bdev.c b/block/bdev.c
index d674ad381c52..a3e2af580a73 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -245,7 +245,7 @@ int bdev_freeze(struct block_device *bdev)
 	bdev->bd_fsfreeze_sb = sb;
 
 sync:
-	sync_blockdev(bdev);
+	error = sync_blockdev(bdev);
 done:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;

-- 
2.34.1

[PATCH v2 04/10] bdev: add freeze and thaw holder operations

[Christian Brauner]

Add block device freeze and thaw holder operations. Follow-up patches
will implement block device freeze and thaw based on stuct
blk_holder_ops.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-2-ecc36d9ab4d9@kernel.org
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/blkdev.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7a3da7f44afb..1bc776335ff8 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1468,6 +1468,16 @@ struct blk_holder_ops {
 	 * Sync the file system mounted on the block device.
 	 */
 	void (*sync)(struct block_device *bdev);
+
+	/*
+	 * Freeze the file system mounted on the block device.
+	 */
+	int (*freeze)(struct block_device *bdev);
+
+	/*
+	 * Thaw the file system mounted on the block device.
+	 */
+	int (*thaw)(struct block_device *bdev);
 };
 
 extern const struct blk_holder_ops fs_holder_ops;

-- 
2.34.1

[PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Christian Brauner]

The old method of implementing block device freeze and thaw operations
required us to rely on get_active_super() to walk the list of all
superblocks on the system to find any superblock that might use the
block device. This is wasteful and not very pleasant overall.

Now that we can finally go straight from block device to owning
superblock things become way simpler.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c              |  62 +++++++++++------------
 fs/super.c                | 124 ++++++++++++++++++++++++++++++++++++----------
 include/linux/blk_types.h |   2 +-
 3 files changed, 128 insertions(+), 60 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index a3e2af580a73..9deacd346192 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -222,31 +222,24 @@ EXPORT_SYMBOL(sync_blockdev_range);
  */
 int bdev_freeze(struct block_device *bdev)
 {
-	struct super_block *sb;
 	int error = 0;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (++bdev->bd_fsfreeze_count > 1)
-		goto done;
-
-	sb = get_active_super(bdev);
-	if (!sb)
-		goto sync;
-	if (sb->s_op->freeze_super)
-		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
-	else
-		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
-	deactivate_super(sb);
 
-	if (error) {
-		bdev->bd_fsfreeze_count--;
-		goto done;
+	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return 0;
+	}
+
+	mutex_lock(&bdev->bd_holder_lock);
+	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
+		error = bdev->bd_holder_ops->freeze(bdev);
+		lockdep_assert_not_held(&bdev->bd_holder_lock);
+	} else {
+		mutex_unlock(&bdev->bd_holder_lock);
+		error = sync_blockdev(bdev);
 	}
-	bdev->bd_fsfreeze_sb = sb;
 
-sync:
-	error = sync_blockdev(bdev);
-done:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
 }
@@ -262,29 +255,30 @@ EXPORT_SYMBOL(bdev_freeze);
  */
 int bdev_thaw(struct block_device *bdev)
 {
-	struct super_block *sb;
-	int error = -EINVAL;
+	int error = -EINVAL, nr_freeze;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (!bdev->bd_fsfreeze_count)
+
+	/*
+	 * If this returns < 0 it means that @bd_fsfreeze_count was
+	 * already 0 and no decrement was performed.
+	 */
+	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
+	if (nr_freeze < 0)
 		goto out;
 
 	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (nr_freeze > 0)
 		goto out;
 
-	sb = bdev->bd_fsfreeze_sb;
-	if (!sb)
-		goto out;
+	mutex_lock(&bdev->bd_holder_lock);
+	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
+		error = bdev->bd_holder_ops->thaw(bdev);
+		lockdep_assert_not_held(&bdev->bd_holder_lock);
+	} else {
+		mutex_unlock(&bdev->bd_holder_lock);
+	}
 
-	if (sb->s_op->thaw_super)
-		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
-	else
-		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
-	if (error)
-		bdev->bd_fsfreeze_count++;
-	else
-		bdev->bd_fsfreeze_sb = NULL;
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
diff --git a/fs/super.c b/fs/super.c
index b224182f2440..ee0795ce09c7 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1430,14 +1430,8 @@ struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
 EXPORT_SYMBOL(sget_dev);
 
 #ifdef CONFIG_BLOCK
-/*
- * Lock the superblock that is holder of the bdev. Returns the superblock
- * pointer if we successfully locked the superblock and it is alive. Otherwise
- * we return NULL and just unlock bdev->bd_holder_lock.
- *
- * The function must be called with bdev->bd_holder_lock and releases it.
- */
-static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
+
+static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
 	__releases(&bdev->bd_holder_lock)
 {
 	struct super_block *sb = bdev->bd_holder;
@@ -1451,18 +1445,37 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 	spin_lock(&sb_lock);
 	sb->s_count++;
 	spin_unlock(&sb_lock);
+
 	mutex_unlock(&bdev->bd_holder_lock);
 
-	locked = super_lock_shared(sb);
-	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
-		put_super(sb);
+	locked = super_lock(sb, excl);
+	put_super(sb);
+	if (!locked)
+		return NULL;
+
+	return sb;
+}
+
+/*
+ * Lock the superblock that is holder of the bdev. Returns the superblock
+ * pointer if we successfully locked the superblock and it is alive. Otherwise
+ * we return NULL and just unlock bdev->bd_holder_lock.
+ *
+ * The function must be called with bdev->bd_holder_lock and releases it.
+ */
+static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
+{
+	struct super_block *sb;
+
+	sb = bdev_super_lock(bdev, false);
+	if (!sb)
+		return NULL;
+
+	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
+		super_unlock_shared(sb);
 		return NULL;
 	}
-	/*
-	 * The superblock is active and we hold s_umount, we can drop our
-	 * temporary reference now.
-	 */
-	put_super(sb);
+
 	return sb;
 }
 
@@ -1495,9 +1508,76 @@ static void fs_bdev_sync(struct block_device *bdev)
 	super_unlock_shared(sb);
 }
 
+static struct super_block *get_bdev_super(struct block_device *bdev)
+{
+	bool active = false;
+	struct super_block *sb;
+
+	sb = bdev_super_lock(bdev, true);
+	if (sb) {
+		active = atomic_inc_not_zero(&sb->s_active);
+		super_unlock_excl(sb);
+	}
+	if (!active)
+		return NULL;
+	return sb;
+}
+
+static int fs_bdev_freeze(struct block_device *bdev)
+{
+	struct super_block *sb;
+	int error = 0;
+
+	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
+
+	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
+		return -EINVAL;
+
+	sb = get_bdev_super(bdev);
+	if (!sb)
+		return -EINVAL;
+
+	if (sb->s_op->freeze_super)
+		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+	else
+		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+	if (error)
+		atomic_dec(&bdev->bd_fsfreeze_count);
+	if (!error)
+		error = sync_blockdev(bdev);
+	deactivate_super(sb);
+	return error;
+}
+
+static int fs_bdev_thaw(struct block_device *bdev)
+{
+	struct super_block *sb;
+	int error;
+
+	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
+
+	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
+		return -EINVAL;
+
+	sb = get_bdev_super(bdev);
+	if (WARN_ON_ONCE(!sb))
+		return -EINVAL;
+
+	if (sb->s_op->thaw_super)
+		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+	else
+		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+	if (error)
+		atomic_inc(&bdev->bd_fsfreeze_count);
+	deactivate_super(sb);
+	return error;
+}
+
 const struct blk_holder_ops fs_holder_ops = {
 	.mark_dead		= fs_bdev_mark_dead,
 	.sync			= fs_bdev_sync,
+	.freeze			= fs_bdev_freeze,
+	.thaw			= fs_bdev_thaw,
 };
 EXPORT_SYMBOL_GPL(fs_holder_ops);
 
@@ -1527,15 +1607,10 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 	}
 
 	/*
-	 * Until SB_BORN flag is set, there can be no active superblock
-	 * references and thus no filesystem freezing. get_active_super() will
-	 * just loop waiting for SB_BORN so even bdev_freeze() cannot proceed.
-	 *
-	 * It is enough to check bdev was not frozen before we set s_bdev.
+	 * It is enough to check bdev was not frozen before we set
+	 * s_bdev as freezing will wait until SB_BORN is set.
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	if (atomic_read(&bdev->bd_fsfreeze_count) > 0) {
 		if (fc)
 			warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
 		bdev_release(bdev_handle);
@@ -1548,7 +1623,6 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 	if (bdev_stable_writes(bdev))
 		sb->s_iflags |= SB_I_STABLE_WRITES;
 	spin_unlock(&sb_lock);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 
 	snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
 	shrinker_debugfs_rename(&sb->s_shrink, "sb-%s:%s", sb->s_type->name,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d5c5e59ddbd2..88e1848b0869 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -57,7 +57,7 @@ struct block_device {
 	const struct blk_holder_ops *bd_holder_ops;
 	struct mutex		bd_holder_lock;
 	/* The counter of freeze processes */
-	int			bd_fsfreeze_count;
+	atomic_t		bd_fsfreeze_count;
 	int			bd_holders;
 	struct kobject		*bd_holder_dir;
 

-- 
2.34.1

[PATCH v2 06/10] fs: remove get_active_super()

[Christian Brauner]

This function is now unused so remove it. One less function that uses
the global superblock list.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-4-ecc36d9ab4d9@kernel.org
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c         | 28 ----------------------------
 include/linux/fs.h |  1 -
 2 files changed, 29 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index ee0795ce09c7..c9658ddb53f7 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1024,34 +1024,6 @@ void iterate_supers_type(struct file_system_type *type,
 
 EXPORT_SYMBOL(iterate_supers_type);
 
-/**
- * get_active_super - get an active reference to the superblock of a device
- * @bdev: device to get the superblock for
- *
- * Scans the superblock list and finds the superblock of the file system
- * mounted on the device given.  Returns the superblock with an active
- * reference or %NULL if none was found.
- */
-struct super_block *get_active_super(struct block_device *bdev)
-{
-	struct super_block *sb;
-
-	if (!bdev)
-		return NULL;
-
-	spin_lock(&sb_lock);
-	list_for_each_entry(sb, &super_blocks, s_list) {
-		if (sb->s_bdev == bdev) {
-			if (!grab_super(sb))
-				return NULL;
-			super_unlock_excl(sb);
-			return sb;
-		}
-	}
-	spin_unlock(&sb_lock);
-	return NULL;
-}
-
 struct super_block *user_get_super(dev_t dev, bool excl)
 {
 	struct super_block *sb;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 03afc8b6f2af..5174e821d451 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3053,7 +3053,6 @@ extern int vfs_readlink(struct dentry *, char __user *, int);
 extern struct file_system_type *get_filesystem(struct file_system_type *fs);
 extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
-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 *);

-- 
2.34.1

[PATCH v2 07/10] super: remove bd_fsfreeze_sb

[Christian Brauner]

Remove bd_fsfreeze_sb as it's now unused and can be removed. Also move
bd_fsfreeze_count down to not have it weirdly placed in the middle of
the holder fields.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-5-ecc36d9ab4d9@kernel.org
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Jan Kara <jack@suse.cz>
Suggested-by: Jan Kara <jack@suse.cz>
Suggested-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/blk_types.h | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index 88e1848b0869..749203277fee 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -56,14 +56,11 @@ struct block_device {
 	void *			bd_holder;
 	const struct blk_holder_ops *bd_holder_ops;
 	struct mutex		bd_holder_lock;
-	/* The counter of freeze processes */
-	atomic_t		bd_fsfreeze_count;
 	int			bd_holders;
 	struct kobject		*bd_holder_dir;
 
-	/* Mutex for freeze */
-	struct mutex		bd_fsfreeze_mutex;
-	struct super_block	*bd_fsfreeze_sb;
+	atomic_t		bd_fsfreeze_count; /* number of freeze requests */
+	struct mutex		bd_fsfreeze_mutex; /* serialize freeze/thaw */
 
 	struct partition_meta_info *bd_meta_info;
 #ifdef CONFIG_FAIL_MAKE_REQUEST

-- 
2.34.1

[PATCH v2 08/10] fs: remove unused helper

[Christian Brauner]

The grab_super() helper is now only used by grab_super_dead(). Merge the
two helpers into one.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-6-ecc36d9ab4d9@kernel.org
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jan Kara <jack@suse.cz>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 fs/super.c | 54 ++++++++++++++----------------------------------------
 1 file changed, 14 insertions(+), 40 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index c9658ddb53f7..b26b302f870d 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -520,37 +520,6 @@ void deactivate_super(struct super_block *s)
 
 EXPORT_SYMBOL(deactivate_super);
 
-/**
- *	grab_super - acquire an active reference
- *	@s: reference we are trying to make active
- *
- *	Tries to acquire an active reference.  grab_super() is used when we
- * 	had just found a superblock in super_blocks or fs_type->fs_supers
- *	and want to turn it into a full-blown active reference.  grab_super()
- *	is called with sb_lock held and drops it.  Returns 1 in case of
- *	success, 0 if we had failed (superblock contents was already dead or
- *	dying when grab_super() had been called).  Note that this is only
- *	called for superblocks not in rundown mode (== ones still on ->fs_supers
- *	of their type), so increment of ->s_count is OK here.
- */
-static int grab_super(struct super_block *s) __releases(sb_lock)
-{
-	bool locked;
-
-	s->s_count++;
-	spin_unlock(&sb_lock);
-	locked = super_lock_excl(s);
-	if (locked) {
-		if (atomic_inc_not_zero(&s->s_active)) {
-			put_super(s);
-			return 1;
-		}
-		super_unlock_excl(s);
-	}
-	put_super(s);
-	return 0;
-}
-
 static inline bool wait_dead(struct super_block *sb)
 {
 	unsigned int flags;
@@ -564,7 +533,7 @@ static inline bool wait_dead(struct super_block *sb)
 }
 
 /**
- * grab_super_dead - acquire an active reference to a superblock
+ * grab_super - acquire an active reference to a superblock
  * @sb: superblock to acquire
  *
  * Acquire a temporary reference on a superblock and try to trade it for
@@ -575,16 +544,21 @@ static inline bool wait_dead(struct super_block *sb)
  * Return: This returns true if an active reference could be acquired,
  *         false if not.
  */
-static bool grab_super_dead(struct super_block *sb)
+static bool grab_super(struct super_block *sb)
 {
+	bool locked;
+
 	sb->s_count++;
-	if (grab_super(sb)) {
-		put_super(sb);
-		lockdep_assert_held(&sb->s_umount);
-		return true;
+	spin_unlock(&sb_lock);
+	locked = super_lock_excl(sb);
+	if (locked) {
+		if (atomic_inc_not_zero(&sb->s_active)) {
+			put_super(sb);
+			return true;
+		}
+		super_unlock_excl(sb);
 	}
 	wait_var_event(&sb->s_flags, wait_dead(sb));
-	lockdep_assert_not_held(&sb->s_umount);
 	put_super(sb);
 	return false;
 }
@@ -835,7 +809,7 @@ struct super_block *sget_fc(struct fs_context *fc,
 			warnfc(fc, "reusing existing filesystem in another namespace not allowed");
 		return ERR_PTR(-EBUSY);
 	}
-	if (!grab_super_dead(old))
+	if (!grab_super(old))
 		goto retry;
 	destroy_unused_super(s);
 	return old;
@@ -879,7 +853,7 @@ struct super_block *sget(struct file_system_type *type,
 				destroy_unused_super(s);
 				return ERR_PTR(-EBUSY);
 			}
-			if (!grab_super_dead(old))
+			if (!grab_super(old))
 				goto retry;
 			destroy_unused_super(s);
 			return old;

-- 
2.34.1

[PATCH v2 09/10] porting: document block device freeze and thaw changes

[Christian Brauner]

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-7-ecc36d9ab4d9@kernel.org
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 Documentation/filesystems/porting.rst | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
index d69f59700a23..78fc854c9e41 100644
--- a/Documentation/filesystems/porting.rst
+++ b/Documentation/filesystems/porting.rst
@@ -1052,3 +1052,15 @@ kill_anon_super(), or kill_block_super() helpers.
 
 Lock ordering has been changed so that s_umount ranks above open_mutex again.
 All places where s_umount was taken under open_mutex have been fixed up.
+
+---
+
+**recommended**
+
+Block device freezing and thawing have been moved to holder operations.
+
+Before this change, get_active_super() would only be able to find the
+superblock of the main block device, i.e., the one stored in sb->s_bdev. Block
+device freezing now works for any block device owned by a given superblock, not
+just the main block device. The get_active_super() helper is gone, so are
+bd_fsfreeze_sb, and bd_fsfreeze_mutex.

-- 
2.34.1

[PATCH v2 10/10] blkdev: comment fs_holder_ops

[Christian Brauner]

Add a comment to @fs_holder_ops that @holder must point to a superblock.

Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 include/linux/blkdev.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 1bc776335ff8..abf71cce785c 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -1480,6 +1480,11 @@ struct blk_holder_ops {
 	int (*thaw)(struct block_device *bdev);
 };
 
+/*
+ * For filesystems using @fs_holder_ops, the @holder argument passed to
+ * helpers used to open and claim block devices via
+ * bd_prepare_to_claim() must point to a superblock.
+ */
 extern const struct blk_holder_ops fs_holder_ops;
 
 /*

-- 
2.34.1

Re: [PATCH v2 03/10] bdev: surface the error from sync_blockdev()

[Darrick J. Wong]

On Tue, Oct 24, 2023 at 03:01:09PM +0200, Christian Brauner wrote:
> When freeze_super() is called, sync_filesystem() will be called which
> calls sync_blockdev() and already surfaces any errors. Do the same for
> block devices that aren't owned by a superblock and also for filesystems
> that don't call sync_blockdev() internally but implicitly rely on
> bdev_freeze() to do it.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Yessss less EIO munching in the VFS layer!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  block/bdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index d674ad381c52..a3e2af580a73 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -245,7 +245,7 @@ int bdev_freeze(struct block_device *bdev)
>  	bdev->bd_fsfreeze_sb = sb;
>  
>  sync:
> -	sync_blockdev(bdev);
> +	error = sync_blockdev(bdev);
>  done:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 10/10] blkdev: comment fs_holder_ops

[Darrick J. Wong]

On Tue, Oct 24, 2023 at 03:01:16PM +0200, Christian Brauner wrote:
> Add a comment to @fs_holder_ops that @holder must point to a superblock.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  include/linux/blkdev.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 1bc776335ff8..abf71cce785c 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1480,6 +1480,11 @@ struct blk_holder_ops {
>  	int (*thaw)(struct block_device *bdev);
>  };
>  
> +/*
> + * For filesystems using @fs_holder_ops, the @holder argument passed to
> + * helpers used to open and claim block devices via
> + * bd_prepare_to_claim() must point to a superblock.
> + */
>  extern const struct blk_holder_ops fs_holder_ops;
>  
>  /*
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 09/10] porting: document block device freeze and thaw changes

[Darrick J. Wong]

On Tue, Oct 24, 2023 at 03:01:15PM +0200, Christian Brauner wrote:
> Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-7-ecc36d9ab4d9@kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  Documentation/filesystems/porting.rst | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/Documentation/filesystems/porting.rst b/Documentation/filesystems/porting.rst
> index d69f59700a23..78fc854c9e41 100644
> --- a/Documentation/filesystems/porting.rst
> +++ b/Documentation/filesystems/porting.rst
> @@ -1052,3 +1052,15 @@ kill_anon_super(), or kill_block_super() helpers.
>  
>  Lock ordering has been changed so that s_umount ranks above open_mutex again.
>  All places where s_umount was taken under open_mutex have been fixed up.
> +
> +---
> +
> +**recommended**
> +
> +Block device freezing and thawing have been moved to holder operations.
> +
> +Before this change, get_active_super() would only be able to find the
> +superblock of the main block device, i.e., the one stored in sb->s_bdev. Block
> +device freezing now works for any block device owned by a given superblock, not
> +just the main block device. The get_active_super() helper is gone, so are
> +bd_fsfreeze_sb, and bd_fsfreeze_mutex.
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Darrick J. Wong]

On Tue, Oct 24, 2023 at 03:01:11PM +0200, Christian Brauner wrote:
> The old method of implementing block device freeze and thaw operations
> required us to rely on get_active_super() to walk the list of all
> superblocks on the system to find any superblock that might use the
> block device. This is wasteful and not very pleasant overall.
> 
> Now that we can finally go straight from block device to owning
> superblock things become way simpler.
> 
> Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good now,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  block/bdev.c              |  62 +++++++++++------------
>  fs/super.c                | 124 ++++++++++++++++++++++++++++++++++++----------
>  include/linux/blk_types.h |   2 +-
>  3 files changed, 128 insertions(+), 60 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index a3e2af580a73..9deacd346192 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -222,31 +222,24 @@ EXPORT_SYMBOL(sync_blockdev_range);
>   */
>  int bdev_freeze(struct block_device *bdev)
>  {
> -	struct super_block *sb;
>  	int error = 0;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (++bdev->bd_fsfreeze_count > 1)
> -		goto done;
> -
> -	sb = get_active_super(bdev);
> -	if (!sb)
> -		goto sync;
> -	if (sb->s_op->freeze_super)
> -		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	deactivate_super(sb);
>  
> -	if (error) {
> -		bdev->bd_fsfreeze_count--;
> -		goto done;
> +	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;
> +	}
> +
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
> +		error = bdev->bd_holder_ops->freeze(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +		error = sync_blockdev(bdev);
>  	}
> -	bdev->bd_fsfreeze_sb = sb;
>  
> -sync:
> -	error = sync_blockdev(bdev);
> -done:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
>  }
> @@ -262,29 +255,30 @@ EXPORT_SYMBOL(bdev_freeze);
>   */
>  int bdev_thaw(struct block_device *bdev)
>  {
> -	struct super_block *sb;
> -	int error = -EINVAL;
> +	int error = -EINVAL, nr_freeze;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (!bdev->bd_fsfreeze_count)
> +
> +	/*
> +	 * If this returns < 0 it means that @bd_fsfreeze_count was
> +	 * already 0 and no decrement was performed.
> +	 */
> +	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
> +	if (nr_freeze < 0)
>  		goto out;
>  
>  	error = 0;
> -	if (--bdev->bd_fsfreeze_count > 0)
> +	if (nr_freeze > 0)
>  		goto out;
>  
> -	sb = bdev->bd_fsfreeze_sb;
> -	if (!sb)
> -		goto out;
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
> +		error = bdev->bd_holder_ops->thaw(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +	}
>  
> -	if (sb->s_op->thaw_super)
> -		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> -	if (error)
> -		bdev->bd_fsfreeze_count++;
> -	else
> -		bdev->bd_fsfreeze_sb = NULL;
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> diff --git a/fs/super.c b/fs/super.c
> index b224182f2440..ee0795ce09c7 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1430,14 +1430,8 @@ struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
>  EXPORT_SYMBOL(sget_dev);
>  
>  #ifdef CONFIG_BLOCK
> -/*
> - * Lock the superblock that is holder of the bdev. Returns the superblock
> - * pointer if we successfully locked the superblock and it is alive. Otherwise
> - * we return NULL and just unlock bdev->bd_holder_lock.
> - *
> - * The function must be called with bdev->bd_holder_lock and releases it.
> - */
> -static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> +
> +static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
>  	__releases(&bdev->bd_holder_lock)
>  {
>  	struct super_block *sb = bdev->bd_holder;
> @@ -1451,18 +1445,37 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
>  	spin_lock(&sb_lock);
>  	sb->s_count++;
>  	spin_unlock(&sb_lock);
> +
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
> -	locked = super_lock_shared(sb);
> -	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> -		put_super(sb);
> +	locked = super_lock(sb, excl);
> +	put_super(sb);
> +	if (!locked)
> +		return NULL;
> +
> +	return sb;
> +}
> +
> +/*
> + * Lock the superblock that is holder of the bdev. Returns the superblock
> + * pointer if we successfully locked the superblock and it is alive. Otherwise
> + * we return NULL and just unlock bdev->bd_holder_lock.
> + *
> + * The function must be called with bdev->bd_holder_lock and releases it.
> + */
> +static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +
> +	sb = bdev_super_lock(bdev, false);
> +	if (!sb)
> +		return NULL;
> +
> +	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> +		super_unlock_shared(sb);
>  		return NULL;
>  	}
> -	/*
> -	 * The superblock is active and we hold s_umount, we can drop our
> -	 * temporary reference now.
> -	 */
> -	put_super(sb);
> +
>  	return sb;
>  }
>  
> @@ -1495,9 +1508,76 @@ static void fs_bdev_sync(struct block_device *bdev)
>  	super_unlock_shared(sb);
>  }
>  
> +static struct super_block *get_bdev_super(struct block_device *bdev)
> +{
> +	bool active = false;
> +	struct super_block *sb;
> +
> +	sb = bdev_super_lock(bdev, true);
> +	if (sb) {
> +		active = atomic_inc_not_zero(&sb->s_active);
> +		super_unlock_excl(sb);
> +	}
> +	if (!active)
> +		return NULL;
> +	return sb;
> +}
> +
> +static int fs_bdev_freeze(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error = 0;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> +		return -EINVAL;
> +
> +	sb = get_bdev_super(bdev);
> +	if (!sb)
> +		return -EINVAL;
> +
> +	if (sb->s_op->freeze_super)
> +		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	if (error)
> +		atomic_dec(&bdev->bd_fsfreeze_count);
> +	if (!error)
> +		error = sync_blockdev(bdev);
> +	deactivate_super(sb);
> +	return error;
> +}
> +
> +static int fs_bdev_thaw(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> +		return -EINVAL;
> +
> +	sb = get_bdev_super(bdev);
> +	if (WARN_ON_ONCE(!sb))
> +		return -EINVAL;
> +
> +	if (sb->s_op->thaw_super)
> +		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	if (error)
> +		atomic_inc(&bdev->bd_fsfreeze_count);
> +	deactivate_super(sb);
> +	return error;
> +}
> +
>  const struct blk_holder_ops fs_holder_ops = {
>  	.mark_dead		= fs_bdev_mark_dead,
>  	.sync			= fs_bdev_sync,
> +	.freeze			= fs_bdev_freeze,
> +	.thaw			= fs_bdev_thaw,
>  };
>  EXPORT_SYMBOL_GPL(fs_holder_ops);
>  
> @@ -1527,15 +1607,10 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
>  	}
>  
>  	/*
> -	 * Until SB_BORN flag is set, there can be no active superblock
> -	 * references and thus no filesystem freezing. get_active_super() will
> -	 * just loop waiting for SB_BORN so even bdev_freeze() cannot proceed.
> -	 *
> -	 * It is enough to check bdev was not frozen before we set s_bdev.
> +	 * It is enough to check bdev was not frozen before we set
> +	 * s_bdev as freezing will wait until SB_BORN is set.
>  	 */
> -	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (bdev->bd_fsfreeze_count > 0) {
> -		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +	if (atomic_read(&bdev->bd_fsfreeze_count) > 0) {
>  		if (fc)
>  			warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
>  		bdev_release(bdev_handle);
> @@ -1548,7 +1623,6 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
>  	if (bdev_stable_writes(bdev))
>  		sb->s_iflags |= SB_I_STABLE_WRITES;
>  	spin_unlock(&sb_lock);
> -	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  
>  	snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
>  	shrinker_debugfs_rename(&sb->s_shrink, "sb-%s:%s", sb->s_type->name,
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index d5c5e59ddbd2..88e1848b0869 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -57,7 +57,7 @@ struct block_device {
>  	const struct blk_holder_ops *bd_holder_ops;
>  	struct mutex		bd_holder_lock;
>  	/* The counter of freeze processes */
> -	int			bd_fsfreeze_count;
> +	atomic_t		bd_fsfreeze_count;
>  	int			bd_holders;
>  	struct kobject		*bd_holder_dir;
>  
> 
> -- 
> 2.34.1
> 

Re: [PATCH v2 01/10] fs: massage locking helpers

[Jan Kara]

On Tue 24-10-23 15:01:07, Christian Brauner wrote:
> Multiple people have balked at the the fact that
> super_lock{_shared,_excluse}() return booleans and even if they return
> false hold s_umount. So let's change them to only hold s_umount when
> true is returned and change the code accordingly.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Yeah, it's easier to grasp calling convention I guess.

> @@ -1429,7 +1441,7 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
>  	__releases(&bdev->bd_holder_lock)
>  {
>  	struct super_block *sb = bdev->bd_holder;
> -	bool born;
> +	bool locked;
>  
>  	lockdep_assert_held(&bdev->bd_holder_lock);
>  	lockdep_assert_not_held(&sb->s_umount);
> @@ -1441,9 +1453,8 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
>  	spin_unlock(&sb_lock);
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
> -	born = super_lock_shared(sb);
> -	if (!born || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> -		super_unlock_shared(sb);
> +	locked = super_lock_shared(sb);
> +	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
>  		put_super(sb);
>  		return NULL;

Here if locked == true but say !(sb->s_flags & SB_ACTIVE), we fail to
unlock the superblock now AFAICT.

> @@ -1959,9 +1970,11 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  {
>  	int ret;
>  
> +	if (!super_lock_excl(sb)) {
> +		WARN_ON_ONCE("Dying superblock while freezing!");
> +		return -EINVAL;
> +	}
>  	atomic_inc(&sb->s_active);
> -	if (!super_lock_excl(sb))
> -		WARN(1, "Dying superblock while freezing!");
>  
>  retry:
>  	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> @@ -2009,8 +2022,10 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
>  	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
>  	super_unlock_excl(sb);
>  	sb_wait_write(sb, SB_FREEZE_WRITE);
> -	if (!super_lock_excl(sb))
> -		WARN(1, "Dying superblock while freezing!");
> +	if (!super_lock_excl(sb)) {
> +		WARN_ON_ONCE("Dying superblock while freezing!");
> +		return -EINVAL;
> +	}

And here if you really mean it with some kind of clean bail out, we should
somehow get rid of the s_active reference we have. But exactly because of
that getting super_lock_excl() failure here would be really weird...

Otherwise the patch looks good.

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

Re: [PATCH v2 03/10] bdev: surface the error from sync_blockdev()

[Jan Kara]

On Tue 24-10-23 15:01:09, Christian Brauner wrote:
> When freeze_super() is called, sync_filesystem() will be called which
> calls sync_blockdev() and already surfaces any errors. Do the same for
> block devices that aren't owned by a superblock and also for filesystems
> that don't call sync_blockdev() internally but implicitly rely on
> bdev_freeze() to do it.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>
								Honza

> ---
>  block/bdev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index d674ad381c52..a3e2af580a73 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -245,7 +245,7 @@ int bdev_freeze(struct block_device *bdev)
>  	bdev->bd_fsfreeze_sb = sb;
>  
>  sync:
> -	sync_blockdev(bdev);
> +	error = sync_blockdev(bdev);
>  done:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 01/10] fs: massage locking helpers

[Christian Brauner]

> Here if locked == true but say !(sb->s_flags & SB_ACTIVE), we fail to
> unlock the superblock now AFAICT.

Yeah, I've already fixed that up in-tree. I realized this because I've
fixed it correctly in the last patch.

> And here if you really mean it with some kind of clean bail out, we should
> somehow get rid of the s_active reference we have. But exactly because of
> that getting super_lock_excl() failure here would be really weird...
> 
> Otherwise the patch looks good.

With the above fix folded in can I take your Ack?

Re: [PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Jan Kara]

On Tue 24-10-23 15:01:11, Christian Brauner wrote:
> The old method of implementing block device freeze and thaw operations
> required us to rely on get_active_super() to walk the list of all
> superblocks on the system to find any superblock that might use the
> block device. This is wasteful and not very pleasant overall.
> 
> Now that we can finally go straight from block device to owning
> superblock things become way simpler.
> 
> Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Some comments below:

> diff --git a/block/bdev.c b/block/bdev.c
> index a3e2af580a73..9deacd346192 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -222,31 +222,24 @@ EXPORT_SYMBOL(sync_blockdev_range);
>   */
>  int bdev_freeze(struct block_device *bdev)
>  {
> -	struct super_block *sb;
>  	int error = 0;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (++bdev->bd_fsfreeze_count > 1)
> -		goto done;
> -
> -	sb = get_active_super(bdev);
> -	if (!sb)
> -		goto sync;
> -	if (sb->s_op->freeze_super)
> -		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	deactivate_super(sb);
>  
> -	if (error) {
> -		bdev->bd_fsfreeze_count--;
> -		goto done;
> +	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;
> +	}
> +
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
> +		error = bdev->bd_holder_ops->freeze(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +		error = sync_blockdev(bdev);
>  	}
> -	bdev->bd_fsfreeze_sb = sb;
>  
> -sync:
> -	error = sync_blockdev(bdev);
> -done:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;

It's somewhat strange that if we return error here, we still keep
bd_fsfreeze_count elevated. So I'd add here:

	if (error)
		atomic_dec(&bdev->bd_fsfreeze_count);

> @@ -262,29 +255,30 @@ EXPORT_SYMBOL(bdev_freeze);
>   */
>  int bdev_thaw(struct block_device *bdev)
>  {
> -	struct super_block *sb;
> -	int error = -EINVAL;
> +	int error = -EINVAL, nr_freeze;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (!bdev->bd_fsfreeze_count)
> +
> +	/*
> +	 * If this returns < 0 it means that @bd_fsfreeze_count was
> +	 * already 0 and no decrement was performed.
> +	 */
> +	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
> +	if (nr_freeze < 0)
>  		goto out;
>  
>  	error = 0;
> -	if (--bdev->bd_fsfreeze_count > 0)
> +	if (nr_freeze > 0)
>  		goto out;
>  
> -	sb = bdev->bd_fsfreeze_sb;
> -	if (!sb)
> -		goto out;
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
> +		error = bdev->bd_holder_ops->thaw(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +	}
>  
> -	if (sb->s_op->thaw_super)
> -		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> -	if (error)
> -		bdev->bd_fsfreeze_count++;
> -	else
> -		bdev->bd_fsfreeze_sb = NULL;

Here I'd also keep the error handling behavior and increment
bd_fsfreeze_count in case of error from ->thaw().

> diff --git a/fs/super.c b/fs/super.c
> index b224182f2440..ee0795ce09c7 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1430,14 +1430,8 @@ struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
>  EXPORT_SYMBOL(sget_dev);
>  
>  #ifdef CONFIG_BLOCK
> -/*
> - * Lock the superblock that is holder of the bdev. Returns the superblock
> - * pointer if we successfully locked the superblock and it is alive. Otherwise
> - * we return NULL and just unlock bdev->bd_holder_lock.
> - *
> - * The function must be called with bdev->bd_holder_lock and releases it.
> - */
> -static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> +
> +static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
>  	__releases(&bdev->bd_holder_lock)
>  {
>  	struct super_block *sb = bdev->bd_holder;
> @@ -1451,18 +1445,37 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
>  	spin_lock(&sb_lock);
>  	sb->s_count++;
>  	spin_unlock(&sb_lock);
> +
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
> -	locked = super_lock_shared(sb);
> -	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> -		put_super(sb);
> +	locked = super_lock(sb, excl);
> +	put_super(sb);

Perhaps you can preserve the comment before put_super() here like:
	/*
	 * The superblock is not SB_DYING and we hold s_umount, we can drop
	 * our temporary reference now.
	 */

> +	if (!locked)
> +		return NULL;
> +
> +	return sb;
> +}
> +
> +/*
> + * Lock the superblock that is holder of the bdev. Returns the superblock
> + * pointer if we successfully locked the superblock and it is alive. Otherwise
> + * we return NULL and just unlock bdev->bd_holder_lock.
> + *
> + * The function must be called with bdev->bd_holder_lock and releases it.
> + */
> +static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +
> +	sb = bdev_super_lock(bdev, false);
> +	if (!sb)
> +		return NULL;
> +
> +	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> +		super_unlock_shared(sb);

Any reason why you didn't keep these checks in bdev_super_lock()? The use
in get_bdev_super() is actually limited to active superblocks anyway. And
then we can just get rid of bdev_super_lock_shared() and use
bdev_super_lock(bdev, false) instead.

> @@ -1495,9 +1508,76 @@ static void fs_bdev_sync(struct block_device *bdev)
>  	super_unlock_shared(sb);
>  }
>  
> +static struct super_block *get_bdev_super(struct block_device *bdev)
> +{
> +	bool active = false;
> +	struct super_block *sb;
> +
> +	sb = bdev_super_lock(bdev, true);
> +	if (sb) {
> +		active = atomic_inc_not_zero(&sb->s_active);
> +		super_unlock_excl(sb);
> +	}
> +	if (!active)
> +		return NULL;
> +	return sb;
> +}
> +
> +static int fs_bdev_freeze(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error = 0;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> +		return -EINVAL;

This is going to break assumptions that bd_holder_lock is dropped by the
->freeze callback. Also the check seems a bit pointless to me given the
single callsite but whatever.

> +
> +	sb = get_bdev_super(bdev);
> +	if (!sb)
> +		return -EINVAL;
> +
> +	if (sb->s_op->freeze_super)
> +		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	if (error)
> +		atomic_dec(&bdev->bd_fsfreeze_count);

This IMHO belongs into bdev_freeze().

> +	if (!error)
> +		error = sync_blockdev(bdev);
> +	deactivate_super(sb);
> +	return error;
> +}
> +
> +static int fs_bdev_thaw(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> +		return -EINVAL;

The same comment about bd_holder_lock applies here...

> +
> +	sb = get_bdev_super(bdev);
> +	if (WARN_ON_ONCE(!sb))
> +		return -EINVAL;
> +
> +	if (sb->s_op->thaw_super)
> +		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	if (error)
> +		atomic_inc(&bdev->bd_fsfreeze_count);

And this error handling belongs into bdev_thaw().

> +	deactivate_super(sb);
> +	return error;
> +}
> +

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

Re: [PATCH v2 01/10] fs: massage locking helpers

[Jan Kara]

On Wed 25-10-23 15:21:06, Christian Brauner wrote:
> > Here if locked == true but say !(sb->s_flags & SB_ACTIVE), we fail to
> > unlock the superblock now AFAICT.
> 
> Yeah, I've already fixed that up in-tree. I realized this because I've
> fixed it correctly in the last patch.
> 
> > And here if you really mean it with some kind of clean bail out, we should
> > somehow get rid of the s_active reference we have. But exactly because of
> > that getting super_lock_excl() failure here would be really weird...
> > 
> > Otherwise the patch looks good.
> 
> With the above fix folded in can I take your Ack?

Yes. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 09/10] porting: document block device freeze and thaw changes

[Jan Kara]

On Tue 24-10-23 15:01:15, Christian Brauner wrote:
> Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-7-ecc36d9ab4d9@kernel.org
> Signed-off-by: Christian Brauner <brauner@kernel.org>
> ---

...

> +
> +---
> +
> +**recommended**
> +
> +Block device freezing and thawing have been moved to holder operations.
> +
> +Before this change, get_active_super() would only be able to find the
> +superblock of the main block device, i.e., the one stored in sb->s_bdev. Block
> +device freezing now works for any block device owned by a given superblock, not
> +just the main block device. The get_active_super() helper is gone, so are
> +bd_fsfreeze_sb, and bd_fsfreeze_mutex.

Well, bd_fsfreeze_mutex is still there... Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

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

Re: [PATCH v2 10/10] blkdev: comment fs_holder_ops

[Jan Kara]

On Tue 24-10-23 15:01:16, Christian Brauner wrote:
> Add a comment to @fs_holder_ops that @holder must point to a superblock.
> 
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>
								Honza


> ---
>  include/linux/blkdev.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 1bc776335ff8..abf71cce785c 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -1480,6 +1480,11 @@ struct blk_holder_ops {
>  	int (*thaw)(struct block_device *bdev);
>  };
>  
> +/*
> + * For filesystems using @fs_holder_ops, the @holder argument passed to
> + * helpers used to open and claim block devices via
> + * bd_prepare_to_claim() must point to a superblock.
> + */
>  extern const struct blk_holder_ops fs_holder_ops;
>  
>  /*
> 
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 10/10] blkdev: comment fs_holder_ops

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Christian Brauner]

On Wed, Oct 25, 2023 at 04:01:05PM +0200, Jan Kara wrote:
> On Tue 24-10-23 15:01:11, Christian Brauner wrote:
> > The old method of implementing block device freeze and thaw operations
> > required us to rely on get_active_super() to walk the list of all
> > superblocks on the system to find any superblock that might use the
> > block device. This is wasteful and not very pleasant overall.
> > 
> > Now that we can finally go straight from block device to owning
> > superblock things become way simpler.
> > 
> > Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
> > Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
> Some comments below:

Thanks, all great points!

> 
> > diff --git a/block/bdev.c b/block/bdev.c
> > index a3e2af580a73..9deacd346192 100644
> > --- a/block/bdev.c
> > +++ b/block/bdev.c
> > @@ -222,31 +222,24 @@ EXPORT_SYMBOL(sync_blockdev_range);
> >   */
> >  int bdev_freeze(struct block_device *bdev)
> >  {
> > -	struct super_block *sb;
> >  	int error = 0;
> >  
> >  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> > -	if (++bdev->bd_fsfreeze_count > 1)
> > -		goto done;
> > -
> > -	sb = get_active_super(bdev);
> > -	if (!sb)
> > -		goto sync;
> > -	if (sb->s_op->freeze_super)
> > -		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> > -	else
> > -		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> > -	deactivate_super(sb);
> >  
> > -	if (error) {
> > -		bdev->bd_fsfreeze_count--;
> > -		goto done;
> > +	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
> > +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> > +		return 0;
> > +	}
> > +
> > +	mutex_lock(&bdev->bd_holder_lock);
> > +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
> > +		error = bdev->bd_holder_ops->freeze(bdev);
> > +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> > +	} else {
> > +		mutex_unlock(&bdev->bd_holder_lock);
> > +		error = sync_blockdev(bdev);
> >  	}
> > -	bdev->bd_fsfreeze_sb = sb;
> >  
> > -sync:
> > -	error = sync_blockdev(bdev);
> > -done:
> >  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
> >  	return error;
> 
> It's somewhat strange that if we return error here, we still keep
> bd_fsfreeze_count elevated. So I'd add here:
> 
> 	if (error)
> 		atomic_dec(&bdev->bd_fsfreeze_count);

Yes, good point. Also means we can move it out of fs_bdev_freeze().

> 
> > @@ -262,29 +255,30 @@ EXPORT_SYMBOL(bdev_freeze);
> >   */
> >  int bdev_thaw(struct block_device *bdev)
> >  {
> > -	struct super_block *sb;
> > -	int error = -EINVAL;
> > +	int error = -EINVAL, nr_freeze;
> >  
> >  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> > -	if (!bdev->bd_fsfreeze_count)
> > +
> > +	/*
> > +	 * If this returns < 0 it means that @bd_fsfreeze_count was
> > +	 * already 0 and no decrement was performed.
> > +	 */
> > +	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
> > +	if (nr_freeze < 0)
> >  		goto out;
> >  
> >  	error = 0;
> > -	if (--bdev->bd_fsfreeze_count > 0)
> > +	if (nr_freeze > 0)
> >  		goto out;
> >  
> > -	sb = bdev->bd_fsfreeze_sb;
> > -	if (!sb)
> > -		goto out;
> > +	mutex_lock(&bdev->bd_holder_lock);
> > +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
> > +		error = bdev->bd_holder_ops->thaw(bdev);
> > +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> > +	} else {
> > +		mutex_unlock(&bdev->bd_holder_lock);
> > +	}
> >  
> > -	if (sb->s_op->thaw_super)
> > -		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> > -	else
> > -		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> > -	if (error)
> > -		bdev->bd_fsfreeze_count++;
> > -	else
> > -		bdev->bd_fsfreeze_sb = NULL;
> 
> Here I'd also keep the error handling behavior and increment
> bd_fsfreeze_count in case of error from ->thaw().

Yeah, the patch does that but in fs_bdev_thaw() which is I probably
because it used to rely on bd_holder_lock. Moving back to bdev_thaw(),
thanks!

> 
> > diff --git a/fs/super.c b/fs/super.c
> > index b224182f2440..ee0795ce09c7 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1430,14 +1430,8 @@ struct super_block *sget_dev(struct fs_context *fc, dev_t dev)
> >  EXPORT_SYMBOL(sget_dev);
> >  
> >  #ifdef CONFIG_BLOCK
> > -/*
> > - * Lock the superblock that is holder of the bdev. Returns the superblock
> > - * pointer if we successfully locked the superblock and it is alive. Otherwise
> > - * we return NULL and just unlock bdev->bd_holder_lock.
> > - *
> > - * The function must be called with bdev->bd_holder_lock and releases it.
> > - */
> > -static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> > +
> > +static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
> >  	__releases(&bdev->bd_holder_lock)
> >  {
> >  	struct super_block *sb = bdev->bd_holder;
> > @@ -1451,18 +1445,37 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> >  	spin_lock(&sb_lock);
> >  	sb->s_count++;
> >  	spin_unlock(&sb_lock);
> > +
> >  	mutex_unlock(&bdev->bd_holder_lock);
> >  
> > -	locked = super_lock_shared(sb);
> > -	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> > -		put_super(sb);
> > +	locked = super_lock(sb, excl);
> > +	put_super(sb);
> 
> Perhaps you can preserve the comment before put_super() here like:
> 	/*
> 	 * The superblock is not SB_DYING and we hold s_umount, we can drop
> 	 * our temporary reference now.
> 	 */

Yes, will do.

> 
> > +	if (!locked)
> > +		return NULL;
> > +
> > +	return sb;
> > +}
> > +
> > +/*
> > + * Lock the superblock that is holder of the bdev. Returns the superblock
> > + * pointer if we successfully locked the superblock and it is alive. Otherwise
> > + * we return NULL and just unlock bdev->bd_holder_lock.
> > + *
> > + * The function must be called with bdev->bd_holder_lock and releases it.
> > + */
> > +static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> > +{
> > +	struct super_block *sb;
> > +
> > +	sb = bdev_super_lock(bdev, false);
> > +	if (!sb)
> > +		return NULL;
> > +
> > +	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> > +		super_unlock_shared(sb);
> 
> Any reason why you didn't keep these checks in bdev_super_lock()? The use
> in get_bdev_super() is actually limited to active superblocks anyway. And
> then we can just get rid of bdev_super_lock_shared() and use
> bdev_super_lock(bdev, false) instead.

Yeah, good idea. Done.

> 
> > @@ -1495,9 +1508,76 @@ static void fs_bdev_sync(struct block_device *bdev)
> >  	super_unlock_shared(sb);
> >  }
> >  
> > +static struct super_block *get_bdev_super(struct block_device *bdev)
> > +{
> > +	bool active = false;
> > +	struct super_block *sb;
> > +
> > +	sb = bdev_super_lock(bdev, true);
> > +	if (sb) {
> > +		active = atomic_inc_not_zero(&sb->s_active);
> > +		super_unlock_excl(sb);
> > +	}
> > +	if (!active)
> > +		return NULL;
> > +	return sb;
> > +}
> > +
> > +static int fs_bdev_freeze(struct block_device *bdev)
> > +{
> > +	struct super_block *sb;
> > +	int error = 0;
> > +
> > +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> > +
> > +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> > +		return -EINVAL;
> 
> This is going to break assumptions that bd_holder_lock is dropped by the
> ->freeze callback. Also the check seems a bit pointless to me given the
> single callsite but whatever.
> 
> > +
> > +	sb = get_bdev_super(bdev);
> > +	if (!sb)
> > +		return -EINVAL;
> > +
> > +	if (sb->s_op->freeze_super)
> > +		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> > +	else
> > +		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> > +	if (error)
> > +		atomic_dec(&bdev->bd_fsfreeze_count);
> 
> This IMHO belongs into bdev_freeze().

Yeah, I've done that now.

> 
> > +	if (!error)
> > +		error = sync_blockdev(bdev);
> > +	deactivate_super(sb);
> > +	return error;
> > +}
> > +
> > +static int fs_bdev_thaw(struct block_device *bdev)
> > +{
> > +	struct super_block *sb;
> > +	int error;
> > +
> > +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> > +
> > +	if (WARN_ON_ONCE(unlikely(!bdev->bd_holder)))
> > +		return -EINVAL;
> 
> The same comment about bd_holder_lock applies here...
> 
> > +
> > +	sb = get_bdev_super(bdev);
> > +	if (WARN_ON_ONCE(!sb))
> > +		return -EINVAL;
> > +
> > +	if (sb->s_op->thaw_super)
> > +		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> > +	else
> > +		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> > +	if (error)
> > +		atomic_inc(&bdev->bd_fsfreeze_count);
> 
> And this error handling belongs into bdev_thaw().

Yes, done.

Here's the updated diff:

From ac7b3ae74f1eae9d85a9e179ecf7facc498e9bee Mon Sep 17 00:00:00 2001
From: Christian Brauner <brauner@kernel.org>
Date: Wed, 27 Sep 2023 15:21:16 +0200
Subject: [PATCH v3 05/10] bdev: implement freeze and thaw holder operations

The old method of implementing block device freeze and thaw operations
required us to rely on get_active_super() to walk the list of all
superblocks on the system to find any superblock that might use the
block device. This is wasteful and not very pleasant overall.

Now that we can finally go straight from block device to owning
superblock things become way simpler.

Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
Reviewed-by: Darrick J. Wong <djwong@kernel.org>
Signed-off-by: Christian Brauner <brauner@kernel.org>
---
 block/bdev.c              | 65 +++++++++++++------------
 fs/super.c                | 99 +++++++++++++++++++++++++++++++--------
 include/linux/blk_types.h |  2 +-
 3 files changed, 112 insertions(+), 54 deletions(-)

diff --git a/block/bdev.c b/block/bdev.c
index a3e2af580a73..4a502fb9b814 100644
--- a/block/bdev.c
+++ b/block/bdev.c
@@ -222,31 +222,27 @@ EXPORT_SYMBOL(sync_blockdev_range);
  */
 int bdev_freeze(struct block_device *bdev)
 {
-	struct super_block *sb;
 	int error = 0;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (++bdev->bd_fsfreeze_count > 1)
-		goto done;
-
-	sb = get_active_super(bdev);
-	if (!sb)
-		goto sync;
-	if (sb->s_op->freeze_super)
-		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
-	else
-		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
-	deactivate_super(sb);
 
-	if (error) {
-		bdev->bd_fsfreeze_count--;
-		goto done;
+	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
+		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+		return 0;
+	}
+
+	mutex_lock(&bdev->bd_holder_lock);
+	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
+		error = bdev->bd_holder_ops->freeze(bdev);
+		lockdep_assert_not_held(&bdev->bd_holder_lock);
+	} else {
+		mutex_unlock(&bdev->bd_holder_lock);
+		error = sync_blockdev(bdev);
 	}
-	bdev->bd_fsfreeze_sb = sb;
 
-sync:
-	error = sync_blockdev(bdev);
-done:
+	if (error)
+		atomic_dec(&bdev->bd_fsfreeze_count);
+
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
 }
@@ -262,29 +258,32 @@ EXPORT_SYMBOL(bdev_freeze);
  */
 int bdev_thaw(struct block_device *bdev)
 {
-	struct super_block *sb;
-	int error = -EINVAL;
+	int error = -EINVAL, nr_freeze;
 
 	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (!bdev->bd_fsfreeze_count)
+
+	/*
+	 * If this returns < 0 it means that @bd_fsfreeze_count was
+	 * already 0 and no decrement was performed.
+	 */
+	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
+	if (nr_freeze < 0)
 		goto out;
 
 	error = 0;
-	if (--bdev->bd_fsfreeze_count > 0)
+	if (nr_freeze > 0)
 		goto out;
 
-	sb = bdev->bd_fsfreeze_sb;
-	if (!sb)
-		goto out;
+	mutex_lock(&bdev->bd_holder_lock);
+	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
+		error = bdev->bd_holder_ops->thaw(bdev);
+		lockdep_assert_not_held(&bdev->bd_holder_lock);
+	} else {
+		mutex_unlock(&bdev->bd_holder_lock);
+	}
 
-	if (sb->s_op->thaw_super)
-		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
-	else
-		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
 	if (error)
-		bdev->bd_fsfreeze_count++;
-	else
-		bdev->bd_fsfreeze_sb = NULL;
+		atomic_inc(&bdev->bd_fsfreeze_count);
 out:
 	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 	return error;
diff --git a/fs/super.c b/fs/super.c
index b224182f2440..3b39da5334c5 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -1437,7 +1437,7 @@ EXPORT_SYMBOL(sget_dev);
  *
  * The function must be called with bdev->bd_holder_lock and releases it.
  */
-static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
+static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
 	__releases(&bdev->bd_holder_lock)
 {
 	struct super_block *sb = bdev->bd_holder;
@@ -1451,18 +1451,25 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 	spin_lock(&sb_lock);
 	sb->s_count++;
 	spin_unlock(&sb_lock);
+
 	mutex_unlock(&bdev->bd_holder_lock);
 
-	locked = super_lock_shared(sb);
-	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
-		put_super(sb);
-		return NULL;
-	}
+	locked = super_lock(sb, excl);
+
 	/*
-	 * The superblock is active and we hold s_umount, we can drop our
-	 * temporary reference now.
-	 */
+	 * If the superblock wasn't already SB_DYING then we hold
+	 * s_umount and can safely drop our temporary reference.
+         */
 	put_super(sb);
+
+	if (!locked)
+		return NULL;
+
+	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
+		super_unlock(sb, excl);
+		return NULL;
+	}
+
 	return sb;
 }
 
@@ -1470,7 +1477,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 {
 	struct super_block *sb;
 
-	sb = bdev_super_lock_shared(bdev);
+	sb = bdev_super_lock(bdev, false);
 	if (!sb)
 		return;
 
@@ -1488,16 +1495,74 @@ static void fs_bdev_sync(struct block_device *bdev)
 {
 	struct super_block *sb;
 
-	sb = bdev_super_lock_shared(bdev);
+	sb = bdev_super_lock(bdev, false);
 	if (!sb)
 		return;
+
 	sync_filesystem(sb);
 	super_unlock_shared(sb);
 }
 
+static struct super_block *get_bdev_super(struct block_device *bdev)
+{
+	bool active = false;
+	struct super_block *sb;
+
+	sb = bdev_super_lock(bdev, true);
+	if (sb) {
+		active = atomic_inc_not_zero(&sb->s_active);
+		super_unlock_excl(sb);
+	}
+	if (!active)
+		return NULL;
+	return sb;
+}
+
+static int fs_bdev_freeze(struct block_device *bdev)
+{
+	struct super_block *sb;
+	int error = 0;
+
+	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
+
+	sb = get_bdev_super(bdev);
+	if (!sb)
+		return -EINVAL;
+
+	if (sb->s_op->freeze_super)
+		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+	else
+		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
+	if (!error)
+		error = sync_blockdev(bdev);
+	deactivate_super(sb);
+	return error;
+}
+
+static int fs_bdev_thaw(struct block_device *bdev)
+{
+	struct super_block *sb;
+	int error;
+
+	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
+
+	sb = get_bdev_super(bdev);
+	if (WARN_ON_ONCE(!sb))
+		return -EINVAL;
+
+	if (sb->s_op->thaw_super)
+		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+	else
+		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
+	deactivate_super(sb);
+	return error;
+}
+
 const struct blk_holder_ops fs_holder_ops = {
 	.mark_dead		= fs_bdev_mark_dead,
 	.sync			= fs_bdev_sync,
+	.freeze			= fs_bdev_freeze,
+	.thaw			= fs_bdev_thaw,
 };
 EXPORT_SYMBOL_GPL(fs_holder_ops);
 
@@ -1527,15 +1592,10 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 	}
 
 	/*
-	 * Until SB_BORN flag is set, there can be no active superblock
-	 * references and thus no filesystem freezing. get_active_super() will
-	 * just loop waiting for SB_BORN so even bdev_freeze() cannot proceed.
-	 *
-	 * It is enough to check bdev was not frozen before we set s_bdev.
+	 * It is enough to check bdev was not frozen before we set
+	 * s_bdev as freezing will wait until SB_BORN is set.
 	 */
-	mutex_lock(&bdev->bd_fsfreeze_mutex);
-	if (bdev->bd_fsfreeze_count > 0) {
-		mutex_unlock(&bdev->bd_fsfreeze_mutex);
+	if (atomic_read(&bdev->bd_fsfreeze_count) > 0) {
 		if (fc)
 			warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
 		bdev_release(bdev_handle);
@@ -1548,7 +1608,6 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
 	if (bdev_stable_writes(bdev))
 		sb->s_iflags |= SB_I_STABLE_WRITES;
 	spin_unlock(&sb_lock);
-	mutex_unlock(&bdev->bd_fsfreeze_mutex);
 
 	snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
 	shrinker_debugfs_rename(&sb->s_shrink, "sb-%s:%s", sb->s_type->name,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index d5c5e59ddbd2..88e1848b0869 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -57,7 +57,7 @@ struct block_device {
 	const struct blk_holder_ops *bd_holder_ops;
 	struct mutex		bd_holder_lock;
 	/* The counter of freeze processes */
-	int			bd_fsfreeze_count;
+	atomic_t		bd_fsfreeze_count;
 	int			bd_holders;
 	struct kobject		*bd_holder_dir;
 
-- 
2.34.1

Re: [PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Jan Kara]

On Thu 26-10-23 10:44:27, Christian Brauner wrote:
> Here's the updated diff:
> 
> From ac7b3ae74f1eae9d85a9e179ecf7facc498e9bee Mon Sep 17 00:00:00 2001
> From: Christian Brauner <brauner@kernel.org>
> Date: Wed, 27 Sep 2023 15:21:16 +0200
> Subject: [PATCH v3 05/10] bdev: implement freeze and thaw holder operations
> 
> The old method of implementing block device freeze and thaw operations
> required us to rely on get_active_super() to walk the list of all
> superblocks on the system to find any superblock that might use the
> block device. This is wasteful and not very pleasant overall.
> 
> Now that we can finally go straight from block device to owning
> superblock things become way simpler.
> 
> Link: https://lore.kernel.org/r/20230927-vfs-super-freeze-v1-3-ecc36d9ab4d9@kernel.org
> Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> Signed-off-by: Christian Brauner <brauner@kernel.org>

Looks good to me now. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza


> ---
>  block/bdev.c              | 65 +++++++++++++------------
>  fs/super.c                | 99 +++++++++++++++++++++++++++++++--------
>  include/linux/blk_types.h |  2 +-
>  3 files changed, 112 insertions(+), 54 deletions(-)
> 
> diff --git a/block/bdev.c b/block/bdev.c
> index a3e2af580a73..4a502fb9b814 100644
> --- a/block/bdev.c
> +++ b/block/bdev.c
> @@ -222,31 +222,27 @@ EXPORT_SYMBOL(sync_blockdev_range);
>   */
>  int bdev_freeze(struct block_device *bdev)
>  {
> -	struct super_block *sb;
>  	int error = 0;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (++bdev->bd_fsfreeze_count > 1)
> -		goto done;
> -
> -	sb = get_active_super(bdev);
> -	if (!sb)
> -		goto sync;
> -	if (sb->s_op->freeze_super)
> -		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> -	deactivate_super(sb);
>  
> -	if (error) {
> -		bdev->bd_fsfreeze_count--;
> -		goto done;
> +	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;
> +	}
> +
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->freeze) {
> +		error = bdev->bd_holder_ops->freeze(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +		error = sync_blockdev(bdev);
>  	}
> -	bdev->bd_fsfreeze_sb = sb;
>  
> -sync:
> -	error = sync_blockdev(bdev);
> -done:
> +	if (error)
> +		atomic_dec(&bdev->bd_fsfreeze_count);
> +
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
>  }
> @@ -262,29 +258,32 @@ EXPORT_SYMBOL(bdev_freeze);
>   */
>  int bdev_thaw(struct block_device *bdev)
>  {
> -	struct super_block *sb;
> -	int error = -EINVAL;
> +	int error = -EINVAL, nr_freeze;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (!bdev->bd_fsfreeze_count)
> +
> +	/*
> +	 * If this returns < 0 it means that @bd_fsfreeze_count was
> +	 * already 0 and no decrement was performed.
> +	 */
> +	nr_freeze = atomic_dec_if_positive(&bdev->bd_fsfreeze_count);
> +	if (nr_freeze < 0)
>  		goto out;
>  
>  	error = 0;
> -	if (--bdev->bd_fsfreeze_count > 0)
> +	if (nr_freeze > 0)
>  		goto out;
>  
> -	sb = bdev->bd_fsfreeze_sb;
> -	if (!sb)
> -		goto out;
> +	mutex_lock(&bdev->bd_holder_lock);
> +	if (bdev->bd_holder_ops && bdev->bd_holder_ops->thaw) {
> +		error = bdev->bd_holder_ops->thaw(bdev);
> +		lockdep_assert_not_held(&bdev->bd_holder_lock);
> +	} else {
> +		mutex_unlock(&bdev->bd_holder_lock);
> +	}
>  
> -	if (sb->s_op->thaw_super)
> -		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> -	else
> -		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
>  	if (error)
> -		bdev->bd_fsfreeze_count++;
> -	else
> -		bdev->bd_fsfreeze_sb = NULL;
> +		atomic_inc(&bdev->bd_fsfreeze_count);
>  out:
>  	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  	return error;
> diff --git a/fs/super.c b/fs/super.c
> index b224182f2440..3b39da5334c5 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -1437,7 +1437,7 @@ EXPORT_SYMBOL(sget_dev);
>   *
>   * The function must be called with bdev->bd_holder_lock and releases it.
>   */
> -static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
> +static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
>  	__releases(&bdev->bd_holder_lock)
>  {
>  	struct super_block *sb = bdev->bd_holder;
> @@ -1451,18 +1451,25 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
>  	spin_lock(&sb_lock);
>  	sb->s_count++;
>  	spin_unlock(&sb_lock);
> +
>  	mutex_unlock(&bdev->bd_holder_lock);
>  
> -	locked = super_lock_shared(sb);
> -	if (!locked || !sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> -		put_super(sb);
> -		return NULL;
> -	}
> +	locked = super_lock(sb, excl);
> +
>  	/*
> -	 * The superblock is active and we hold s_umount, we can drop our
> -	 * temporary reference now.
> -	 */
> +	 * If the superblock wasn't already SB_DYING then we hold
> +	 * s_umount and can safely drop our temporary reference.
> +         */
>  	put_super(sb);
> +
> +	if (!locked)
> +		return NULL;
> +
> +	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
> +		super_unlock(sb, excl);
> +		return NULL;
> +	}
> +
>  	return sb;
>  }
>  
> @@ -1470,7 +1477,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
>  {
>  	struct super_block *sb;
>  
> -	sb = bdev_super_lock_shared(bdev);
> +	sb = bdev_super_lock(bdev, false);
>  	if (!sb)
>  		return;
>  
> @@ -1488,16 +1495,74 @@ static void fs_bdev_sync(struct block_device *bdev)
>  {
>  	struct super_block *sb;
>  
> -	sb = bdev_super_lock_shared(bdev);
> +	sb = bdev_super_lock(bdev, false);
>  	if (!sb)
>  		return;
> +
>  	sync_filesystem(sb);
>  	super_unlock_shared(sb);
>  }
>  
> +static struct super_block *get_bdev_super(struct block_device *bdev)
> +{
> +	bool active = false;
> +	struct super_block *sb;
> +
> +	sb = bdev_super_lock(bdev, true);
> +	if (sb) {
> +		active = atomic_inc_not_zero(&sb->s_active);
> +		super_unlock_excl(sb);
> +	}
> +	if (!active)
> +		return NULL;
> +	return sb;
> +}
> +
> +static int fs_bdev_freeze(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error = 0;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	sb = get_bdev_super(bdev);
> +	if (!sb)
> +		return -EINVAL;
> +
> +	if (sb->s_op->freeze_super)
> +		error = sb->s_op->freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = freeze_super(sb, FREEZE_HOLDER_USERSPACE);
> +	if (!error)
> +		error = sync_blockdev(bdev);
> +	deactivate_super(sb);
> +	return error;
> +}
> +
> +static int fs_bdev_thaw(struct block_device *bdev)
> +{
> +	struct super_block *sb;
> +	int error;
> +
> +	lockdep_assert_held(&bdev->bd_fsfreeze_mutex);
> +
> +	sb = get_bdev_super(bdev);
> +	if (WARN_ON_ONCE(!sb))
> +		return -EINVAL;
> +
> +	if (sb->s_op->thaw_super)
> +		error = sb->s_op->thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	else
> +		error = thaw_super(sb, FREEZE_HOLDER_USERSPACE);
> +	deactivate_super(sb);
> +	return error;
> +}
> +
>  const struct blk_holder_ops fs_holder_ops = {
>  	.mark_dead		= fs_bdev_mark_dead,
>  	.sync			= fs_bdev_sync,
> +	.freeze			= fs_bdev_freeze,
> +	.thaw			= fs_bdev_thaw,
>  };
>  EXPORT_SYMBOL_GPL(fs_holder_ops);
>  
> @@ -1527,15 +1592,10 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
>  	}
>  
>  	/*
> -	 * Until SB_BORN flag is set, there can be no active superblock
> -	 * references and thus no filesystem freezing. get_active_super() will
> -	 * just loop waiting for SB_BORN so even bdev_freeze() cannot proceed.
> -	 *
> -	 * It is enough to check bdev was not frozen before we set s_bdev.
> +	 * It is enough to check bdev was not frozen before we set
> +	 * s_bdev as freezing will wait until SB_BORN is set.
>  	 */
> -	mutex_lock(&bdev->bd_fsfreeze_mutex);
> -	if (bdev->bd_fsfreeze_count > 0) {
> -		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +	if (atomic_read(&bdev->bd_fsfreeze_count) > 0) {
>  		if (fc)
>  			warnf(fc, "%pg: Can't mount, blockdev is frozen", bdev);
>  		bdev_release(bdev_handle);
> @@ -1548,7 +1608,6 @@ int setup_bdev_super(struct super_block *sb, int sb_flags,
>  	if (bdev_stable_writes(bdev))
>  		sb->s_iflags |= SB_I_STABLE_WRITES;
>  	spin_unlock(&sb_lock);
> -	mutex_unlock(&bdev->bd_fsfreeze_mutex);
>  
>  	snprintf(sb->s_id, sizeof(sb->s_id), "%pg", bdev);
>  	shrinker_debugfs_rename(&sb->s_shrink, "sb-%s:%s", sb->s_type->name,
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index d5c5e59ddbd2..88e1848b0869 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -57,7 +57,7 @@ struct block_device {
>  	const struct blk_holder_ops *bd_holder_ops;
>  	struct mutex		bd_holder_lock;
>  	/* The counter of freeze processes */
> -	int			bd_fsfreeze_count;
> +	atomic_t		bd_fsfreeze_count;
>  	int			bd_holders;
>  	struct kobject		*bd_holder_dir;
>  
> -- 
> 2.34.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2 00/10] Implement freeze and thaw as holder operations

[Christian Brauner]

On Tue, 24 Oct 2023 15:01:06 +0200, Christian Brauner wrote:
> Hey Christoph,
> Hey Jan,
> Hey Darrick,
> 
> This is v2 and based on vfs.super. I'm sending this out right now
> because frankly, shortly before the merge window is the time when I have
> time to do something. Otherwise, I would've waited a bit.
> 
> [...]

for v6.8

---

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[01/10] fs: massage locking helpers
        https://git.kernel.org/vfs/vfs/c/eebe374c4824
[02/10] bdev: rename freeze and thaw helpers
        https://git.kernel.org/vfs/vfs/c/48ebe79e5e5f
[03/10] bdev: surface the error from sync_blockdev()
        https://git.kernel.org/vfs/vfs/c/7cb1d5f9e92c
[04/10] bdev: add freeze and thaw holder operations
        https://git.kernel.org/vfs/vfs/c/bd05ce12dd8d
[05/10] bdev: implement freeze and thaw holder operations
        https://git.kernel.org/vfs/vfs/c/8246b9ef23c3
[06/10] fs: remove get_active_super()
        https://git.kernel.org/vfs/vfs/c/085b0e223302
[07/10] super: remove bd_fsfreeze_sb
        https://git.kernel.org/vfs/vfs/c/36d253481290
[08/10] fs: remove unused helper
        https://git.kernel.org/vfs/vfs/c/350e08366980
[09/10] porting: document block device freeze and thaw changes
        https://git.kernel.org/vfs/vfs/c/c3e5c6b60a75
[10/10] blkdev: comment fs_holder_ops
        https://git.kernel.org/vfs/vfs/c/15d2068af6f4

Re: [PATCH v2 01/10] fs: massage locking helpers

[Christoph Hellwig]

On Tue, Oct 24, 2023 at 03:01:07PM +0200, Christian Brauner wrote:
> Multiple people have balked at the the fact that
> super_lock{_shared,_excluse}() return booleans and even if they return
> false hold s_umount. So let's change them to only hold s_umount when
> true is returned and change the code accordingly.

With the fix suggested by Jan this looks good an a huge improvement:

Reviewed-by: Christoph Hellwig <hch@lst.de>

That being said, I find the {,__}super_{,un}lock{,_excl} helpers still
confusing as hell.

I'd prefer to remove the __super_lock and super_unlock and wrapping
the loking calls is just horrible confusing, and instead of just
looking at a trivial say:

	down_write(&sb->s_umount)

I now have to chase through two levels on indirections to figure out
what is going on, not helped by the boolean flag that is just
passed as true/false instead of clearly documenting what it does.

Below is what I'd do (on top of the whole series):

 - remove all wrappers
 - rename super_lock to lock_ready_super
 - add an enum telling if it locks exclusive or shared

I could probably live with 2 helpers taking/releasing s_umount
based on the same enum if we really want, but preferable only
for the case that actually handle both case.   But I think just
open coding them is easier to understand.

---
diff --git a/fs/super.c b/fs/super.c
index b26b302f870d24..b8ea32c7cd03e6 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -50,37 +50,6 @@ static char *sb_writers_name[SB_FREEZE_LEVELS] = {
 	"sb_internal",
 };
 
-static inline void __super_lock(struct super_block *sb, bool excl)
-{
-	if (excl)
-		down_write(&sb->s_umount);
-	else
-		down_read(&sb->s_umount);
-}
-
-static inline void super_unlock(struct super_block *sb, bool excl)
-{
-	if (excl)
-		up_write(&sb->s_umount);
-	else
-		up_read(&sb->s_umount);
-}
-
-static inline void __super_lock_excl(struct super_block *sb)
-{
-	__super_lock(sb, true);
-}
-
-static inline void super_unlock_excl(struct super_block *sb)
-{
-	super_unlock(sb, true);
-}
-
-static inline void super_unlock_shared(struct super_block *sb)
-{
-	super_unlock(sb, false);
-}
-
 static inline bool wait_born(struct super_block *sb)
 {
 	unsigned int flags;
@@ -93,10 +62,15 @@ static inline bool wait_born(struct super_block *sb)
 	return flags & (SB_BORN | SB_DYING);
 }
 
+enum lock_super_mode {
+	LOCK_SUPER_SHARED,
+	LOCK_SUPER_EXCL,
+};
+
 /**
- * super_lock - wait for superblock to become ready and lock it
+ * lock_ready_super - wait for superblock to become ready and lock it
  * @sb: superblock to wait for
- * @excl: whether exclusive access is required
+ * @mode: whether exclusive access is required
  *
  * If the superblock has neither passed through vfs_get_tree() or
  * generic_shutdown_super() yet wait for it to happen. Either superblock
@@ -109,29 +83,37 @@ static inline bool wait_born(struct super_block *sb)
  *         s_umount held. The function returns false if SB_DYING was
  *         set and without s_umount held.
  */
-static __must_check bool super_lock(struct super_block *sb, bool excl)
+static __must_check bool lock_ready_super(struct super_block *sb,
+		enum lock_super_mode mode)
 {
+	bool is_dying;
 
 	lockdep_assert_not_held(&sb->s_umount);
 
 relock:
-	__super_lock(sb, excl);
+	if (mode == LOCK_SUPER_EXCL)
+		down_write(&sb->s_umount);
+	else
+		down_read(&sb->s_umount);
+
+	/* Has called ->get_tree() successfully. */
+	if ((sb->s_flags & (SB_BORN | SB_DYING)) == SB_BORN)
+		return true;
 
 	/*
 	 * Has gone through generic_shutdown_super() in the meantime.
 	 * @sb->s_root is NULL and @sb->s_active is 0. No one needs to
 	 * grab a reference to this. Tell them so.
 	 */
-	if (sb->s_flags & SB_DYING) {
-		super_unlock(sb, excl);
-		return false;
-	}
+	is_dying = sb->s_flags & SB_DYING;
 
-	/* Has called ->get_tree() successfully. */
-	if (sb->s_flags & SB_BORN)
-		return true;
+	if (mode == LOCK_SUPER_EXCL)
+		up_write(&sb->s_umount);
+	else
+		up_read(&sb->s_umount);
 
-	super_unlock(sb, excl);
+	if (is_dying)
+		return false;
 
 	/* wait until the superblock is ready or dying */
 	wait_var_event(&sb->s_flags, wait_born(sb));
@@ -143,18 +125,6 @@ static __must_check bool super_lock(struct super_block *sb, bool excl)
 	goto relock;
 }
 
-/* wait and try to acquire read-side of @sb->s_umount */
-static inline bool super_lock_shared(struct super_block *sb)
-{
-	return super_lock(sb, false);
-}
-
-/* wait and try to acquire write-side of @sb->s_umount */
-static inline bool super_lock_excl(struct super_block *sb)
-{
-	return super_lock(sb, true);
-}
-
 /* wake waiters */
 #define SUPER_WAKE_FLAGS (SB_BORN | SB_DYING | SB_DEAD)
 static void super_wake(struct super_block *sb, unsigned int flag)
@@ -163,7 +133,7 @@ static void super_wake(struct super_block *sb, unsigned int flag)
 	WARN_ON_ONCE(hweight32(flag & SUPER_WAKE_FLAGS) > 1);
 
 	/*
-	 * Pairs with smp_load_acquire() in super_lock() to make sure
+	 * Pairs with smp_load_acquire() in lock_ready_super() to make sure
 	 * all initializations in the superblock are seen by the user
 	 * seeing SB_BORN sent.
 	 */
@@ -237,7 +207,7 @@ static unsigned long super_cache_scan(struct shrinker *shrink,
 		freed += sb->s_op->free_cached_objects(sb, sc);
 	}
 
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 	return freed;
 }
 
@@ -303,7 +273,7 @@ static void destroy_unused_super(struct super_block *s)
 {
 	if (!s)
 		return;
-	super_unlock_excl(s);
+	up_write(&s->s_umount);
 	list_lru_destroy(&s->s_dentry_lru);
 	list_lru_destroy(&s->s_inode_lru);
 	security_sb_free(s);
@@ -496,7 +466,7 @@ void deactivate_locked_super(struct super_block *s)
 		put_filesystem(fs);
 		put_super(s);
 	} else {
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 	}
 }
 
@@ -513,7 +483,7 @@ EXPORT_SYMBOL(deactivate_locked_super);
 void deactivate_super(struct super_block *s)
 {
 	if (!atomic_add_unless(&s->s_active, -1, 1)) {
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		deactivate_locked_super(s);
 	}
 }
@@ -550,13 +520,13 @@ static bool grab_super(struct super_block *sb)
 
 	sb->s_count++;
 	spin_unlock(&sb_lock);
-	locked = super_lock_excl(sb);
+	locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 	if (locked) {
 		if (atomic_inc_not_zero(&sb->s_active)) {
 			put_super(sb);
 			return true;
 		}
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 	}
 	wait_var_event(&sb->s_flags, wait_dead(sb));
 	put_super(sb);
@@ -586,7 +556,7 @@ bool super_trylock_shared(struct super_block *sb)
 		if (!(sb->s_flags & SB_DYING) && sb->s_root &&
 		    (sb->s_flags & SB_BORN))
 			return true;
-		super_unlock_shared(sb);
+		up_read(&sb->s_umount);
 	}
 
 	return false;
@@ -611,13 +581,13 @@ bool super_trylock_shared(struct super_block *sb)
 void retire_super(struct super_block *sb)
 {
 	WARN_ON(!sb->s_bdev);
-	__super_lock_excl(sb);
+	down_write(&sb->s_umount);
 	if (sb->s_iflags & SB_I_PERSB_BDI) {
 		bdi_unregister(sb->s_bdi);
 		sb->s_iflags &= ~SB_I_PERSB_BDI;
 	}
 	sb->s_iflags |= SB_I_RETIRED;
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 }
 EXPORT_SYMBOL(retire_super);
 
@@ -699,7 +669,7 @@ void generic_shutdown_super(struct super_block *sb)
 	 * sget{_fc}() until we passed sb->kill_sb().
 	 */
 	super_wake(sb, SB_DYING);
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	if (sb->s_bdi != &noop_backing_dev_info) {
 		if (sb->s_iflags & SB_I_PERSB_BDI)
 			bdi_unregister(sb->s_bdi);
@@ -886,7 +856,7 @@ EXPORT_SYMBOL(sget);
 
 void drop_super(struct super_block *sb)
 {
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 	put_super(sb);
 }
 
@@ -894,7 +864,7 @@ EXPORT_SYMBOL(drop_super);
 
 void drop_super_exclusive(struct super_block *sb)
 {
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	put_super(sb);
 }
 EXPORT_SYMBOL(drop_super_exclusive);
@@ -941,11 +911,11 @@ void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		locked = super_lock_shared(sb);
+		locked = lock_ready_super(sb, LOCK_SUPER_SHARED);
 		if (locked) {
 			if (sb->s_root)
 				f(sb, arg);
-			super_unlock_shared(sb);
+			up_read(&sb->s_umount);
 		}
 
 		spin_lock(&sb_lock);
@@ -979,11 +949,11 @@ void iterate_supers_type(struct file_system_type *type,
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		locked = super_lock_shared(sb);
+		locked = lock_ready_super(sb, LOCK_SUPER_SHARED);
 		if (locked) {
 			if (sb->s_root)
 				f(sb, arg);
-			super_unlock_shared(sb);
+			up_read(&sb->s_umount);
 		}
 
 		spin_lock(&sb_lock);
@@ -1010,11 +980,14 @@ struct super_block *user_get_super(dev_t dev, bool excl)
 			sb->s_count++;
 			spin_unlock(&sb_lock);
 			/* still alive? */
-			locked = super_lock(sb, excl);
+			locked = lock_ready_super(sb, excl);
 			if (locked) {
 				if (sb->s_root)
 					return sb;
-				super_unlock(sb, excl);
+				if (excl)
+					up_write(&sb->s_umount);
+				else
+					up_read(&sb->s_umount);
 			}
 			/* nope, got unmounted */
 			spin_lock(&sb_lock);
@@ -1061,9 +1034,9 @@ int reconfigure_super(struct fs_context *fc)
 
 	if (remount_ro) {
 		if (!hlist_empty(&sb->s_pins)) {
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			group_pin_kill(&sb->s_pins);
-			__super_lock_excl(sb);
+			down_write(&sb->s_umount);
 			if (!sb->s_root)
 				return 0;
 			if (sb->s_writers.frozen != SB_UNFROZEN)
@@ -1126,7 +1099,7 @@ int reconfigure_super(struct fs_context *fc)
 
 static void do_emergency_remount_callback(struct super_block *sb)
 {
-	bool locked = super_lock_excl(sb);
+	bool locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 
 	if (locked && sb->s_root && sb->s_bdev && !sb_rdonly(sb)) {
 		struct fs_context *fc;
@@ -1140,7 +1113,7 @@ static void do_emergency_remount_callback(struct super_block *sb)
 		}
 	}
 	if (locked)
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 }
 
 static void do_emergency_remount(struct work_struct *work)
@@ -1163,7 +1136,7 @@ void emergency_remount(void)
 
 static void do_thaw_all_callback(struct super_block *sb)
 {
-	bool locked = super_lock_excl(sb);
+	bool locked = lock_ready_super(sb, LOCK_SUPER_EXCL);
 
 	if (locked && sb->s_root) {
 		if (IS_ENABLED(CONFIG_BLOCK))
@@ -1173,7 +1146,7 @@ static void do_thaw_all_callback(struct super_block *sb)
 		return;
 	}
 	if (locked)
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 }
 
 static void do_thaw_all(struct work_struct *work)
@@ -1394,7 +1367,7 @@ static struct super_block *bdev_super_lock(struct block_device *bdev, bool excl)
 
 	mutex_unlock(&bdev->bd_holder_lock);
 
-	locked = super_lock(sb, excl);
+	locked = lock_ready_super(sb, excl);
 	put_super(sb);
 	if (!locked)
 		return NULL;
@@ -1418,7 +1391,7 @@ static struct super_block *bdev_super_lock_shared(struct block_device *bdev)
 		return NULL;
 
 	if (!sb->s_root || !(sb->s_flags & SB_ACTIVE)) {
-		super_unlock_shared(sb);
+		up_read(&sb->s_umount);
 		return NULL;
 	}
 
@@ -1440,7 +1413,7 @@ static void fs_bdev_mark_dead(struct block_device *bdev, bool surprise)
 	if (sb->s_op->shutdown)
 		sb->s_op->shutdown(sb);
 
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 }
 
 static void fs_bdev_sync(struct block_device *bdev)
@@ -1451,7 +1424,7 @@ static void fs_bdev_sync(struct block_device *bdev)
 	if (!sb)
 		return;
 	sync_filesystem(sb);
-	super_unlock_shared(sb);
+	up_read(&sb->s_umount);
 }
 
 static struct super_block *get_bdev_super(struct block_device *bdev)
@@ -1462,7 +1435,7 @@ static struct super_block *get_bdev_super(struct block_device *bdev)
 	sb = bdev_super_lock(bdev, true);
 	if (sb) {
 		active = atomic_inc_not_zero(&sb->s_active);
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 	}
 	if (!active)
 		return NULL;
@@ -1619,9 +1592,9 @@ int get_tree_bdev(struct fs_context *fc,
 		 * bdev_mark_dead()). It is safe because we have active sb
 		 * reference and SB_BORN is not set yet.
 		 */
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 		error = setup_bdev_super(s, fc->sb_flags, fc);
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		if (!error)
 			error = fill_super(s, fc);
 		if (error) {
@@ -1671,9 +1644,9 @@ struct dentry *mount_bdev(struct file_system_type *fs_type,
 		 * bdev_mark_dead()). It is safe because we have active sb
 		 * reference and SB_BORN is not set yet.
 		 */
-		super_unlock_excl(s);
+		up_write(&s->s_umount);
 		error = setup_bdev_super(s, flags, NULL);
-		__super_lock_excl(s);
+		down_write(&s->s_umount);
 		if (!error)
 			error = fill_super(s, data, flags & SB_SILENT ? 1 : 0);
 		if (error) {
@@ -1990,7 +1963,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 {
 	int ret;
 
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
@@ -2010,7 +1983,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 		 * freeze and assign the active ref to the freeze.
 		 */
 		sb->s_writers.freeze_holders |= who;
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;
 	}
 
@@ -2025,7 +1998,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	}
 
 	if (!(sb->s_flags & SB_BORN)) {
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;	/* sic - it's "nothing to do" */
 	}
 
@@ -2034,15 +2007,15 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 		sb->s_writers.freeze_holders |= who;
 		sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 		wake_up_var(&sb->s_writers.frozen);
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return 0;
 	}
 
 	sb->s_writers.frozen = SB_FREEZE_WRITE;
 	/* Release s_umount to preserve sb_start_write -> s_umount ordering */
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	sb_wait_write(sb, SB_FREEZE_WRITE);
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while freezing!");
 		return -EINVAL;
 	}
@@ -2085,7 +2058,7 @@ int freeze_super(struct super_block *sb, enum freeze_holder who)
 	sb->s_writers.frozen = SB_FREEZE_COMPLETE;
 	wake_up_var(&sb->s_writers.frozen);
 	lockdep_sb_freeze_release(sb);
-	super_unlock_excl(sb);
+	up_write(&sb->s_umount);
 	return 0;
 }
 EXPORT_SYMBOL(freeze_super);
@@ -2102,7 +2075,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 
 	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
 		if (!(sb->s_writers.freeze_holders & who)) {
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			return -EINVAL;
 		}
 
@@ -2117,7 +2090,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 			return 0;
 		}
 	} else {
-		super_unlock_excl(sb);
+		up_write(&sb->s_umount);
 		return -EINVAL;
 	}
 
@@ -2135,7 +2108,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 		if (error) {
 			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			super_unlock_excl(sb);
+			up_write(&sb->s_umount);
 			return error;
 		}
 	}
@@ -2163,7 +2136,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
  */
 int thaw_super(struct super_block *sb, enum freeze_holder who)
 {
-	if (!super_lock_excl(sb)) {
+	if (!lock_ready_super(sb, LOCK_SUPER_EXCL)) {
 		WARN_ON_ONCE("Dying superblock while thawing!");
 		return -EINVAL;
 	}

Re: [PATCH v2 03/10] bdev: surface the error from sync_blockdev()

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 04/10] bdev: add freeze and thaw holder operations

[Christoph Hellwig]

I still find it weird to add the methods in a separate commit without
the users, but otherwise this still loooks good to me.

Re: [PATCH v2 05/10] bdev: implement freeze and thaw holder operations

[Christoph Hellwig]

On Thu, Oct 26, 2023 at 10:44:27AM +0200, Christian Brauner wrote:
>   */
>  int bdev_freeze(struct block_device *bdev)
>  {
>  	int error = 0;
>  
>  	mutex_lock(&bdev->bd_fsfreeze_mutex);
>  
> +	if (atomic_inc_return(&bdev->bd_fsfreeze_count) > 1) {
> +		mutex_unlock(&bdev->bd_fsfreeze_mutex);
> +		return 0;

Use a goto out_unlock here to share the lock release with the
main flow?

>  	/*
> +	 * If the superblock wasn't already SB_DYING then we hold
> +	 * s_umount and can safely drop our temporary reference.
> +         */

Spaces instead of tabs here.

Otherwise looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v2 08/10] fs: remove unused helper

[Christoph Hellwig]

On Tue, Oct 24, 2023 at 03:01:14PM +0200, Christian Brauner wrote:
> The grab_super() helper is now only used by grab_super_dead(). Merge the
> two helpers into one.

I'd add grab_super to the end of the subject line, without that it is
not very descriptive.

Re: [PATCH v2 00/10] Implement freeze and thaw as holder operations

[Christoph Hellwig]

Btw, while reviewing this I also noticed that thaw_super_locked feels
unreasonably convoluted.  Maybe something like this would be a good
addition for the branch?


---
From f5cbee13dcca6b025c82b365042bc5fab7ac6642 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <hch@lst.de>
Date: Fri, 27 Oct 2023 08:36:04 +0200
Subject: fs: streamline thaw_super_locked

Add a new out_unlock label to share code that just releases s_umount
and returns an error, and rename and reuse the out label that deactivates
the sb for one more case.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/super.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index b26b302f870d24..38381c4b76f09e 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -2098,34 +2098,28 @@ EXPORT_SYMBOL(freeze_super);
  */
 static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 {
-	int error;
+	int error = -EINVAL;
 
-	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
-		if (!(sb->s_writers.freeze_holders & who)) {
-			super_unlock_excl(sb);
-			return -EINVAL;
-		}
+	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
+		goto out_unlock;
+	if (!(sb->s_writers.freeze_holders & who))
+		goto out_unlock;
 
-		/*
-		 * Freeze is shared with someone else.  Release our hold and
-		 * drop the active ref that freeze_super assigned to the
-		 * freezer.
-		 */
-		if (sb->s_writers.freeze_holders & ~who) {
-			sb->s_writers.freeze_holders &= ~who;
-			deactivate_locked_super(sb);
-			return 0;
-		}
-	} else {
-		super_unlock_excl(sb);
-		return -EINVAL;
+	/*
+	 * Freeze is shared with someone else.  Release our hold and drop the
+	 * active ref that freeze_super assigned to the freezer.
+	 */
+	error = 0;
+	if (sb->s_writers.freeze_holders & ~who) {
+		sb->s_writers.freeze_holders &= ~who;
+		goto out_deactivate;
 	}
 
 	if (sb_rdonly(sb)) {
 		sb->s_writers.freeze_holders &= ~who;
 		sb->s_writers.frozen = SB_UNFROZEN;
 		wake_up_var(&sb->s_writers.frozen);
-		goto out;
+		goto out_deactivate;
 	}
 
 	lockdep_sb_freeze_acquire(sb);
@@ -2135,8 +2129,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 		if (error) {
 			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
 			lockdep_sb_freeze_release(sb);
-			super_unlock_excl(sb);
-			return error;
+			goto out_unlock;
 		}
 	}
 
@@ -2144,9 +2137,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
 	sb->s_writers.frozen = SB_UNFROZEN;
 	wake_up_var(&sb->s_writers.frozen);
 	sb_freeze_unlock(sb, SB_FREEZE_FS);
-out:
+out_deactivate:
 	deactivate_locked_super(sb);
 	return 0;
+
+out_unlock:
+	super_unlock_excl(sb);
+	return error;
 }
 
 /**
-- 
2.39.2

Re: [PATCH v2 00/10] Implement freeze and thaw as holder operations

[Jan Kara]

On Fri 27-10-23 08:40:01, Christoph Hellwig wrote:
> Btw, while reviewing this I also noticed that thaw_super_locked feels
> unreasonably convoluted.  Maybe something like this would be a good
> addition for the branch?
> 
> 
> ---
> From f5cbee13dcca6b025c82b365042bc5fab7ac6642 Mon Sep 17 00:00:00 2001
> From: Christoph Hellwig <hch@lst.de>
> Date: Fri, 27 Oct 2023 08:36:04 +0200
> Subject: fs: streamline thaw_super_locked
> 
> Add a new out_unlock label to share code that just releases s_umount
> and returns an error, and rename and reuse the out label that deactivates
> the sb for one more case.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks like a nice simplification. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/super.c | 43 ++++++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 23 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index b26b302f870d24..38381c4b76f09e 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -2098,34 +2098,28 @@ EXPORT_SYMBOL(freeze_super);
>   */
>  static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>  {
> -	int error;
> +	int error = -EINVAL;
>  
> -	if (sb->s_writers.frozen == SB_FREEZE_COMPLETE) {
> -		if (!(sb->s_writers.freeze_holders & who)) {
> -			super_unlock_excl(sb);
> -			return -EINVAL;
> -		}
> +	if (sb->s_writers.frozen != SB_FREEZE_COMPLETE)
> +		goto out_unlock;
> +	if (!(sb->s_writers.freeze_holders & who))
> +		goto out_unlock;
>  
> -		/*
> -		 * Freeze is shared with someone else.  Release our hold and
> -		 * drop the active ref that freeze_super assigned to the
> -		 * freezer.
> -		 */
> -		if (sb->s_writers.freeze_holders & ~who) {
> -			sb->s_writers.freeze_holders &= ~who;
> -			deactivate_locked_super(sb);
> -			return 0;
> -		}
> -	} else {
> -		super_unlock_excl(sb);
> -		return -EINVAL;
> +	/*
> +	 * Freeze is shared with someone else.  Release our hold and drop the
> +	 * active ref that freeze_super assigned to the freezer.
> +	 */
> +	error = 0;
> +	if (sb->s_writers.freeze_holders & ~who) {
> +		sb->s_writers.freeze_holders &= ~who;
> +		goto out_deactivate;
>  	}
>  
>  	if (sb_rdonly(sb)) {
>  		sb->s_writers.freeze_holders &= ~who;
>  		sb->s_writers.frozen = SB_UNFROZEN;
>  		wake_up_var(&sb->s_writers.frozen);
> -		goto out;
> +		goto out_deactivate;
>  	}
>  
>  	lockdep_sb_freeze_acquire(sb);
> @@ -2135,8 +2129,7 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>  		if (error) {
>  			printk(KERN_ERR "VFS:Filesystem thaw failed\n");
>  			lockdep_sb_freeze_release(sb);
> -			super_unlock_excl(sb);
> -			return error;
> +			goto out_unlock;
>  		}
>  	}
>  
> @@ -2144,9 +2137,13 @@ static int thaw_super_locked(struct super_block *sb, enum freeze_holder who)
>  	sb->s_writers.frozen = SB_UNFROZEN;
>  	wake_up_var(&sb->s_writers.frozen);
>  	sb_freeze_unlock(sb, SB_FREEZE_FS);
> -out:
> +out_deactivate:
>  	deactivate_locked_super(sb);
>  	return 0;
> +
> +out_unlock:
> +	super_unlock_excl(sb);
> +	return error;
>  }
>  
>  /**
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] fs: streamline thaw_super_locked

[Christian Brauner]

On Fri, 27 Oct 2023 08:40:01 +0200, Christoph Hellwig wrote:
> Add a new out_unlock label to share code that just releases s_umount
> and returns an error, and rename and reuse the out label that deactivates
> the sb for one more case.
> 
> 

Applied to the vfs.super branch of the vfs/vfs.git tree.
Patches in the vfs.super branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.super

[1/1] fs: streamline thaw_super_locked
      https://git.kernel.org/vfs/vfs/c/6d0acff564f2