Re: [PATCH] adding per sb inode list to make invalidate_inodes()faster

[Kirill Korotaev]

Linus Torvalds wrote:
> On Fri, 10 Sep 2004, Kirill Korotaev wrote:

>>>Hmm.. I don't mind the approach per se, but I get very nervous about
>>>the fact that I don't see any initialization of "inode->i_sb_list".
>>
>>inode->i_sb_list is a link list_head, not real list head (real list head
>>is sb->s_inodes and it's initialized). i.e. it doesn't require
>>initialization.
>
> It _does_ require initialization. And no, there is no difference 
between a
> "real" head and a entry "link" in the list. They both need to be
> initialized.
>
>>all the operations I perform on i_sb_list are
>>- list_add(&inode->i_sb_list, ...);
>
> This one is ok without initialzing the entry, since it will do so itself.
>
>>- list_del(&inode->i_sb_list);
>
> This one is NOT ok. If list_del() is ever done on a link entry that 
hasn't
> been initialized, you crash. If "list_del()" is ever done twice on an
> entry, you will crash and/or corrupt memory elsewhere.
We never do list_del twice, nor we do list_del on unitialized inodes!

>>1. struct inode is allocated only in one place!
>>it's alloc_inode(). Next alloc_inode() is static and is called from 3
>>places:
>>new_inode(), get_new_inode() and get_new_inode_fast().
>>
>>All 3 above functions do list_add(&inode->i_sb_list, &sb->s_inodes);
>>i.e. newly allocated inodes are always in super block list.

> Good. _This_ is what I was after.
>
>
>>2. list_del(&inode->i_sb_list) doesn't leave super block list invalid!
>
> No, but it leaves _itself_ invalid. There had better not be anything 
that
> touches it ever after without an initialization. That wasn't obvious...
ok. But now I hope I proved that it's ok?
no one does list_del() twice and no one does list_del() on unitialized 
inodes.

If you want i_sb_list to be really ALWAYS initialized than we can 
replace list_del with list_del_init and insert INIT_LIST_HEAD(i_sb_list) 
in inode_init_once().
But I don't think it's a good idea.
Moreover, I think that list_del_init() and other initialization 
functions of link list_heads in such places usually only hide the 
problems, not solve them.

Kirill

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/