From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx2.suse.de ([195.135.220.15]:40792 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753097AbdAKIaZ (ORCPT ); Wed, 11 Jan 2017 03:30:25 -0500 Date: Wed, 11 Jan 2017 09:30:22 +0100 From: Jan Kara To: Andreas Dilger Cc: Jan Kara , Al Viro , linux-fsdevel@vger.kernel.org Subject: Re: [PATCH] vfs: Remove unnecessary list_for_each_entry_safe() variants Message-ID: <20170111083022.GB16116@quack2.suse.cz> References: <20170110122729.7891-1-jack@suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Tue 10-01-17 12:10:27, Andreas Dilger wrote: > On Jan 10, 2017, at 5:27 AM, Jan Kara 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 > > --- > > 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 SUSE Labs, CR