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

[Rick Macklem]

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.)

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