Date: Sat, 24 Sep 2022 21:20:13 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Benjamin Kaduk <bjkfbsd@gmail.com> Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 2c2ef670a79b - main - pseudofs: use the vget_prep/vget_finish idiom Message-ID: <CAGudoHG6Fsa5SOwNbDQ5kXAaD102J56wKuTAiCE3gKQ8vywoUw@mail.gmail.com> In-Reply-To: <CAJ5_RoBPt9uFWbTk9N=Gc=SV6i1ZySrh7hZ6rfdh_bV9ixu67Q@mail.gmail.com> References: <202209192009.28JK924f075123@gitrepo.freebsd.org> <CAJ5_RoA4=NqQ-XZ52rJ7J1xJMUg2U=kB%2B7b_x5sLHXFsJGwVSg@mail.gmail.com> <CAGudoHF1peBhw03ucSv%2B8eGdkC5=Fq_sOjPWjjd6iqtu44qgDw@mail.gmail.com> <CAJ5_RoBPt9uFWbTk9N=Gc=SV6i1ZySrh7hZ6rfdh_bV9ixu67Q@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 9/24/22, Benjamin Kaduk <bjkfbsd@gmail.com> wrote: > On Thu, Sep 22, 2022 at 3:11 AM Mateusz Guzik <mjguzik@gmail.com> wrote: > >> On 9/19/22, Benjamin Kaduk <bjkfbsd@gmail.com> wrote: >> > On Mon, Sep 19, 2022 at 1:09 PM Mateusz Guzik <mjg@freebsd.org> wrote: >> > >> >> The branch main has been updated by mjg: >> >> >> >> URL: >> >> >> https://cgit.FreeBSD.org/src/commit/?id=2c2ef670a79b7f8fa84796a04885a3f76c914762 >> >> >> >> commit 2c2ef670a79b7f8fa84796a04885a3f76c914762 >> >> Author: Mateusz Guzik <mjg@FreeBSD.org> >> >> AuthorDate: 2022-09-19 20:07:10 +0000 >> >> Commit: Mateusz Guzik <mjg@FreeBSD.org> >> >> CommitDate: 2022-09-19 20:08:40 +0000 >> >> >> >> pseudofs: use the vget_prep/vget_finish idiom >> >> >> >> >> > >> > Picking an arbitrary commit to reply to: could you please add a bit >> > more >> > detail about the "why" to commit messages in the future? >> > Having looked a little bit, it seems that this would be "as part of the >> > broader effort to remove the vnode interlock [from a specific class of >> > operations?]". A pointer to a bigger-picture doc would be great as >> > well. >> >> The idiom at hand is over 3 years old now and employing it at this >> point should not require a justification. >> >> One can easily see not taking the interlock is faster both single- and >> multi-threaded, which makes the change worthwhile regardless of >> support for the flag in locking primitives going away or not. >> >> > Maybe you can easily see so, and maybe I can easily see so as well. > But it is not guaranteed to be the case for everyone looking at the commit > history. > The "what" of the commit is contained in the commit itself, but the "why" > can easily > be lost to the depths of time; that is why I see it as more important to > record in the commit > message. > The what is to employ an in idiom, it being an idiom should require no justification in the commit message. > Indeed, when I was left here to guess at the "why", I got it wrong! So is > it so hard to add > another sentence or even a clause like "an easy microoptimization" to the > commit message? > Having some hint at the "why" greatly improves the quality of the record > that is the commit. > What did you think it is? VI_LOCK + vhold disappear in favor of vget_prep, quick look inside shows there is no interlock taken whatsoever and that's how one finds out it gets avoided. Performance implications should be obvious from that point. The way I see it extra content in this case decreases the quality significantly. Anyone familiar with the idiom can comfortably skip this commit while browsing the history because the one liner indicates there is nothing more to it. Should extra sentences be present it would suggest something non-standard is going on and attention is required, when it is not. It's equivalent to adding comments like "/* free the buffer */" just prior to "free(buf);". Anyone not familiar with the idiom can quickly find out what it is. Explaining something like this every single time a change of the sort is made makes no sense to me whatsoever. > > > >> So happens I'm going to keep the flag support in vget itself for the time >> being. >> >> > I co-maintain an out-of-tree filesystem and commit messages like this >> make >> > it really hard for me to get a handle on whether I need to do anything >> and, >> > if so, where to start looking to find out what to do. An overall >> > project >> > page would be a great reference, or even comment around the >> implementations >> > that points to a key differential revision that implemented the core >> > behavior. One of the things that's been really nice about developing >> > for >> > the FreeBSD VFS in the past is how easy it is to determine what a >> > filesystem implementation needs to provide, and I'd love to see us >> continue >> > that tradition. >> > >> >> I'm definitely confused by this comment. VFS has rampant layering >> violations, including wtfs like filesystems sneaking in their own >> changes to cn_flags by literally or-ing into it mid-lookup (recently >> fixed, see 5b5b7e2ca2fa9a2418dd51749f4ef6f881ae7179) or leaking their >> internals out and consequently imposing completely unnecessary >> restrictions on the layer (see a comment above vput_final for an >> example). >> >> > > I did not say that the VFS itself was great to develop, but rather that my > experience as a filesystem author has been good ("develop for" vs "develop" > is > an unfortunately subtle distinction in English). I consider my point above > about > including "why" in the commit message to be far more important than this > part, > which was just an attempt to add some motivation as to why I personally > care. > Other people have other good reasons to want to include the "why" in the > commit > message, that I surely did not cover fully here. > I did understood you as claiming vfs is easy to develop for, but apparently my response did not come across as intended, so let me try again. I know for a fact a lot of non-ufs filesystem code was written by copy pasting from ufs and I would be surprised if afs was fundamentally different here. The VFS interfaces are poor quality and documentation is for the most part stale, misleading, straight up wrong or missing. To give you an example here a comment concerning recently retired SAVENAME flag: * SAVENAME may be set by either the callers of namei or by VOP_LOOKUP. True. * If the caller of namei sets the flag (for example execve wants to * know the name of the program that is being executed), then it must * free the buffer. If VOP_LOOKUP sets the flag, then the buffer must * be freed by either the commit routine or the VOP_ABORT routine. 1. VOP_ABORT does not exist 2. What's 'commit routine'? Is that something in the fs? I presume it's not the caller, since otherwise it would be referred to as such 3. all freeing was performed in callers, so the above is wrong about cases where the filesystem sneaks in the flag So no, I don't think there was enough info to write a working filesystem from scratch just by reading vfs docs/comments even prior to any of my VFS changes. What was needed was reading an existing implementation, like ufs. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHG6Fsa5SOwNbDQ5kXAaD102J56wKuTAiCE3gKQ8vywoUw>