Re: [PATCH 16/18] fs: Reduce inode I_FREEING and factor inodedisposal

[Christoph Hellwig]

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