Re: [RFC PATCH 0/7] vfs: improving inode cache iteration scalability

[Christoph Hellwig <hch@infradead.org>]

On Thu, Oct 03, 2024 at 01:45:55PM +0200, Jan Kara wrote:
> /* Find next inode on the inode list eligible for processing */
> #define sb_inode_iter_next(sb, inode, old_inode, inode_eligible) 	\
> ({									\
> 	struct inode *ret = NULL;					\

<snip>

> 	ret;								\
> })

How is this going to interact with calling into the file system
to do the interaction, which is kinda the point of this series?

Sure, you could have a get_next_inode-style method, but it would need
a fair amount of entirely file system specific state that needs
to be stashed away somewhere, and all options for that looks pretty
ugly.

Also even with your pre-iget callback we'd at best halve the number
of indirect calls for something that isn't exactly performance
critical.  So while it could be done that way, it feels like a
more complex and much harder to reason about version for no real
benefit.

> #define for_each_sb_inode(sb, inode, inode_eligible)			\
> 	for (DEFINE_FREE(old_inode, struct inode *, if (_T) iput(_T)),	\
> 	     inode = NULL;						\
> 	     inode = sb_inode_iter_next((sb), inode, &old_inode,	\
> 					 inode_eligible);		\
> 	    )

And while I liked:

	obj = NULL;

	while ((obj = get_next_object(foo, obj))) {
	}

style iterators, magic for_each macros that do magic cleanup are just
a nightmare to read.  Keep it simple and optimize for someone actually
having to read and understand the code, and not for saving a few lines
of code.