Re: [PATCH -V5 RESEND 03/21] swap: Support PMD swap mapping in swap_duplicate()

[Daniel Jordan]

On Fri, Sep 28, 2018 at 04:19:03PM +0800, Huang, Ying wrote:
> Daniel Jordan <daniel.m.jordan@xxxxxxxxxx> writes:
> > One way is to change
> > copy_one_pte's return to int so we can just pass the error code back to
> > copy_pte_range so it knows whether to try adding the continuation.
> 
> There may be even more problems.  After add_swap_count_continuation(),
> copy_one_pte() will be retried, and the CPU may hang with dead loop.

That's true, it would do that.

> But before the changes in this patchset, the behavior is,
> __swap_duplicate() return an error that isn't -ENOMEM, such as -EEXIST.
> Then copy_one_pte() would thought the operation has been done
> successfully, and go to call set_pte_at().  This will cause the system
> state become inconsistent, and the system may panic or hang somewhere
> later.
> 
> So per my understanding, if we thought page table corruption isn't a
> real problem (that is, __swap_duplicate() will never return e.g. -EEXIST
> if copied by copy_one_pte() indirectly), both the original and the new
> code should be OK.
> 
> If we thought it is a real problem, we need to fix the original code and
> keep it fixed in the new code.  Do you agree?

Yes, if it was a real problem, which seems less and less the case the more I
stare at this.

> There's several ways to fix the problem.  But the page table shouldn't
> be corrupted in practice, unless there's some programming error.  So I
> suggest to make it as simple as possible via adding,
> 
> VM_BUG_ON(error != -ENOMEM);
> 
> in swap_duplicate().
> 
> Do you agree?

Yes, I'm ok with that, adding in -ENOTDIR along with it.

The error handling in __swap_duplicate (before this series) still leaves
something to be desired IMHO.  Why all the different returns when callers
ignore them or only specifically check for -ENOMEM or -EEXIST?  Could maybe
stand a cleanup, but outside this series.