On Tue 10-01-17 12:10:27, Andreas Dilger wrote:
> On Jan 10, 2017, at 5:27 AM, Jan Kara <jack@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
> Cheers, Andreas
>
>
>
>
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
