linux-kernel - Re: [PATCH 16/18] fs: Reduce inode I

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20101013135130.GE5263@infradead.org>
Date:	Wed, 13 Oct 2010 09:51:30 -0400
From:	Christoph Hellwig <hch@...radead.org>
To:	Dave Chinner <david@...morbit.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/