On Wed, Jun 3, 2020 at 3:56 PM Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> From: John Hubbard <jhubbard@xxxxxxxxxx>
> Subject: mm/gup: refactor and de-duplicate gup_fast() code
>
> There were two nearly identical sets of code for gup_fast() style of
> walking the page tables with interrupts disabled. This has lead to the
> usual maintenance problems that arise from having duplicated code.
Andrew, this is actually an example of why you absolutely should *not*
rebase your series in the middle of the development tree.
Now you've rebased it on top of my commit 17839856fd58 ("gup: document
and work around "COW can break either way" issue") and in the process
you broke the result completely for read-only pages.
Now it uses FOLL_WRITE (because that's what
internal_get_user_pages_fast() does), which will disallow read-only
pages (in order to handle them properly for COW in the slow path), and
then the fact that the slow-path is entirely disabled for this case
means that it doesn't work at all.
This "rebase onto whatever random base Linus has today" absolutely has
*got* to stop.
It's not ok for git trees, but it's not ok for these patch-queues
either. It means that all the testing your patch queue got in
linux-next is completely worthless, because what you send me is
something very different from what was tested. Exactly as with the git
trees, where I tell people constantly not to rebase their patches.
Give me a base that it has been tested on, and a series that has
actually been tested. Not this "rebased for your convenience" thing.
I'd _much_ rather get a merge conflict when your patch series changes
something that somebody else also changed.
Because then I know something clashed, and if I screw up the merge, I
only have myself to blame. If it's a very complex merge, I'll ask for
help.
That would be much better than getting a patch-bomb with 131 patches
that all _look_ sane and build cleanly, but can be randomly broken
because they got rebased hours before with no testing.
The "let me fix things up onto a daily snapshot" really is a
completely broken model. You are making it _harder_ for me, not
easier, because now I have to look for subtle issues in every single
commit rather than the big honking clue of "oh, I got a merge error,
I'll need to really look at it".
It so happened that with this one, I was very aware of the rebase,
because you rebased on a patch that I wrote so when I looked through
the patches I went "Hmm.."
What about all the other times when I wouldn't have noticed and been
so aware of what changed recently?
Again: merge conflicts are *much* better than silently rebasing and
hiding problems.
Linus
