Messages in this thread | | | Date | Fri, 10 Sep 2004 20:56:45 +0400 | From | Kirill Korotaev <> | Subject | Re: [PATCH] adding per sb inode list to make invalidate_inodes() faster |
| |
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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |