From owner-svn-src-all@freebsd.org Thu Jun 28 07:36:09 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 4C850102252F for ; Thu, 28 Jun 2018 07:36:09 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io0-x234.google.com (mail-io0-x234.google.com [IPv6:2607:f8b0:4001:c06::234]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 7A0A68EE71 for ; Thu, 28 Jun 2018 07:36:08 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io0-x234.google.com with SMTP id g7-v6so4290580ioh.11 for ; Thu, 28 Jun 2018 00:36:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=7wrVmBoag/CYQnyBQ3a3Lrg9/2R9kv/urElyCzrU/cY=; b=did1aPdoDNL0ypFgNO9C/Wog5dfMvZPTSzWLSWrEsDqjj8Qx0SF2A7TuHf4VU4SHGc r5GFiCQwUuFAcuhKJhj6gg9pQn5HvKUUPOQIHF+dW0wA/JTd8oo6ctrjyTVEbTPc+FYa FHGtUGtNOzsCBtpho64pl86fWMwJcmJ3sFcCeE8+Qv1wbjFR/MDLVF4QKu8MY/fB0Dbz 6GgxuZS+x5r0wHBR/8PmsNM0fZI3yqj/CbpDoSXiMpB5oeTcejKEFVQwWM6/1Aq9t6Fp BE8EjBfxLE64G7ZnBsIbVKAkWxn199FXmOeMaEMzB/OimoTjhoDUQQaPonPbHCFscEYD j1eA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=7wrVmBoag/CYQnyBQ3a3Lrg9/2R9kv/urElyCzrU/cY=; b=q7vBX+EeROXfHFJe2LBE/pXPNePptcF0hGZvQ5KXiE1FYYbx7QiTfTQF+T5F9JToGh serNsqpR/q4p1GdPJOhTHXIY9EBp8eFIENYzUq4Db9nfWUH4/YkWTPsiX4SFVqXfMFXt jwrY2FmBAOyjiQfcMPEl68awqXZ/VPBur4CNk1K4F1N3FafOwKBnejm/+Dzo+gBmIYmH jOUbtWiJQFLwYiiP6ibtNulMrSNTGljV8TPPGhJrfRGn9oPOV1+VRTh8yHUOqaa2oxcl Z8tYt/UYGnfh3MreHNz+I9erp6EnUsUwAZ9tZ6Pk/p8tVoviXXOc/3KFiuIFzmwTltVc A7zQ== X-Gm-Message-State: APt69E0nEF+j9d23Bnb+mAvZdL1CWmTpdwp3PWV7jI/AMImK5y/mf/IU bZx42gtPPVeFlSX5VBCfd50ZA7LOBncYnIjy1IS6lw== X-Google-Smtp-Source: AAOMgpcY7KXiR2zpix1BWq+JN5G8o5dvIzHz6fnAalCpSv/esOcF2Mk527eR+Vqu0B8FNFwliXZvxY4TWbvr7VKYaDE= X-Received: by 2002:a6b:29c4:: with SMTP id p187-v6mr7423286iop.299.1530171367672; Thu, 28 Jun 2018 00:36:07 -0700 (PDT) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 2002:a4f:5945:0:0:0:0:0 with HTTP; Thu, 28 Jun 2018 00:36:07 -0700 (PDT) X-Originating-IP: [2603:300b:6:5100:1052:acc7:f9de:2b6d] In-Reply-To: <7C4AEADC-1973-482B-A7A5-75F23397D44E@FreeBSD.org> References: <201806270411.w5R4B9ZB078994@repo.freebsd.org> <20180627235216.GO1165@FreeBSD.org> <16B9F36D-E92A-4FEC-B096-5E24A4E180FC@FreeBSD.org> <623F6077-0A89-4D35-9E83-76BF40CC290F@FreeBSD.org> <7C4AEADC-1973-482B-A7A5-75F23397D44E@FreeBSD.org> From: Warner Losh Date: Thu, 28 Jun 2018 01:36:07 -0600 X-Google-Sender-Auth: OE5l_Z4gMClEUhGqKEhMcIdbNxc Message-ID: Subject: Re: svn commit: r335690 - head/sys/kern To: Devin Teske Cc: Gleb Smirnoff , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.26 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 28 Jun 2018 07:36:09 -0000 On Thu, Jun 28, 2018 at 12:54 AM, Devin Teske wrote: > > On Jun 27, 2018, at 7:35 PM, Warner Losh wrote: > > > > On Wed, Jun 27, 2018 at 8:14 PM, Devin Teske wrote: > >> >> On Jun 27, 2018, at 6:58 PM, Warner Losh wrote: >> >> >> >> On Wed, Jun 27, 2018 at 7:49 PM, Devin Teske wrote: >> >>> >>> On Jun 27, 2018, at 5:59 PM, Warner Losh wrote: >>> >>> >>> >>> On Wed, Jun 27, 2018 at 5:52 PM, Gleb Smirnoff >>> 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 >>>> 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 > #include > > #include > > 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. Warner