From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751982AbbEDFMF (ORCPT ); Mon, 4 May 2015 01:12:05 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:36223 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750790AbbEDFL5 (ORCPT ); Mon, 4 May 2015 01:11:57 -0400 Date: Mon, 4 May 2015 06:11:29 +0100 From: Al Viro To: NeilBrown Cc: Christoph Hellwig , Linus Torvalds , linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org Subject: Re: [RFC][PATCHSET] non-recursive link_path_walk() and reducing stack footprint Message-ID: <20150504051129.GA3246@ZenIV.linux.org.uk> References: <553668C1.8030707@nod.at> <20150421154504.GT889@ZenIV.linux.org.uk> <20150421212007.GU889@ZenIV.linux.org.uk> <20150422180702.GA15209@ZenIV.linux.org.uk> <20150422201238.GW889@ZenIV.linux.org.uk> <20150422210553.GX889@ZenIV.linux.org.uk> <20150423174544.48ae3122@notabene.brown> <20150423180754.GA889@ZenIV.linux.org.uk> <20150424163534.6eb109eb@notabene.brown> <20150424134203.GC889@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150424134203.GC889@ZenIV.linux.org.uk> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Apr 24, 2015 at 02:42:03PM +0100, Al Viro wrote: > That avoids this spin_lock() on each absolute symlink at the price of extra > 32 bits in struct nameidata. It looks like doing on-demand reallocation > of nd->stack is the right way to go anyway, so the pressure on nameidata size > is going to be weaker and that might be the right way to go... OK, on-demand reallocation is done. What I have right now is * flat MAXSYMLINKS 40, no matter what kind of nesting there might be. * purely iterative link_path_walk(). * no damn nameidata on stack for generic_readlink() * stack footprint of the entire thing independent from the nesting depth, and about on par with "no symlinks at all" case in mainline. * some massage towards RCU follow_link done (in the end of queue), but quite a bit more work remains. What I've got so far is in vfs.git#link_path_walk; I'm not too happy about posting a 70-chunk mailbomb, but it really will need review and testing. It survives xfstests and LTP with no regressions, but it will need serious profiling, etc., along with RTFS. I tried to keep it in reasonably small pieces, but there's a lot of them ;-/ FWIW, I've a bit more reorganization plotted out, but it's not far from where we need to be for RCU follow_link. Some notes: * I don't believe we want to pass flags to ->follow_link() - it's much simpler to give the damn thing NULL for dentry in RCU case. In *all* cases where we might have a change to get the symlink body without blocking we can do that by inode alone. We obviously want to pass dentry and inode separately (and in case of fast symlinks we don't hit the filesystem at all), but that's it - flags isn't needed. * terminate_walk() should do bulk put_link(). So should the failure cases of complete_walk(). _Success_ of complete_walk() should be careful about legitimizing links - it *can* be called with one link on stack, and be followed by access to link body. Yes, really - do_last() in O_CREAT case. * do_last(), lookup_last() and mountpoint_last() ought to have put_link() done when called on non-empty stack (thus turning the loops into something like while ((err = lookup_last(nd)) > 0) { err = trailing_symlink(nd); if (err) break; } _After_ the point where they don't need to look at the last component of name, obviously. * I think we should leave terminate_walk() to callers in failure cases of walk_component() and handle_dots(), as well as get_link(). Makes life simpler in callers, actually. I'll play with that a bit more. * it might make sense to add the second flag to walk_component(), in addition to LOOKUP_FOLLOW, meaning "do put_link() once you are done looking at the name". In principle, it promises simpler logics with unlazy_walk(), but that's one area I'm still not entirely sure about. Will need to experiment a bit... * nd->seq clobbering will need to be dealt with, as discussed upthread. * I _really_ hate your "let's use the LSB of struct page * to tell if we need to kunmap()" approach. It's too damn ugly to live. _And_ it's trivial to avoid - all we need is to have (non-lazy) page_follow_link_light() and page_symlink() to remove __GFP_HIGHMEM from inode->i_mapping before ever asking to allocate pages there. That'll suffice, and it makes sense regardless of RCU work - that kmap/kunmap with potential for minutes in between (while waiting for stuck NFS server in the middle of symlink traversal) is simply wrong.