Date: Tue, 21 Apr 2015 15:49:59 +0100
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: NeilBrown <neilb@suse.de>, 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: <20150421144959.GR889@ZenIV.linux.org.uk>
References: <20150420181222.GK889@ZenIV.linux.org.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150420181222.GK889@ZenIV.linux.org.uk>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3735
Lines: 75

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);
}

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

Other observations about the Neil's series:
	* the games played in page_follow_link_light/page_put_link are
completely pointless; this
 void page_put_link(struct dentry *dentry, char *link, void *cookie)
 {
-       struct page *page = cookie;
+       struct page *page = (void *)((unsigned long)cookie & ~1UL);

        if (page) {
-               kunmap(page);
+               if (page == cookie)
+                       kunmap(page);
                page_cache_release(page);
        }
 }
is trivially avoided by making sure that inodes with such ->put_link() in
their inode_operations have
	mapping_set_gfp_mask(mapping,
			     mapping_get_gfp_mask(mapping) & ~__GFP_HIGHMEM);
done to them.  And to hell with that kmap/kunmap pair - it's a bad idea to
have kmap held across practically unlimited period, anyway (run into a hung
NFS server while traversing the symlink body, get stuck for minutes until it
times out).  I'll take care of that, it's not much work.
	* 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...  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...

	FWIW, there's a potentially interesting alternative - the thing is,
normal callers of link_path_walk() already know the length of name; both
getname() and getname_kernel() calculate the lengths.  And ->follow_link()
tend to have the lengths available as well (and usually equal to ->i_size of
the inode in question).  So we might consider passing (pointer,length) pairs
to link_path_walk().  In principle, it might simplify things...  Comments?

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/