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

[Julian Sun <sunjunchao@bytedance.com>]

On 9/11/26 1:47 AM, Jan Kara wrote:
> On Wed 09-09-26 17:01:07, Julian Sun wrote:
>> 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 <sunjunchao@bytedance.com>
> 
> 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/
> 
>> ---
>>   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 <sunjunchao@bytedance.com>