Re: [PATCH] filemap: Handle error return from __filemap_get_folio()

[Linus Torvalds <torvalds@linux-foundation.org>]

On Sat, May 6, 2023 at 9:09 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Thanks, applied directly.

Actually, I take that back.

I applied it, and then I looked at the context some more, and decided
that I hated it.

It's not that the patch itself was wrong, but the whole original

        if (folio)
                folio_put(folio);

was wrong in that context, and shouldn't have been done that way.

Converting the conditional to use !IS_ERR() fixes a conversion error,
but doesn't fix the original problem with the code.

The only path that triggered that "we have no folio" was wrong to jump
to that "should I drop this folio" code sequence in the first place.

So the fix is to not use "out_retry" at all for the IS_ERR(folio) case.

Yes, yes, compilers can do jump threading, and in a perfect world
might realize that there are two consecutive tests for IS_ERR(folio)
and just do this kind of conversion automatically.

But the fact that compilers *might* fix out our code to do the right
thing doesn't mean that the code shouldn't have been written to just
have the proper error handling nesting in the first place.

And yes, the simplest fix for the "wrong test" would be to just add a
new "out_nofolio" error case after "out_retry", and use that.

However, even that seems wrong, because the return value for that path
is the wrong one.

So the "goto out_retry" was actually *doubly* wrong.

If the __filemap_get_folio(FGP_CREAT) failed, then we should not
return VM_FAULT_RETRY at all. We should return VM_FAULT_OOM, like it
does for the no-fpin case.

So the whole

                if (IS_ERR(folio)) {
                        if (fpin)
                                goto out_retry;

sequence seems to be completely wrong in several ways. It shouldn't go
to "out_retry" at all, because this is simply not a "retry" case.

And that in turn means that "out_retry" shouldn't be testing folio at
all (whether for IS_ERR() _or_ for NULL).

So i really think the proper fix is this (whitespace-damaged) patch instead:

    --- a/mm/filemap.c
    +++ b/mm/filemap.c
    @@ -3291,7 +3291,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
                                              vmf->gfp_mask);
                    if (IS_ERR(folio)) {
    -                       if (fpin)
    -                               goto out_retry;
                            filemap_invalidate_unlock_shared(mapping);
    +                       if (fpin)
    +                               fput(fpin);
                            return VM_FAULT_OOM;
                    }
    @@ -3379,6 +3379,5 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
             * page.
             */
    -       if (folio)
    -               folio_put(folio);
    +       folio_put(folio);
            if (mapping_locked)
                    filemap_invalidate_unlock_shared(mapping);

which fixes both problems by simply avoiding a bogus 'goto' entirely.

Hmm?

                      Linus