All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Theodore Y. Ts'o" <tytso@mit.edu>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Linux-Next Mailing List <linux-next@vger.kernel.org>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	Olof Johansson <olof@lxom.net>,
	Konstantin Ryabitsev <konstantin@linuxfoundation.org>,
	David Miller <davem@davemloft.net>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	Paul Lawrence <paullawrence@google.com>,
	Sandipan Das <sandipan@linux.vnet.ibm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	David Woodhouse <dwmw2@infradead.org>,
	Will Deacon <will.deacon@arm.com>,
	Philippe Ombredanne <pombredanne@nexb.com>,
	Paul Burton <paul.burton@mips.com>,
	David Rientjes <rientjes@google.com>, Willy Tarreau <w@1wt.eu>,
	Martin Sebor <msebor@gmail.com>,
	Christopher Li <sparse@chrisli.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Geert Uytterhoeven <geert@linux-m68k.org>,
	Rasmus Villemoes <linux@rasmusvillemoes.dk>,
	Joe Perches <joe@perches.com>, Arnd Bergmann <arnd@arndb.de>,
	Dominique Martinet <asmadeus@codewreck.org>,
	Stefan Agner <stefan@agner.ch>,
	Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Linux Doc Mailing List <linux-doc@vger.kernel.org>,
	Ext4 Developers List <linux-ext4@vger.kernel.org>,
	linux-sparse@vger.kernel.org, linux-kbuild@vger.kernel.org,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Stephen Rothwell <sfr@canb.auug.org.au>
Subject: Re: [GIT PULL linux-next] Add Compiler Attributes tree
Date: Thu, 4 Oct 2018 01:01:40 -0400	[thread overview]
Message-ID: <20181004050140.GC646@thunk.org> (raw)
In-Reply-To: <CANiq72nMu6NXg1Ovn49qJ=FP_S4_LQJSwPuS+Q_zofhOEbVZng@mail.gmail.com>

On Wed, Oct 03, 2018 at 11:41:08PM +0200, Miguel Ojeda wrote:
> 
> I forgot to mention that if the definitions were different, it could
> have caused a problem, because your definition wouldn't apply, so your
> 27+ hours of testing wouldn't have mattered :-P Without the #ifndef,
> we would have at least got a redefinition warning about it.

In this case, yes.  Again, I emphasize that I was using the ext4.h
cleanup as an *example*.  The point I was trying to make was that your
change did *not* do a full set of deep ext4 regression tests because
the your change didn't go through the ext4.git tree.  I have seen
cases where "simple mechanical changes" were anything but, and while
the kernel *compiled* it resulted in the file system not working.
(And in the worst case, it resulted actual data loss or file system
corruption.)

My test scripts are public, and in fact I've gone out of my way to
make them easy for other people to run them, with documentation[1][2],
slide sets[3], etc.  But the vast majority of people who submit
patches to ext4.git don't bother --- and as far as I know *no one* who
sends ext4 changes via other git trees *ever* bothered.  (And, for one
recent patchset where the ext4 developer spent a lot of time using
kvm-xfstests before submission, accepting that set of changes was easy
and was a joy.  I'm a big believe in tests.)

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/gce-xfstests.md
[2] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
[3] https://thunk.org/gce-xfstests

As far as what you want to do, there are solutions, but they require a
radically different way of developing.  For example, inside Google,
for the non-public sources (e.g., excluding Android, Chrome OS, etc.)
there is a single source tree, with thousands of projects.  They use
common set of C++ utility libraries/classes, and there is a
standardized build system (bazel[4]), and a standardized way of
writing tests.  More importantly, there is a fully automated
continuous testing system which will automatically trigger and run the
appropriate tests --- at the moment when the patch is submitted into
the Google's custom code review system --- and the results of the test
are automatically submitted as comments in the code review system, so
the automated testing is integrated into the code review.  For changes
that affected all or substantially all projects, it's possible to run
tests across the entire source tree using automated, centralized
resources, and even then it can take a significantlly non-trivial (as
in days) of calendar time running on many systems in parallel.

[4] https://bazel.build/

So if you are willing to completely standardize your testing
infrastructure and devote utterly lavish amounts of automated testing
which is deeply integrated into a standardized, non-email code review
system, the challenge you have identified *has* been solved.  But
trust me when I say that it is a very non-trivial thing to do, and it
requires a lot of very strict code development practices that are
imposed on all Google C++ developers working in the common tree (which
is probably 90%+ of the SWE's inside Google).  I'm not at all
convinced that it could be adopted or imposed on the kernel
development process.  In fact, I'm quite confident it could not be.

I actually think the Linux Kernel's approach of carefully segregating
how code and headers to avoid merge conflicts (and worse, semantic
conflicts that may git merge and build succesfully, but then blow up
in subtle ways leading to user data loss) is actually a pretty good
way of doing things.  It works 99.99% of the the commits.  True, it
wasn't optimal for the changes you were trying to make; but your
experience is the exception, not the rule.

The risk here is that it's very easy to engineer changes in processes
for corner cases, and which make things much worse (either taking more
time, or more effort, or being less reliable) for the common case.

Cheers,

					- Ted


WARNING: multiple messages have this Message-ID (diff)
From: "Theodore Y. Ts'o" <tytso@mit.edu>
To: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg KH <gregkh@linuxfoundation.org>,
	Linux-Next Mailing List <linux-next@vger.kernel.org>,
	Andreas Dilger <adilger.kernel@dilger.ca>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Michal Marek <michal.lkml@markovi.net>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mauro Carvalho Chehab <mchehab+samsung@kernel.org>,
	Olof Johansson <olof@lxom.net>,
	Konstantin Ryabitsev <konstantin@linuxfoundation.org>,
	David Miller <davem@davemloft.net>,
	Andrey Ryabinin <aryabinin@virtuozzo.com>,
	Kees Cook <keescook@chromium.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	Paul Lawrence <paullawrence@google.com>,
	Sandipan Das <sandipan@linux.vnet.ibm.com>
Subject: Re: [GIT PULL linux-next] Add Compiler Attributes tree
Date: Thu, 4 Oct 2018 01:01:40 -0400	[thread overview]
Message-ID: <20181004050140.GC646@thunk.org> (raw)
In-Reply-To: <CANiq72nMu6NXg1Ovn49qJ=FP_S4_LQJSwPuS+Q_zofhOEbVZng@mail.gmail.com>

On Wed, Oct 03, 2018 at 11:41:08PM +0200, Miguel Ojeda wrote:
> 
> I forgot to mention that if the definitions were different, it could
> have caused a problem, because your definition wouldn't apply, so your
> 27+ hours of testing wouldn't have mattered :-P Without the #ifndef,
> we would have at least got a redefinition warning about it.

In this case, yes.  Again, I emphasize that I was using the ext4.h
cleanup as an *example*.  The point I was trying to make was that your
change did *not* do a full set of deep ext4 regression tests because
the your change didn't go through the ext4.git tree.  I have seen
cases where "simple mechanical changes" were anything but, and while
the kernel *compiled* it resulted in the file system not working.
(And in the worst case, it resulted actual data loss or file system
corruption.)

My test scripts are public, and in fact I've gone out of my way to
make them easy for other people to run them, with documentation[1][2],
slide sets[3], etc.  But the vast majority of people who submit
patches to ext4.git don't bother --- and as far as I know *no one* who
sends ext4 changes via other git trees *ever* bothered.  (And, for one
recent patchset where the ext4 developer spent a lot of time using
kvm-xfstests before submission, accepting that set of changes was easy
and was a joy.  I'm a big believe in tests.)

[1] https://github.com/tytso/xfstests-bld/blob/master/Documentation/gce-xfstests.md
[2] https://github.com/tytso/xfstests-bld/blob/master/Documentation/kvm-quickstart.md
[3] https://thunk.org/gce-xfstests

As far as what you want to do, there are solutions, but they require a
radically different way of developing.  For example, inside Google,
for the non-public sources (e.g., excluding Android, Chrome OS, etc.)
there is a single source tree, with thousands of projects.  They use
common set of C++ utility libraries/classes, and there is a
standardized build system (bazel[4]), and a standardized way of
writing tests.  More importantly, there is a fully automated
continuous testing system which will automatically trigger and run the
appropriate tests --- at the moment when the patch is submitted into
the Google's custom code review system --- and the results of the test
are automatically submitted as comments in the code review system, so
the automated testing is integrated into the code review.  For changes
that affected all or substantially all projects, it's possible to run
tests across the entire source tree using automated, centralized
resources, and even then it can take a significantlly non-trivial (as
in days) of calendar time running on many systems in parallel.

[4] https://bazel.build/

So if you are willing to completely standardize your testing
infrastructure and devote utterly lavish amounts of automated testing
which is deeply integrated into a standardized, non-email code review
system, the challenge you have identified *has* been solved.  But
trust me when I say that it is a very non-trivial thing to do, and it
requires a lot of very strict code development practices that are
imposed on all Google C++ developers working in the common tree (which
is probably 90%+ of the SWE's inside Google).  I'm not at all
convinced that it could be adopted or imposed on the kernel
development process.  In fact, I'm quite confident it could not be.

I actually think the Linux Kernel's approach of carefully segregating
how code and headers to avoid merge conflicts (and worse, semantic
conflicts that may git merge and build succesfully, but then blow up
in subtle ways leading to user data loss) is actually a pretty good
way of doing things.  It works 99.99% of the the commits.  True, it
wasn't optimal for the changes you were trying to make; but your
experience is the exception, not the rule.

The risk here is that it's very easy to engineer changes in processes
for corner cases, and which make things much worse (either taking more
time, or more effort, or being less reliable) for the common case.

Cheers,

					- Ted

  reply	other threads:[~2018-10-04  5:02 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02 13:47 [GIT PULL linux-next] Add Compiler Attributes tree Miguel Ojeda
2018-10-02 13:47 ` Miguel Ojeda
2018-10-02 21:10 ` Stephen Rothwell
2018-10-02 21:10   ` Stephen Rothwell
2018-10-02 21:16   ` Nick Desaulniers
2018-10-02 21:16     ` Nick Desaulniers
2018-10-02 22:12     ` Miguel Ojeda
2018-10-02 22:12       ` Miguel Ojeda
2018-10-02 22:36       ` Dominique Martinet
2018-10-02 22:36         ` Dominique Martinet
2018-10-02 23:00         ` Stephen Rothwell
2018-10-02 23:00           ` Stephen Rothwell
2018-10-03 12:34           ` Miguel Ojeda
2018-10-03 12:34             ` Miguel Ojeda
2018-10-03 13:00             ` Steven Rostedt
2018-10-03 13:00               ` Steven Rostedt
2018-10-03 12:14         ` Miguel Ojeda
2018-10-03 12:14           ` Miguel Ojeda
2018-10-03 12:30           ` Steven Rostedt
2018-10-03 12:30             ` Steven Rostedt
2018-10-02 23:24       ` Theodore Y. Ts'o
2018-10-02 23:24         ` Theodore Y. Ts'o
2018-10-03 11:54         ` Miguel Ojeda
2018-10-03 11:54           ` Miguel Ojeda
2018-10-03 15:33           ` Theodore Y. Ts'o
2018-10-03 15:33             ` Theodore Y. Ts'o
2018-10-03 21:23             ` Miguel Ojeda
2018-10-03 21:23               ` Miguel Ojeda
2018-10-03 21:41               ` Miguel Ojeda
2018-10-03 21:41                 ` Miguel Ojeda
2018-10-04  5:01                 ` Theodore Y. Ts'o [this message]
2018-10-04  5:01                   ` Theodore Y. Ts'o
2018-10-04  9:06                   ` Miguel Ojeda
2018-10-04  9:06                     ` Miguel Ojeda
2018-10-02 22:04   ` Miguel Ojeda
2018-10-02 22:04     ` Miguel Ojeda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181004050140.GC646@thunk.org \
    --to=tytso@mit.edu \
    --cc=adilger.kernel@dilger.ca \
    --cc=akpm@linux-foundation.org \
    --cc=andreyknvl@google.com \
    --cc=arnd@arndb.de \
    --cc=aryabinin@virtuozzo.com \
    --cc=asmadeus@codewreck.org \
    --cc=corbet@lwn.net \
    --cc=davem@davemloft.net \
    --cc=dwmw2@infradead.org \
    --cc=geert@linux-m68k.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=joe@perches.com \
    --cc=keescook@chromium.org \
    --cc=konstantin@linuxfoundation.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-kbuild@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-next@vger.kernel.org \
    --cc=linux-sparse@vger.kernel.org \
    --cc=linux@rasmusvillemoes.dk \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=mchehab+samsung@kernel.org \
    --cc=michal.lkml@markovi.net \
    --cc=miguel.ojeda.sandonis@gmail.com \
    --cc=mingo@kernel.org \
    --cc=msebor@gmail.com \
    --cc=ndesaulniers@google.com \
    --cc=olof@lxom.net \
    --cc=paul.burton@mips.com \
    --cc=paullawrence@google.com \
    --cc=pombredanne@nexb.com \
    --cc=rientjes@google.com \
    --cc=rostedt@goodmis.org \
    --cc=sandipan@linux.vnet.ibm.com \
    --cc=sfr@canb.auug.org.au \
    --cc=sparse@chrisli.org \
    --cc=stefan@agner.ch \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=w@1wt.eu \
    --cc=will.deacon@arm.com \
    --cc=yamada.masahiro@socionext.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.