lkml.org 
[lkml]   [2004]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] adding per sb inode list to make invalidate_inodes() faster
Linus Torvalds wrote:
>
> On Thu, 9 Sep 2004, Kirill Korotaev wrote:
>
>>This patch fixes the problem that huge inode cache
>>can make invalidate_inodes() calls very long. Meanwhile
>>lock_kernel() and inode_lock are being held and the system
>>can freeze for seconds.
>
>
> 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.

all the operations I perform on i_sb_list are
- list_add(&inode->i_sb_list, ...);
- list_del(&inode->i_sb_list);

So it's all safe.

> Yes, you do a
> list_add(&inode->i_sb_list, &sb->s_inodes);
>
> in new_inode(), but there are a ton of users that allocate inodes other
> ways, and more importantly, even if this was the only allocation function,
> you do various "list_del(&inode->i_sb_list)" things which leaves the inode
> around but with an invalid superblock list.
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.

2. list_del(&inode->i_sb_list) doesn't leave super block list invalid!

I state that I remove inodes from sb list only and only when usual
inode->i_list is removed and inode can't be found after that moment
neither in my per sb list nor in any other list (unused_inodes,
inodes_in_use, sb->s_io, etc.)

See the details below.

> So at the very _least_, you should document why all of this is safe very
> carefully (I get nervous about fundamental FS infrastructure changes), and
> it should be left to simmer in -mm for a longish time to make sure it
> really works..
Ok. This patch is safe because the use of new inode->i_sb_list list is
fully symmetric to the use of inode->i_list. i.e.

- when inode is created it's added by inode->i_list to one of std lists
(inodes_in_use, unused_inodes, sb->s_io). It lives in one of this lists
during whole lifetime. So in places where inode is created I've just
added list_add(&inode->i_sb_list, &sb->s_inodes). There are 3 such
places: new_inode(), get_new_inode() and get_new_inode_fast()

- when inode is about to be destroyed it's usually removed from std
lists (and sometimes is moved to 'freeable' list). It's the places where
inode is removed from the hash as well. In such places I've just
inserted list_del(&inode->i_sb_list). These places are in
generic_forget_inode(), generic_delete_inode(), invalidate_list(),
prune_icache(), hugetlbfs_delete_inode(), hugetlbfs_forget_inode().

So as you can see from the description the lifetime of inode in
sb->s_inodes list is the same as in hash and other std lists.
And these new per-sb list is protected by the same inode_lock.

To be sure that there are no other places where i_list field is used
somehow in other ways I've just grepped it.

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/

\
 
 \ /
  Last update: 2005-03-22 14:06    [W:0.099 / U:0.276 seconds]
©2003-2014 Jasper Spaans. hosted at Digital Ocean