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

[Linus Torvalds]

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. 

> So it's all safe.

It's not "all safe". You had better explain _why_ it's safe. You do so 
later:

> 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...

		Linus
-
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/