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

From: Christoph Hellwig <hch@infradead.org>
To: Al Viro <viro@ZenIV.linux.org.uk>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	NeilBrown <neilb@suse.de>,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	Richard Weinberger <richard@nod.at>
Subject: Re: [RFC][PATCHSET] non-recursive link_path_walk() and reducing stack footprint
Date: Tue, 21 Apr 2015 08:04:08 -0700	[thread overview]
Message-ID: <20150421150408.GA29838@infradead.org> (raw)
In-Reply-To: <20150421144959.GR889@ZenIV.linux.org.uk>

On Tue, Apr 21, 2015 at 03:49:59PM +0100, Al Viro wrote:
> On Mon, Apr 20, 2015 at 07:12:22PM +0100, Al Viro wrote:
> 
> > 	Patches 2/24..6/24 are from Neil's RCU follow_link patchset; the
> > rest of his patchset is, of course, derailed by the massage done here,
> > but AFAICS we should be able to port it on top of this one with reasonably
> > little PITA.
> 
> BTW, looking at the ->put_link() instances in the tree, after this series
> all but one of them ignore *everything* other than cookie.  The only exception
> is hppfs; it wants dentry (and its inode as well):
> 
> static void hppfs_put_link(struct dentry *dentry, void *cookie)
> {
>         struct dentry *proc_dentry = HPPFS_I(d_inode(dentry))->proc_dentry;
> 
>         if (d_inode(proc_dentry)->i_op->put_link)
>                 d_inode(proc_dentry)->i_op->put_link(proc_dentry, cookie);
> }

The hppfs code looks totally bogus in general.  Richard, do you know if
anyone still uses that part of UML?

> Does anybody have objections against passing them inodes instead of dentries?
> It would be a lot more convenient for RCU purposes...

Sounds fine to me.

> 	* XFS: is there any reason why you guys don't separate inline and
> non-inline symlinks?  Or don't just use page_follow_link_light() for the
> latter...

Historical reasons, feel free to split it up.

> Note, BTW, that NUL-termination for the latter will be taken
> care of by page_getlink().  It's inline ones that look like crap, actually -
> unless I'm misreading that code, the longest possible inline symlink will
> have ip->i_df.if_u1.if_data completely filled, with no place for terminating
> NUL.  Is that correct?  If so, it might make sense to have three variants -
> short (== inline shorter than XFS_IFORK_DSIZE) just having ->follow_link()
> return ip->i_df.if_u1.if_data and have xfs_setup_inode() do nd_terminate_link()
> to make sure they are NUL-terminated, long (non-inline) just using
> page_follow_link_light() and bogus (inline with length exactly equal to
> XFS_IFORK_DSIZE); the last would be the only ones with XFS-specific non-trivial
> ->follow_link() - those really need to allocate a buffer and copy into it...

In theory we could allocate if_data to include th terminator in memory.
I'd have to see how ugly that would be.

> 	* logfs has ->follow_link equal to page_follow_link_light and
> NULL ->put_link.  Obvious leak, and that one's -stable fodder - it had
> been there all way back to original merge.  I'll send a fix in a minute.

logfs has been unmaintained since 2011, we might as well drop it..