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

["Huang\, Ying" <ying.huang@intel.com>]

Daniel Jordan <daniel.m.jordan@oracle.com> writes:

> On Fri, Sep 28, 2018 at 04:19:03PM +0800, Huang, Ying wrote:
>> Daniel Jordan <daniel.m.jordan@oracle.com> 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.

Sure.  I will do this.

> 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.

Yes.  Maybe.  I guess you will work on this?

Best Regards,
Huang, Ying