svn commit: r335690 - head/sys/kern

[Devin Teske]

> On Jun 28, 2018, at 12:36 AM, Warner Losh <imp at bsdimp.com> wrote:
> 
> 
> 
> On Thu, Jun 28, 2018 at 12:54 AM, Devin Teske <dteske at freebsd.org <mailto:dteske at freebsd.org>> wrote:
> 
>> On Jun 27, 2018, at 7:35 PM, Warner Losh <imp at bsdimp.com <mailto:imp at bsdimp.com>> wrote:
>> 
>> 
>> 
>> On Wed, Jun 27, 2018 at 8:14 PM, Devin Teske <dteske at freebsd.org <mailto:dteske at freebsd.org>> wrote:
>> 
>>> On Jun 27, 2018, at 6:58 PM, Warner Losh <imp at bsdimp.com <mailto:imp at bsdimp.com>> wrote:
>>> 
>>> 
>>> 
>>> On Wed, Jun 27, 2018 at 7:49 PM, Devin Teske <dteske at freebsd.org <mailto:dteske at freebsd.org>> wrote:
>>> 
>>>> On Jun 27, 2018, at 5:59 PM, Warner Losh <imp at bsdimp.com <mailto:imp at bsdimp.com>> wrote:
>>>> 
>>>> 
>>>> 
>>>> On Wed, Jun 27, 2018 at 5:52 PM, Gleb Smirnoff <glebius at freebsd.org <mailto:glebius at freebsd.org>> wrote:
>>>> On Wed, Jun 27, 2018 at 04:11:09AM +0000, Warner Losh wrote:
>>>> W> Author: imp
>>>> W> Date: Wed Jun 27 04:11:09 2018
>>>> W> New Revision: 335690
>>>> W> URL: https://svnweb.freebsd.org/changeset/base/335690 <https://svnweb.freebsd.org/changeset/base/335690>
>>>> W> 
>>>> W> Log:
>>>> W>   Fix devctl generation for core files.
>>>> W>   
>>>> W>   We have a problem with vn_fullpath_global when the file exists. Work
>>>> W>   around it by printing the full path if the core file name starts with /,
>>>> W>   or current working directory followed by the filename if not.
>>>> 
>>>> Is this going to work when a core is dumped not at current working directory,
>>>> but at absolute path? e.g. kern.corefile=/var/log/cores/%N.core
>>>> 
>>>> Yes. That works.
>>>>  
>>>> Looks like the vn_fullpath_global needs to be fixed rather than problem
>>>> workarounded.
>>>> 
>>>> It can't be fixed reliably. FreeBSD does not and cannot map a vnode to a name. The only reason we're able to at all is due to the name cache. And when we recreate a file, we invalidate the name cache. And even if we fixed that, there's no guarantee the name cache won't get flushed before we translate the name.... Linux can do this because it keeps the path name associated with the inode. FreeBSD simply doesn't.
>>>> 
>>> 
>>> They said it couldn't be done, but I personally have done it ...
>>> 
>>> I map vnodes to full paths in dwatch. It's not impossible, just implausibly hard.
>>> 
>>> I derived my formula by reading the C code which was very twisty-turny and rather hard to read at times (and I have been using C since 1998 on everything from AIX and OSF/1 to HP/UX, Cygwin, MinGW, FreeBSD, countless Linux-like things, IRIX, and a God-awful remainder of many others; the vnode code was an adventure to say the least).
>>> 
>>> You're welcome to see how it's done in /usr/libexec/dwatch/vop_create
>>> 
>>> I load up a clause-local variable named "path" with a path constructed from a pointer to a vnode structure argument to VOP_CREATE(9).
>>> 
>>> The D code could easily be rewritten back into C to walk the vnode to completion (compared to the D code which is bounded by depth-limit since DTrace doesn't provide loops; so you have to, as I have done, use a higher-level language wrapper to repeat the code to some desired limit; dwatch defaulting to 64 for directory depth limit).
>>> 
>>> Compared this, say, to vfssnoop.d from Chapter 5 of the DTrace book which utilizes the vfs:namei:lookup: probes. My approach in dwatch is far more accurate, produces full paths, and I've benchmarked it at lower overhead with better results.
>>> 
>>> Maybe some of this can be useful? If not, just ignore me.
>>> 
>>> IMHO, there's no benefit from the crazy hoops than the super simple heuristic I did: if the path starts with / print it. If the path doesn't print cwd / and then the path.
>>> 
>>> I look forward to seeing your conversion of the D to C that works, even when there's namespace cache pressure.
>>> 
>> 
>> I was looking at the output of "dwatch -dX vop_create" (the -d flag to dwatch causes the generated dwatch to be printed instead of executed) and thinking to myself:
>> 
>> I could easily convert this, as I can recognize the flattened loop structure. However, not many people will be able to do it. I wasn't really volunteering, but now I'm curious what other people see when they run "dwatch -dX vop_create". If it's half as bad as I think it is, then I'll gladly do it -- but will have to balance it against work priorities.
>> 
>> We don't have the data you do in vop_create anymore, just the vp at the point in the code where we want to do the reverse translation... But we also have a name that's been filled in from the template and cwd, which gives us almost the same thing as we'd hoped to dig out of the name cache...
>> 
> 
> Given:
> 
> 	struct vnode *vp, const char *fi_name
> 
> Here's a rough (not optimized; unchecked) crack at a C equivalent:
> 
> Thanks, but this code suffers from the same problem that vn_fullpath_global() suffers from: we clear out parts of the cache when we open an existing file with O_CREAT. This one NULL leads to a cascade of failures, as show below.
> 
> 
> 
> 	#include <sys/mount.h>
> 	#include <sys/vnode.h>
> 
> 	#include <limits.h>
> 
> 	int i, max_depth = 64;
> 	char *d_name = NULL;
> 	char *fi_fs = NULL;
> 	char *fi_mount = NULL;
> 	char *cp;
> 	struct namecache *ncp = NULL;
> 	struct mount *mount = NULL;
> 	struct vnode *dvp = NULL;
> 	char *nc[max_depth+1];
> 	char path[PATH_MAX] = __DECONST(char *, "");
> 
> 	if (vp != NULL) {
> 		ncp = vp->v_cache_dst.tqh_first;
> 
> For my case, this is NULL since we clear out the cache when we re-create the file. I confirmed this by adding a printf where I put in my workaround and I get:
> vp->v_cache_dst.tqh_first is 0xfffff80003bd7a10 (first time, no cat.core exists)
> vp->v_cache_dst.tqh_first is 0 (second time, cat.core does exist)
> 
> 		mount = vp->v_mount; /* ptr to vfs we are in */
> 		if (vp->v_cache_dd != NULL)
> 			d_name = vp->v_cache_dd->nc_name;
> 	}
> 	if (mount != NULL) {
> 		fi_fs = mount->mnt_stat.f_fstypename;
> 		if (fi_fs != NULL && *fi_fs != '\0') {
> 			if (!strcmp(fi_fs, "devfs")) ncp = NULL;
> 		} else {
> 			if (fi_name == NULL || *fi_name == '\0') ncp = NULL;
> 		}
> 		fi_mount = mount->mnt_stat.f_mntonname;
> 	} else {
> 		ncp = NULL;
> 	}
> 	for (i = 0; i <= max_depth; i++) {
> 		nc[i] = NULL;
> 	}
> 	if (ncp != NULL) {
> 		/* Reach into vnode of parent of name */
> 		if (ncp->nc_dvp != NULL)
> 			dvp = ncp->nc_dvp->v_cache_dst.tqh_first;
> 		if (dvp != NULL) nc[0] = dvp->nc_name;
> 	}
> 
> since ncp is NULL here, we never set nc[0]
> 
>  
> 	if (nc[0] == NULL || *nc[0] == '\0' || !strcmp(nc[0], "/"))
> 		dvp = NULL;
> 
> so dvp is NULL
>  
> 	/*
> 	 * BEGIN Pathname-depth loop
> 	 */
> 	for (i = 1; i < max_depth; i++) {
> 		if (dvp == NULL) break;
> 
> and we break out of this loop without setting any other nc[] entries.
>  
> 		if (dvp->nc_dvp != NULL)
> 			dvp = dvp->nc_dvp->v_cache_dst.tqh_first;
> 		else
> 			dvp = NULL;
> 		if (dvp != NULL) nc[i] = dvp->nc_name;
> 	}
> 	/*
> 	 * END Pathname-depth loop
> 	 */
> 	if (dvp != NULL) {
> 		if (dvp->nc_dvp != NULL)
> 			dvp = dvp->nc_dvp->v_cache_dst.tqh_first;
> 		else
> 			dvp = NULL;
> 		if (dvp != NULL && dvp->nc_dvp != NULL)
> 			nc[max_depth] = __DECONST(char *, "...");
> 	}
> 	if (fi_mount != NULL) {
> 		/*
> 		 * Join full path
> 		 * NB: Up-to but not including the parent directory (joined below)
> 		 */
> 		strcat(path, fi_mount);
> 		if (strcmp(fi_mount, "/")) strcat(path, "/");
> 		for (i = max_depth; i >= 0; i--) {
> 			char *namei = nc[i];
> 			strcat(path, namei == NULL ? "" : namei);
> 
> which joins a lot of empty strings together
>  
> 			if (namei != NULL) strcat(path, "/");
> 		}
> 		/* Join the parent directory name */
> 		if (d_name != NULL && *d_name != '\0') {
> 			strcat(path, d_name);
> 			strcat(path, "/");
> 		}
> 		/* Join the entry name */
> 		if (fi_name != NULL && *fi_name != '\0')
> 			strcat(path, fi_name);
> 
> since we don't have the fi_name from the VOP_CREATE where we want to do this, we have nothing for here.
>  
> 	}
> 
> So I appreciate the effort here, but I'm not sure that it's useful in this context. vn_fullpath* already can do this sort of thing, but can't in the case where vp->v_cache_dst.tqh_first is NULL.
> 


Thank you so much for the thoughtful response. It means a lot really.
-- 
Cheers,
Devin