svn commit: r344758 - in head/sys/fs: nfs nfsserver

[Cy Schubert]

On March 4, 2019 3:13:05 PM PST, Rick Macklem <rmacklem at uoguelph.ca> wrote:
>Cy Schubert wrote:
>>Sent: Monday, March 4, 2019 3:25 PM
>>To: Edward Napierala
>>Cc: Cy Schubert; Konstantin Belousov; src-committers;
>svn-src-all at freebsd.org; svn-src->head at freebsd.org
>>Subject: Re: svn commit: r344758 - in head/sys/fs: nfs nfsserver
>>
>>In message
><CAFLM3-pFi_-yhMx4bhm_0S5FK3BdLh1xbXV+cKgPks7d8cttMw at mail.gma
>il.com>
>, Edward Napierala writes:
>>> pon., 4 mar 2019 o 15:17 Cy Schubert <Cy.Schubert at cschubert.com>
>napisał(a):
>>> >
>>> > On March 4, 2019 6:30:21 AM PST, Konstantin Belousov
><kostikbel at gmail.com>
>>> wrote:
>>> > >On Mon, Mar 04, 2019 at 01:31:37PM +0000, Edward Napierala wrote:
>>> > >> pon., 4 mar 2019 o 13:20 Konstantin Belousov
><kostikbel at gmail.com>
>>> > >napisał(a):
>>> > >> > > +     p = curthread;
>>> > >> > Why do you name it 'p', which is typical for process, and not
>'td',
>>> > >you are
>>> > >> > changing most of the code anyway.
>>> > >>
>>> > >> To keep the diff size smaller.  You're right, this touches a
>lot of
>>> > >stuff,
>>> > >> but most of those added lines are temporary anyway - they will
>be
>>> > >> removed later, when the td is pushed down even more.
>>> > >But if you create code churn, doing it only half way is worse.
>>> > >
>>> > >>
>>> > >> > Also I am curious why. It is certainly fine to remove td when
>it is
>>> > >used
>>> > >> > as a formal placeholder argument only. But when the first
>action in
>>> > >the
>>> > >> > function is evaluation of curthread() it becomes less
>obvious.
>>> > >>
>>> > >> Again, many/most of those are temporary.  I'm trying to push td
>down
>>> > >> in small steps, "layer by layer", so it's easy to review.
>>> > >>
>>> > >> > curthread() become very cheap on modern amd64, I am not so
>sure
>>> > >about
>>> > >> > older machines or non-x86 cases.
>>> > >>
>>> > >> The main reason is readability.  Right now there's no easy way
>to
>>> > >tell whether
>>> > >> a function can be passed any td, or if it must be curthread.
>>> > >I must admit that this is the weirdnest argument against 'td'
>that I
>>> > >ever
>>> > >heard.  I saw more or less reasonable argumentation
>>> > >- that using less arguments make one more register for argument
>passing
>>> > >  (amd64 has 6 input arg regs),
>>> > >- that less arguments make smaller call code.
>>> > >But trust me, in all cases where function can take td !=
>curthread, it
>>> > >is
>>> > >either obvious or well-known for anybody who works with that
>code.
>>> > >
>>> > >Before you start doing a lot of small changes (AKA continous
>churn)
>>> > >please formulate your goals and get some public feedback.  My
>immediate
>>> > >question that I want answered before you ever start touching the
>code,
>>> > >is what you plan to do with
>>> > >       sys_syscall(struct thread *td, uap)
>>> >
>>> > Agreed on all points. At the very least this group of commits
>should be rev
>>> iewed on phabricator.
>>>
>>> It has been, even though they are pretty much mechanical changes.
>>>
>>> > Can we back all these commits out until there is a proper review,
>please?
>>>
>>> The review from the NFS maintainer is not enough?
>>>
>Btw, I've never listed myself as the NFS maintainer. I need to go look
>to see if
>someone else put me in the maintainer's list. I understand that it is
>mostly authored
>by me, but I consider it FreeBSD project code once committed. (Others
>do commits
>to it without my review and that is just fine with me.)
>
>>As it's NFS-only maybe though for anything substantial, like this, the
>>more eyes the better.
>>
>>I'd agree with kib@ that we want to keep the amount of churn down,
>>though it's understood that you want to separate the functional
>changes
>>from the cosmetic ones. I tend to do the review and use git svn to
>>separate the functional from the cosmetic changes, batching changes if
>>I can. It's more work but IMO well worth it.
>This is way too technical for me. I can barely look at a "diff" and
>make sense of it.;-)
>
>It sounds like there needs to be a discussion (on freebsd-fs@ perhaps?)
>of the
>"big picture change" here.
>
>All I am going to do with the patches in phabricator is take a quick
>look to see
>if I can spot anything that will break the code.
>(I did mention that I didn't understand why he was doing this in one of
>the reviews,
> but noted that it didn't break anything.)

IMO the why is always more important than the how. If there is no reason why then how is irrelevant.

>
>Oh, and the variable was called "p" because the code started in
>OpenBSD, where it
>was a proc ptr and I kept it portable by replacing "struct proc" with
>NFSPROC_T.
>(This portability is no longer a consideration.)
>
>I'll hold off on further phabricator reviews until the "big picture"
>change gets discussed
>on some mailing list. (I don't see phabricator as the correct tool for
>"big picture"
>discussions.)
>
>rick

Hopefully my inline reply on this phone worked. If not I'll try again tonight.

-- 
Pardon the typos and autocorrect, small keyboard in use.
Cheers,
Cy Schubert <Cy.Schubert at cschubert.com>
FreeBSD UNIX: <cy at FreeBSD.org> Web: http://www.FreeBSD.org

	The need of the many outweighs the greed of the few.