On Thu 03-10-24 05:18:30, Christoph Hellwig wrote:
> 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?
Yeah, I was concentrated on the VFS bits and forgot why Dave wrote this
series in the first place. So this style of iterator isn't useful for what
Dave wants to achieve. Sorry for the noise. Still the possibility to have a
callback under inode->i_lock being able to do stuff and decide whether we
should grab a reference or continue would be useful (and would allow us to
combine the three iterations on unmount into one without too much hassle).
> > #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.
Well, I agree the above is hard to read but I don't know how to write it in
a more readable way while keeping the properties of the iterator (like
auto-cleanup when you break out of the loop - which is IMO a must for a
sane iterator). Anyway, this is now mostly academic since I agree this
iterator isn't really useful for the situation here.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
