Messages in this thread | | | Date | Tue, 21 Apr 2015 15:49:59 +0100 | From | Al Viro <> | Subject | Re: [RFC][PATCHSET] non-recursive link_path_walk() and reducing stack footprint |
| |
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.
| |