From nobody Sat Sep 24 16:42:14 2022 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4MZZYt5T3Mz4d3Gj; Sat, 24 Sep 2022 16:42:26 +0000 (UTC) (envelope-from bjkfbsd@gmail.com) Received: from mail-vs1-xe2a.google.com (mail-vs1-xe2a.google.com [IPv6:2607:f8b0:4864:20::e2a]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4MZZYt16jDz3G3Z; Sat, 24 Sep 2022 16:42:26 +0000 (UTC) (envelope-from bjkfbsd@gmail.com) Received: by mail-vs1-xe2a.google.com with SMTP id m66so2750005vsm.12; Sat, 24 Sep 2022 09:42:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=tzt284FL7w3FMrlBsrMuNliGtKppdZ3qhkuOvTR2rcY=; b=S0HOPlPkLreEEkfUiPwdgfn95LFdqKFlWPFAPGQK7oItptleILjG6RBeLtrkpP4pTZ TtucU/jfnFx238i0F2TyyfRDHkNKu6LQeBVB7KGsDE9nFXh56OtPmiqCvKs5vltFiXl0 7p5bQHHXQe2NFrWmg8dgNOL18P8o/hAbjJ7dEacG4BU3PNX/xqKXXGKBona1n+ifbQuS P75LSSo+WGF8qQGjIDr0YajCSlhC5CqxdVYl/ITBJw7yLMxHI3vRvUhLytycfc9wHwQE KadfSDnAmg7120pTnBw6eEnRwIkYuUP8HMTPcQcxQxEGHzSfjZSG3OJumFsCYssoOrSZ 6BXw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=tzt284FL7w3FMrlBsrMuNliGtKppdZ3qhkuOvTR2rcY=; b=VScVmh2qj+rn9VS1T5w2fYR0e7qyjhX3PYOLQqbwTv3SN7Nv71xg+rmHEVgV+s+BYR YnvbuOUsCQlxwfxNVfrpvvffFYPTOULd5WSIq4ZlJfHCqBS6wTjvILO2eR6FntW29Ncq ZYaADGbxy36mmMnwIAenK3/3EIMxKvRUe4hUmPDcdFch1wB7l6wdKf8Zt4xjss26D3ha 2OIRFMW8RPU0vntz0qgZa0qocaiEFwQLWGRltBFl18MKx5RJ42gcEE9wc17T4oUDnhBq fIkqh2fsDMxcRry+p6m4BsOpb3LhMZ3B0f6XiCQpYhWAiSVigEQpuL+1SC5Xdr7dY5Dl 8CpA== X-Gm-Message-State: ACrzQf1mYqnhjArN58nnSAQq3RmSu95Q31DmRZsyzNJgPLxK5P2S0ad3 vWCOfsfPxpoKERmnqiAufW4pBNDyDlfunrxgCv4= X-Google-Smtp-Source: AMsMyM4tGGys2GNz3kDkMkqH32HRm/nQJkdvV69HP3P2MfC8YENTcFsFotKCLk9UOfvgTZITThd53ZxvkSL5EAhI5NE= X-Received: by 2002:a67:dd8c:0:b0:398:4a60:34d2 with SMTP id i12-20020a67dd8c000000b003984a6034d2mr5595889vsk.17.1664037745383; Sat, 24 Sep 2022 09:42:25 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202209192009.28JK924f075123@gitrepo.freebsd.org> In-Reply-To: From: Benjamin Kaduk Date: Sat, 24 Sep 2022 09:42:14 -0700 Message-ID: Subject: Re: git: 2c2ef670a79b - main - pseudofs: use the vget_prep/vget_finish idiom To: Mateusz Guzik Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="000000000000b4db6905e96efa1e" X-Rspamd-Queue-Id: 4MZZYt16jDz3G3Z X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20210112 header.b=S0HOPlPk; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of bjkfbsd@gmail.com designates 2607:f8b0:4864:20::e2a as permitted sender) smtp.mailfrom=bjkfbsd@gmail.com X-Spamd-Result: default: False [-2.37 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.67)[-0.674]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; NEURAL_SPAM_LONG(0.30)[0.300]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20210112]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36:c]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; FREEMAIL_TO(0.00)[gmail.com]; ARC_NA(0.00)[]; MIME_TRACE(0.00)[0:+,1:+,2:~]; FROM_EQ_ENVFROM(0.00)[]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FREEMAIL_ENVFROM(0.00)[gmail.com]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::e2a:from]; DKIM_TRACE(0.00)[gmail.com:+]; MID_RHS_MATCH_FROMTLD(0.00)[]; RCPT_COUNT_THREE(0.00)[4]; FROM_HAS_DN(0.00)[]; RCVD_COUNT_TWO(0.00)[2]; FREEMAIL_FROM(0.00)[gmail.com]; TO_DN_SOME(0.00)[]; RCVD_TLS_LAST(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim] X-ThisMailContainsUnwantedMimeParts: N --000000000000b4db6905e96efa1e Content-Type: text/plain; charset="UTF-8" On Thu, Sep 22, 2022 at 3:11 AM Mateusz Guzik wrote: > On 9/19/22, Benjamin Kaduk wrote: > > On Mon, Sep 19, 2022 at 1:09 PM Mateusz Guzik wrote: > > > >> The branch main has been updated by mjg: > >> > >> URL: > >> > https://cgit.FreeBSD.org/src/commit/?id=2c2ef670a79b7f8fa84796a04885a3f76c914762 > >> > >> commit 2c2ef670a79b7f8fa84796a04885a3f76c914762 > >> Author: Mateusz Guzik > >> AuthorDate: 2022-09-19 20:07:10 +0000 > >> Commit: Mateusz Guzik > >> 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. 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. > 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. > That said, the changes I normally made are either backwards-compatible > or are guaranteed to blow up at compilation time (or worst case at > runtime with an assert). > > Okay, thank you for letting me know. It is reassuring for my own personal situation with OpenAFS. > I agree some form a general doc would be nice, but between spending > time writing one or actually getting things done, I chose the latter. > > That's your prerogative, and I won't attempt to force you to do otherwise. For myself, I find that having even a barebones doc has been incredibly helpful when I come back to the work 3-5 years down the line, let alone when I am looking at stuff someone else did 3-5 years previously. Getting things done is great, and knowing the overall state of the project as it evolves is also great. > There are several filesystems which I have no intent in modifying > beyond bare minimum to keep them operational, thus general theme of > backwards compat will continue. > > Okay. Thank you. -Ben --000000000000b4db6905e96efa1e Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Thu, Sep 22, 2022 at 3:11 AM Mateusz G= uzik <mjguzik@gmail.com> wro= te:
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://c= git.FreeBSD.org/src/commit/?id=3D2c2ef670a79b7f8fa84796a04885a3f76c914762
>>
>> commit 2c2ef670a79b7f8fa84796a04885a3f76c914762
>> Author:=C2=A0 =C2=A0 =C2=A0Mateusz Guzik <mjg@FreeBSD.org> >> AuthorDate: 2022-09-19 20:07:10 +0000
>> Commit:=C2=A0 =C2=A0 =C2=A0Mateusz Guzik <mjg@FreeBSD.org> >> CommitDate: 2022-09-19 20:08:40 +0000
>>
>>=C2=A0 =C2=A0 =C2=A0pseudofs: use the vget_prep/vget_finish idiom >>
>>
>
> Picking an arbitrary commit to reply to: could you please add a bit mo= re
> 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?]".=C2=A0 A pointer to a bigger-picture doc would be g= reat 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.




=C2=A0
So happens I'm going to keep the flag support in vget itself for the ti= me 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.=C2=A0 An overall= project
> page would be a great reference, or even comment around the implementa= tions
> that points to a key differential revision that implemented the core > behavior.=C2=A0 One of the things that's been really nice about de= veloping 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 "develo= p" is
an unfortunately subtle distinction in English).=C2=A0= I consider my point above about
including "why" in the= commit message to be far more important than this part,
which wa= s 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&= quot; in the commit
message, that I surely did not cover fully he= re.
=C2=A0
That said, the changes I normally made are either backwards-compatible
or are guaranteed to blow up at compilation time (or worst case at
runtime with an assert).


Okay, thank you for letting me know.= =C2=A0 It is reassuring for my own personal situation with OpenAFS.
=C2=A0
I agree some form a general doc would be nice, but between spending
time writing one or actually getting things done, I chose the latter.



helpful when I come back to the work 3-5 years down the line, let alone w= hen
There are several filesystems which I have no intent in modifying
beyond bare minimum to keep them operational, thus general theme of
backwards compat will continue.


Okay.=C2=A0 =C2=A0Thank you.

-Ben
--000000000000b4db6905e96efa1e--