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: Windows password security audit tool. GUI, reports in PDF.

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

Date:	Sat, 9 Oct 2010 00:55:35 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Al Viro <viro@...IV.linux.org.uk>
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 01:10:52PM +0100, Al Viro wrote:
> 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. 

Yeah, I see that now. Thanks for pointing it out.

> Moreover, even for umount-only
> side of things, remember that there's fsnotify as well.

I thought that the fsnotify_unmount_inodes() cleaned everything up
before we called invalidate_list().

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

I think I'll avoid the whole issue right now by not making this
change to invalidate_list()....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com
--
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/