Re: [git pull] vfs.git get_user_pages_fast() conversion

[Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>]

On Thu, Nov 16, 2017 at 7:02 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
>         A bunch of places switched to get_user_pages_fast().

I really would have liked a bit of a commentary.

Looking at the individual patches, I notice this, for example:

-               down_read(&current->mm->mmap_sem);
-               page_nr = get_user_pages((unsigned long)userptr,
-                                        (int)(bo->pgnr), 1, pages, NULL);
-               up_read(&current->mm->mmap_sem);
+               page_nr = get_user_pages_fast((unsigned long)userptr,
+                                        (int)(bo->pgnr), 1, pages);


from the atomisp conversion, and it made me throw up my hands in horror.

Not because the conversion was wrong, but because the original code is
so broken.

In particular, that "1" that is unchanged in the arguments is correct
in the conversion, but it was completely wrong in the original, even
if it happened to work.

it _should_ have been a FOLL_WRITE. Yes, it happens to have that
value, but it was broken.

(I note that a bit of grepping shows we have the same issue in a stale
comment in mm/ksm.c).

It would have been nice to see things like this mentioned in the commit message.

Because I'm pretty sure you actually _realized_ that as you made the
conversion, but there's no sign of that in the logs, because the
commit message just says

    atomisp: use get_user_pages_fast()

without mentioning how broken the old case was (even it if happened to work).

                   Linus