Re: [RFC][PATCHSET] non-recursive link_path_walk() and reducing stack footprint

[NeilBrown]

On Mon, 4 May 2015 06:11:29 +0100 Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:

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


Thanks!
I'll have another look and see about adding what is needed for RCU symlink
support.

NeilBrown

Attachment: pgp1yntxn9Wqn.pgp
Description: OpenPGP digital signature