Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants

[Jan Kara <jack@suse.cz>]

On Tue 10-01-17 12:10:27, Andreas Dilger wrote:
> On Jan 10, 2017, at 5:27 AM, Jan Kara <jack@suse.cz> wrote:
> > 
> > evict_inodes() and invalidate_inodes() use list_for_each_entry_safe()
> > to iterate sb->s_inodes list. However, since we use i_lru list entry for
> > our local temporary list of inodes to destroy, the inode is guaranteed
> > to stay in sb->s_inodes list while we hold sb->s_inode_list_lock. So
> > there is no real need for safe iteration variant and we can use
> > list_for_each_entry() just fine.
> 
> This is a pretty "subtle" change, IMHO, with little benefit.  IMHO, using
> the "_safe" variant makes it more clear to the reader that the inode is
> being deleted from the list.  At a minimum, I'd think there should be a
> comment at list_for_each_entry() to the effect that the inode is not going
> to be deleted, so list_for_each_entry_safe() is not needed.  Otherwise, if
> the inode lifetime changes in some manner in the future this may introduce
> hard-to-find corruption of freed memory.

Well, but the inode is not deleted from the list we iterate (and that is
pretty obvious from the loop body) so using a _safe variant looks just
confusing to me... I can add a comment if people think that will help
readability of the code.

								Honza

> 
> Cheers, Andreas
> 
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> > fs/inode.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/inode.c b/fs/inode.c
> > index 88110fd0b282..bd5a47ff2f03 100644
> > --- a/fs/inode.c
> > +++ b/fs/inode.c
> > @@ -601,12 +601,12 @@ static void dispose_list(struct list_head *head)
> >  */
> > void evict_inodes(struct super_block *sb)
> > {
> > -	struct inode *inode, *next;
> > +	struct inode *inode;
> > 	LIST_HEAD(dispose);
> > 
> > again:
> > 	spin_lock(&sb->s_inode_list_lock);
> > -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> > +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> > 		if (atomic_read(&inode->i_count))
> > 			continue;
> > 
> > @@ -651,11 +651,11 @@ void evict_inodes(struct super_block *sb)
> > int invalidate_inodes(struct super_block *sb, bool kill_dirty)
> > {
> > 	int busy = 0;
> > -	struct inode *inode, *next;
> > +	struct inode *inode;
> > 	LIST_HEAD(dispose);
> > 
> > 	spin_lock(&sb->s_inode_list_lock);
> > -	list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> > +	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> > 		spin_lock(&inode->i_lock);
> > 		if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
> > 			spin_unlock(&inode->i_lock);
> > --
> > 2.10.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> Cheers, Andreas
> 
> 
> 
> 
> 


-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR