[5/6] super: use common iterator (Part 2)

Commit Message

Christian Brauner March 28, 2025, 4:15 p.m. UTC

Use a common iterator for all callbacks. We could go for something even
more elaborate (advance step-by-step similar to iov_iter) but I really
don't think this is warranted.

Signed-off-by: Christian Brauner <[e-mail]>
---
 fs/super.c         | 76 +++++++++++++++++++++++++++++++++++++++++++++---------
 include/linux/fs.h |  6 +----
 2 files changed, 65 insertions(+), 17 deletions(-)

Comments

James Bottomley March 28, 2025, 6:58 p.m. UTC | #1

On Fri, 2025-03-28 at 17:15 +0100, Christian Brauner wrote:
[...]
> +static inline void super_cb_grabbed(struct super_block *sb,
> +				    void (*f)(struct super_block *,
> void *),
> +				    void *arg)
> +{
> +	if (super_lock_excl(sb)) {
> +		bool active = atomic_inc_not_zero(&sb->s_active);
> +		super_unlock_excl(sb);
> +		if (active)
> +			f(sb, arg);
> +		deactivate_super(sb);

I don't think this can be right: if we fail to increment s_active
because it's zero, we shouldn't call deactivate_super(), should we?

Regards,

James

Christian Brauner March 29, 2025, 7:34 a.m. UTC | #2

On Fri, Mar 28, 2025 at 02:58:29PM -0400, James Bottomley wrote:
> On Fri, 2025-03-28 at 17:15 +0100, Christian Brauner wrote:
> [...]
> > +static inline void super_cb_grabbed(struct super_block *sb,
> > +				    void (*f)(struct super_block *,
> > void *),
> > +				    void *arg)
> > +{
> > +	if (super_lock_excl(sb)) {
> > +		bool active = atomic_inc_not_zero(&sb->s_active);
> > +		super_unlock_excl(sb);
> > +		if (active)
> > +			f(sb, arg);
> > +		deactivate_super(sb);
> 
> I don't think this can be right: if we fail to increment s_active
> because it's zero, we shouldn't call deactivate_super(), should we?

Fixed in-tree. Thanks.

Patch

diff --git a/fs/super.c b/fs/super.c
index 0dd208804a74..58c95210e66c 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -887,24 +887,71 @@  void drop_super_exclusive(struct super_block *sb)
 }
 EXPORT_SYMBOL(drop_super_exclusive);
 
-void __iterate_supers(void (*f)(struct super_block *, void *), void *arg, bool excl)
+enum super_iter_flags_t {
+	SUPER_ITER_EXCL		= (1U << 0),
+	SUPER_ITER_GRAB		= (1U << 1) | SUPER_ITER_EXCL,
+	SUPER_ITER_REVERSE	= (1U << 2),
+};
+
+static inline struct super_block *first_super(enum super_iter_flags_t flags)
+{
+	if (flags & SUPER_ITER_REVERSE)
+		return list_last_entry(&super_blocks, struct super_block, s_list);
+	return list_first_entry(&super_blocks, struct super_block, s_list);
+}
+
+static inline struct super_block *next_super(struct super_block *sb,
+					     enum super_iter_flags_t flags)
+{
+	if (flags & SUPER_ITER_REVERSE)
+		return list_prev_entry(sb, s_list);
+	return list_next_entry(sb, s_list);
+}
+
+static inline void super_cb_locked(struct super_block *sb,
+				   void (*f)(struct super_block *, void *),
+				   void *arg, bool excl)
+{
+        if (super_lock(sb, excl)) {
+                f(sb, arg);
+                super_unlock(sb, excl);
+        }
+}
+
+static inline void super_cb_grabbed(struct super_block *sb,
+				    void (*f)(struct super_block *, void *),
+				    void *arg)
+{
+	if (super_lock_excl(sb)) {
+		bool active = atomic_inc_not_zero(&sb->s_active);
+		super_unlock_excl(sb);
+		if (active)
+			f(sb, arg);
+		deactivate_super(sb);
+	}
+}
+
+#define invalid_super list_entry_is_head
+
+static void __iterate_supers(void (*f)(struct super_block *, void *), void *arg,
+			     enum super_iter_flags_t flags)
 {
 	struct super_block *sb, *p = NULL;
+	bool excl = flags & SUPER_ITER_EXCL;
 
-	spin_lock(&sb_lock);
-	list_for_each_entry(sb, &super_blocks, s_list) {
-		bool locked;
+	guard(spinlock)(&sb_lock);
 
+	for (sb = first_super(flags); !invalid_super(sb, &super_blocks, s_list);
+	     sb = next_super(sb, flags)) {
 		if (super_flags(sb, SB_DYING))
 			continue;
 		sb->s_count++;
 		spin_unlock(&sb_lock);
 
-		locked = super_lock(sb, excl);
-		if (locked) {
-			f(sb, arg);
-			super_unlock(sb, excl);
-		}
+                if (flags & SUPER_ITER_GRAB)
+                        super_cb_grabbed(sb, f, arg);
+                else
+                        super_cb_locked(sb, f, arg, excl);
 
 		spin_lock(&sb_lock);
 		if (p)
@@ -913,7 +960,11 @@  void __iterate_supers(void (*f)(struct super_block *, void *), void *arg, bool e
 	}
 	if (p)
 		__put_super(p);
-	spin_unlock(&sb_lock);
+}
+
+void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
+{
+	__iterate_supers(f, arg, 0);
 }
 
 /**
@@ -1097,7 +1148,8 @@  static void do_emergency_remount_callback(struct super_block *sb, void *unused)
 
 static void do_emergency_remount(struct work_struct *work)
 {
-	__iterate_supers(do_emergency_remount_callback, NULL, true);
+	__iterate_supers(do_emergency_remount_callback, NULL,
+			 SUPER_ITER_EXCL | SUPER_ITER_REVERSE);
 	kfree(work);
 	printk("Emergency Remount complete\n");
 }
@@ -1124,7 +1176,7 @@  static void do_thaw_all_callback(struct super_block *sb, void *unused)
 
 static void do_thaw_all(struct work_struct *work)
 {
-	__iterate_supers(do_thaw_all_callback, NULL, true);
+	__iterate_supers(do_thaw_all_callback, NULL, SUPER_ITER_EXCL);
 	kfree(work);
 	printk(KERN_WARNING "Emergency Thaw complete\n");
 }
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0351500b71d2..c475fa874055 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3515,11 +3515,7 @@  extern void put_filesystem(struct file_system_type *fs);
 extern struct file_system_type *get_fs_type(const char *name);
 extern void drop_super(struct super_block *sb);
 extern void drop_super_exclusive(struct super_block *sb);
-void __iterate_supers(void (*f)(struct super_block *, void *), void *arg, bool excl);
-static inline void iterate_supers(void (*f)(struct super_block *, void *), void *arg)
-{
-	__iterate_supers(f, arg, false);
-}
+extern void iterate_supers(void (*f)(struct super_block *, void *), void *arg);
 extern void iterate_supers_type(struct file_system_type *,
 			        void (*)(struct super_block *, void *), void *);