Re: [External] Re: [PATCH 2/7] fs: introduce sb_for_each_inodes().

On 9/11/26 1:47 AM, Jan Kara wrote:

On Wed 09-09-26 17:01:07, Julian Sun wrote:

quoted

Add sb_for_each_inodes() to share s_inodes traversal and preserve its
position while s_inode_list_lock is dropped.

Track active iterators on sb->s_inodes_iters and advance their saved
positions before unlinking an inode. Callbacks manage inode references
and per-inode work, allowing both normal walks and eviction to use the
same interface.

Signed-off-by: Julian Sun <redacted>

So I'm not generally opposed the to a superblock inode iterator idea but
what you have looks more complex than I'd expect. Also we shouldn't tie
that to the fix of the lockup in evict_inodes(). So first I'd just
concentrate on fixing that lockup, then we can have a look at the iterator
idea.

You can take some inspiration about inode iteration API from the Dave
Chinner's patch set [1]. That looked more like what I'd expect although
Christoph had some comments on it too.

Regarding the fix I'd just make evict_inodes() do what all other inode
iterators do when they decide to drop s_inode_list_lock - pin an inode in
the list by grabbing refcount and then resume from it.

Thanks for the feedback. I agree that we should fix the lockup separately,
and I'll focus on that first.

I had seen Dave Chinner's series before. I'll revisit it and the review
comments when working on the inode iteration API.>

								Honza

[1] https://lore.kernel.org/all/20241002014017.3801899-1-david@fromorbit.com/ (local)

quoted

---
  fs/inode.c                     | 93 ++++++++++++++++++++++++++++++++++
  fs/super.c                     |  1 +
  include/linux/fs.h             | 15 ++++++
  include/linux/fs/super_types.h |  3 +-
  4 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/fs/inode.c b/fs/inode.c
index ba7da39be4a3..b4279063a5dd 100644
--- a/fs/inode.c
+++ b/fs/inode.c

@@ -69,6 +69,15 @@ const struct address_space_operations empty_aops = {
  };
  EXPORT_SYMBOL(empty_aops);
  
+struct inode_iter {
+	struct list_head	iters_node;	/* sb->s_inodes_iters */
+	struct list_head	*next;		/* next node going to iterate */
+	unsigned int flags;
+	inode_iter_cb func;
+	void *data;
+	int ret;
+};
+
  static DEFINE_PER_CPU(unsigned long, nr_inodes);
  static DEFINE_PER_CPU(unsigned long, nr_unused);
  

@@ -641,12 +650,96 @@ void inode_sb_list_add(struct inode *inode)
  }
  EXPORT_SYMBOL_GPL(inode_sb_list_add);
  
+static void inode_sb_iter_start(struct super_block *sb, struct inode_iter *it,
+				unsigned int flags, inode_iter_cb fn, void *data)
+{
+	it->flags = flags;
+	it->func = fn;
+	it->data = data;
+	it->ret = 0;
+	spin_lock(&sb->s_inode_list_lock);
+	it->next = sb->s_inodes.next;
+	list_add(&it->iters_node, &sb->s_inodes_iters);
+}
+
+static void inode_sb_iter_end(struct inode_iter *it, struct super_block *sb)
+{
+	list_del(&it->iters_node);
+	spin_unlock(&sb->s_inode_list_lock);
+}
+
+static bool inode_sb_iter_next(struct inode_iter *it, struct super_block *sb)
+{
+	struct inode *inode = NULL;
+	int ret;
+
+	while (!inode && it->next != &sb->s_inodes) {
+		inode = list_entry(it->next, struct inode, i_sb_list);
+		if (it->flags & INODE_ITER_UNUSED) {
+			if (icount_read_once(inode)) {
+				it->next = it->next->next;
+				continue;
+			}
+
+			spin_lock(&inode->i_lock);
+			if (icount_read(inode)) {
+				spin_unlock(&inode->i_lock);
+				it->next = it->next->next;
+				continue;
+			}
+		} else {
+			spin_lock(&inode->i_lock);
+		}
+
+		if ((it->flags & INODE_ITER_NORMAL) &&
+		    (inode_state_read(inode) & (I_NEW | I_FREEING | I_WILL_FREE))) {
+			spin_unlock(&inode->i_lock);
+			it->next = it->next->next;
+			continue;
+		}
+
+		it->next = it->next->next;
+		ret = it->func(inode, it->data);
+		if (ret) {
+			it->ret = ret;
+			return false;
+		}
+
+		if (need_resched()) {
+			spin_unlock(&sb->s_inode_list_lock);
+			cond_resched();
+			spin_lock(&sb->s_inode_list_lock);
+		}
+	}
+
+	return it->next == &sb->s_inodes ? false : true;
+}
+
+int sb_for_each_inodes(struct super_block *sb, unsigned int flags,
+		       inode_iter_cb fn, void *data)
+{
+	struct inode_iter it;
+
+	inode_sb_iter_start(sb, &it, flags, fn, data);
+	while (inode_sb_iter_next(&it, sb))
+		;
+	inode_sb_iter_end(&it, sb);
+
+	return it.ret;
+}
+EXPORT_SYMBOL(sb_for_each_inodes);
+
  static inline void inode_sb_list_del(struct inode *inode)
  {
  	struct super_block *sb = inode->i_sb;
+	struct inode_iter *it;
  
  	if (!list_empty(&inode->i_sb_list)) {
  		spin_lock(&sb->s_inode_list_lock);
+		list_for_each_entry(it, &sb->s_inodes_iters, iters_node) {
+			if (it->next == &inode->i_sb_list)
+				it->next = inode->i_sb_list.next;
+		}
  		list_del_init(&inode->i_sb_list);
  		spin_unlock(&sb->s_inode_list_lock);
  	}

diff --git a/fs/super.c b/fs/super.c
index 05e443173038..3e069150c544 100644
--- a/fs/super.c
+++ b/fs/super.c

@@ -382,6 +382,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags,
  	spin_lock_init(&s->s_roots_lock);
  	mutex_init(&s->s_sync_lock);
  	INIT_LIST_HEAD(&s->s_inodes);
+	INIT_LIST_HEAD(&s->s_inodes_iters);
  	spin_lock_init(&s->s_inode_list_lock);
  	INIT_LIST_HEAD(&s->s_inodes_wb);
  	spin_lock_init(&s->s_inode_wblist_lock);

diff --git a/include/linux/fs.h b/include/linux/fs.h
index 09c4db5e9ae0..f3176ab10e65 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h

@@ -870,6 +870,21 @@ struct inode {
  	void			*i_private; /* fs or device private pointer */
  } __randomize_layout;
  
+enum inode_iter_flags_enum {
+	INODE_ITER_NORMAL = (1U << 1),  /* Exclude inodes with (I_NEW | I_FREEING | I_WILL_FREE). */
+	INODE_ITER_UNUSED = (1U << 2),  /* Only return inodes with (i_count == 0). */
+};
+
+/*
+ *                             start          end
+ *  inode->i_lock              locked         unlocked
+ *  sb->s_inode_list_lock      locked         locked
+ */
+typedef int (*inode_iter_cb) (struct inode *, void *);
+
+int sb_for_each_inodes(struct super_block *sb, unsigned int flags,
+		       inode_iter_cb fn, void *data);
+
  /*
   * i_state handling
   *

diff --git a/include/linux/fs/super_types.h b/include/linux/fs/super_types.h
index ecd96aeb1cee..1f81cc219b8e 100644
--- a/include/linux/fs/super_types.h
+++ b/include/linux/fs/super_types.h

@@ -269,9 +269,10 @@ struct super_block {
  	 */
  	int s_stack_depth;
  
-	/* s_inode_list_lock protects s_inodes */
+	/* s_inode_list_lock protects s_inodes and s_inodes_iters */
  	spinlock_t				s_inode_list_lock ____cacheline_aligned_in_smp;
  	struct list_head			s_inodes;	/* all inodes */
+	struct list_head			s_inodes_iters; /* all iterators */
  
  	spinlock_t				s_inode_wblist_lock;
  	struct list_head			s_inodes_wb;	/* writeback inodes */

-- 
2.39.5

Thanks,
-- 
Julian Sun [off-list ref]