linux-kernel - Re: [PATCH v2 3/3] vfs: Use per-cpu list for superblock's inode list

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: <20160222210814.GG25832@dastard>
Date:	Tue, 23 Feb 2016 08:08:14 +1100
From:	Dave Chinner <david@...morbit.com>
To:	Jan Kara <jack@...e.cz>
Cc:	Peter Zijlstra <peterz@...radead.org>,
	Waiman Long <Waiman.Long@....com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Jan Kara <jack@...e.com>,
	Jeff Layton <jlayton@...chiereds.net>,
	"J. Bruce Fields" <bfields@...ldses.org>,
	Tejun Heo <tj@...nel.org>,
	Christoph Lameter <cl@...ux-foundation.org>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Ingo Molnar <mingo@...hat.com>,
	Andi Kleen <andi@...stfloor.org>,
	Dave Chinner <dchinner@...hat.com>,
	Scott J Norton <scott.norton@...com>,
	Douglas Hatch <doug.hatch@...com>
Subject: Re: [PATCH v2 3/3] vfs: Use per-cpu list for superblock's inode list

On Mon, Feb 22, 2016 at 02:04:35PM +0100, Jan Kara wrote:
> On Mon 22-02-16 13:12:22, Peter Zijlstra wrote:
> > On Mon, Feb 22, 2016 at 12:54:35PM +0100, Jan Kara wrote:
> > > > Also, I think fsnotify_unmount_inodes() (as per mainline) is missing a
> > > > final iput(need_iput) at the very end, but I could be mistaken, that
> > > > code hurts my brain.
> > > 
> > > I think the code is actually correct since need_iput contains "inode
> > > further in the list than the current inode". Thus we will always go though
> > > another iteration of the loop which will drop the reference. And inode
> > > cannot change state to I_FREEING or I_WILL_FREE because we hold inode
> > > reference. But it is subtle as hell so I agree that code needs rewrite.
> > 
> > So while talking to dchinner, he doubted fsnotify will actually remove
> > inodes from the sb-list, but wasn't sure and too tired to check now.
> > 
> > (I got lost in the fsnotify code real quick and gave up, for I was
> > mostly trying to make a point that we don't need the CPP magic and can
> > do with 'readable' code).
> > 
> > If it doesn't, it doesn't need to do this extra special magic dance and
> > can use the 'normal' iterator pattern used in all the other functions,
> > greatly reducing complexity.
> 
> Yeah, that would be nice. But fsnotify code needs to iterate over all
> inodes, drop sb_list_lock and do some fsnotify magic with the inode which
> is not substantial for our discussion. Now that fsnotify magic may actually
> drop all the remaining inode references so once we drop our reference
> pinning the inode, it can just disappear. We don't want to restart the scan
> for each inode we have to process so that is the reason why we play ugly
> tricks with pinning the next inode in the list.
> 
> But I agree it should be possible to just use list_for_each_entry() instead
> of list_for_each_entry_safe() and keep current inode pinned till the next
> iteration to make it stick in the sb->s_inodes list. That would make the
> iteration more standard. Lightly tested patch attached.

That's exactly what I was thinking. Patch looks ok from aquick
reading of it, but I haven't I've got anything here to test it
at all. Perhaps we need so xfstests coverage of this code....

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com