linux-kernel - Re: [PATCH 18/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 for Android: free password hash cracker in your pocket

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

Message-ID: <20101008121052.GE19804@ZenIV.linux.org.uk>
Date:	Fri, 8 Oct 2010 13:10:52 +0100
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Dave Chinner <david@...morbit.com>
Cc:	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 18/18] fs: Reduce inode I_FREEING and factor inode
 disposal

On Fri, Oct 08, 2010 at 09:52:49PM +1100, Dave Chinner wrote:
> On Fri, Oct 08, 2010 at 11:18:19AM +0100, Al Viro wrote:
> > On Fri, Oct 08, 2010 at 04:21:32PM +1100, Dave Chinner wrote:
> > 
> > > +			spin_unlock(&sb->s_inodes_lock);
> > >  
> > > -			spin_lock(&inode_lru_lock);
> > > -			list_move(&inode->i_lru, dispose);
> > > -			spin_unlock(&inode_lru_lock);
> > > +			dispose_one_inode(inode);
> > >  
> > > -			percpu_counter_dec(&nr_inodes_unused);
> > > +			spin_lock(&sb->s_inodes_lock);
> > 
> > And now you've unlocked the list and even blocked.  What's going to
> > keep next valid through that fun?
> 
> See the comment at the start of the loop in invalidate_list():
> 
>                 /*
>                  * We can reschedule here without worrying about the list's
>                  * consistency because the per-sb list of inodes must not
>                  * change during umount anymore, and because iprune_sem keeps
>                  * shrink_icache_memory() away.
>                  */
> 		cond_resched_lock(&sb->s_inodes_lock);
> 
> Hence I've assumed it's ok to add another point that drops locks and blocks
> inside the loop and next will still be valid.

I'm not convinced, TBH; IOW, the original might have been broken by that.
The trouble is, this function is called not only on umount().  Block device
invalidation paths also can lead to it.  Moreover, even for umount-only
side of things, remember that there's fsnotify as well.  Original code
did _everything_ except the actual dropping inodes without releasing
inode_lock.  I'm not saying that change is broken (or, in case of
non-umount paths, makes breakage worse), but I'd like to see more analysis
of that area.

Umount races that hit only when you have the right subset of inodes with
idiotify watches on those are really not fun to debug post-factum...


> > > +		spin_unlock(&inode_lru_lock);
> > > +
> > > +		dispose_one_inode(inode);
> > > +		cond_resched();
> > > +
> > > +		spin_lock(&inode_lru_lock);
> > 
> > Same, only worse - in the previous you might hope for lack of activity
> > on fs, in this one you really can't.
> 
> That one in prune_icache() is safe because the loop always gets the
> first inod eon the list:
> 
> 	for (nr_scanned = 0; nr_scanned < nr_to_scan; nr_scanned++) {
> 		struct inode *inode;
> 
> 		if (list_empty(&inode_lru))
> 			break;
> 
> 		inode = list_entry(inode_lru.prev, struct inode, i_lru);
> 		.....

D'oh.  OK, that one looks all right.
--
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/