lkml.org 
[lkml]   [2010]   [Oct]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 16/18] fs: Reduce inode I_FREEING and factor inode disposal
>  /*
> * Locking rules.
> *
> + * inode->i_lock is *always* the innermost lock.
> + *

shouldn't this be added in an earlier patch?

> @@ -48,8 +50,15 @@
> *
> * sb inode lock
> * inode_lru_lock
> - * wb->b_lock
> - * inode->i_lock
> + * wb->b_lock
> + * inode->i_lock
> + *
> + * wb->b_lock
> + * sb_lock (pin sb for writeback)
> + * inode->i_lock
> + *
> + * inode_lru
> + * inode->i_lock

This doesn't seem to be new in this patch either. Maybe just have
a separate patch to introduce the lock order protection comment in
it's final form instead of the various updates?

> - int busy;
> LIST_HEAD(throw_away);
> + int busy;
>
> down_write(&iprune_sem);
> spin_lock(&sb->s_inodes_lock);
> fsnotify_unmount_inodes(&sb->s_inodes);
> busy = invalidate_list(sb, &sb->s_inodes, &throw_away);
> spin_unlock(&sb->s_inodes_lock);
> + up_write(&iprune_sem);
>
> dispose_list(&throw_away);
> - up_write(&iprune_sem);

I first though this was unsafe. But in the end the lock doesn't
actually need to protect anything here. If we're getting here
from generic_shutdown_super the filesystem is dead already and
thus other calls to invalidate_inodes which need a reference to
the superblock won't arrive here. prune_icache could arrive
here, but I_FREEING will make it skip the inode. So it looks
like the shorter hold time is fine. In fact just cycling through
iprune_sem here would probably be enough.

Even better would be getting rid of the gem by simply doing
per-superblock inode LRUs which require to have a reference on
the superblock and thus avoid reclaim reacing with unmount.
Time to ressurect your patch for it once the lock split up is done.

Otherwise looks good to me.


\
 
 \ /
  Last update: 2010-10-13 15:53    [from the cache]
©2003-2011 Jasper Spaans