Re: Buffered I/O broken on s390x with page faults disabled (gfs2)

[David Hildenbrand <david@redhat.com>]

On 08.03.22 14:20, Gerald Schaefer wrote:
> On Tue, 8 Mar 2022 13:24:19 +0100
> David Hildenbrand <david@redhat.com> wrote:
> 
> [...]
>>
>> From 1e51e8a93894f87c0a4d0e908391e0628ae56afe Mon Sep 17 00:00:00 2001
>> From: David Hildenbrand <david@redhat.com>
>> Date: Tue, 8 Mar 2022 12:51:26 +0100
>> Subject: [PATCH] mm/gup: fix buffered I/O on s390x with pagefaults disabled
>>
>> On s390x, we actually need a pte_mkyoung() / pte_mkdirty() instead of
>> going via the page and leaving the PTE unmodified. E.g., if we only
>> mark the page accessed via mark_page_accessed() when doing a FOLL_TOUCH,
>> we'll miss to clear the HW invalid bit in the pte and subsequent accesses
>> via the MMU would still require a pagefault.
>>
>> Otherwise, buffered I/O will loop forever because it will keep stumling
>> over the set HW invalid bit, requiring a page fault.
>>
>> Reported-by: Andreas Gruenbacher <agruenba@redhat.com>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
>> ---
>>  mm/gup.c | 32 +++++++++++++++++++++++++-------
>>  1 file changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index a9d4d724aef7..de3311feb377 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -587,15 +587,33 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
>>  		}
>>  	}
>>  	if (flags & FOLL_TOUCH) {
>> -		if ((flags & FOLL_WRITE) &&
>> -		    !pte_dirty(pte) && !PageDirty(page))
>> -			set_page_dirty(page);
>>  		/*
>> -		 * pte_mkyoung() would be more correct here, but atomic care
>> -		 * is needed to avoid losing the dirty bit: it is easier to use
>> -		 * mark_page_accessed().
>> +		 * We have to be careful with updating the PTE on architectures
>> +		 * that have a HW dirty bit: while updating the PTE we might
>> +		 * lose that bit again and we'd need an atomic update: it is
>> +		 * easier to leave the PTE untouched for these architectures.
>> +		 *
>> +		 * s390x doesn't have a hw referenced / dirty bit and e.g., sets
>> +		 * the hw invalid bit in pte_mkold(), to catch further
>> +		 * references. We have to update the PTE here to e.g., clear the
>> +		 * invalid bit; otherwise, callers that rely on not requiring
>> +		 * an MMU fault once GUP(FOLL_TOUCH) succeeded will loop forever
>> +		 * because the page won't actually be accessible via the MMU.
>>  		 */
>> -		mark_page_accessed(page);
>> +		if (IS_ENABLED(CONFIG_S390)) {
>> +			pte = pte_mkyoung(pte);
>> +			if (flags & FOLL_WRITE)
>> +				pte = pte_mkdirty(pte);
>> +			if (!pte_same(pte, *ptep)) {
>> +				set_pte_at(vma->vm_mm, address, ptep, pte);
>> +				update_mmu_cache(vma, address, ptep);
>> +			}
>> +		} else {
>> +			if ((flags & FOLL_WRITE) &&
>> +			    !pte_dirty(pte) && !PageDirty(page))
>> +				set_page_dirty(page);
>> +			mark_page_accessed(page);
>> +		}
>>  	}
>>  	if ((flags & FOLL_MLOCK) && (vma->vm_flags & VM_LOCKED)) {
>>  		/* Do not mlock pte-mapped THP */
> 
> Thanks David, your analysis looks valid, at least it seems that you found
> a scenario where we would have HW invalid bit set due to pte_mkold() in
> ptep_clear_flush_young(), and still GUP would find and return that page, IIUC.
> 
> I think pte handling should be similar to pmd handling in follow_trans_huge_pmd()
> -> touch_pmd(), or cow_user_page() (see comment on software "accessed" bits),
> which is more or less what your patch does.
> 
> Some possible concerns:
> - set_page_dirty() would not be done any more for s390, is that intended and ok?

I strongly assume so, because the page is mapped via a PTE, which is
writable and dirty. This is similar to THP logic.

> - using set_pte_at() here seems a bit dangerous, as I'm not sure if this will
>   always only operate on invalid PTEs. Using it on active valid PTEs could
>   result in TLB issues because of missing flush. Also not sure about kvm impact.
>   Using ptep_set_access_flags() seems safer, again similar to touch_pmd() and
>   also cow_user_page().

Yeah, I sticked to what follow_pfn_pte() does for simplicity for now.
But I agree that following what touch_pmd() does looks saner --
ptep_set_access_flags().

> 
> Looking at cow_user_page(), I also wonder if the arch_faults_on_old_pte()
> logic could be used here. I must admit that I did not really understand the
> "losing the dirty bit" part of the comment, but it seems that we might need
> to not only check for arch_faults_on_old_pte(), but also for something like
> "arch_faults_for_dirty_pte".
> 
> Last but not least, IIUC, this issue should affect all archs that return
> true on arch_faults_on_old_pte(). After all, the basic problem seems to be
> that a pagefault is required for PTEs marked as old, in combination with
> GUP still returning a valid page. So maybe this should not be restricted
> to IS_ENABLED(CONFIG_S390).

Yeah, as raised, the IS_ENABLED(CONFIG_S390) part is just a quick hack
to see if this would fix the issue.

arch_faults_on_dirty_pte / arch_faults_on_old_pte might be a
replacement. We just would have to be careful for architectures that
e.g., have arch_faults_on_old_pte=true and
arch_faults_on_dirty_pte=false (i.e., hw dirty bit but no hw accessed
bit). Would have to think about how to handle that properly ...

Thanks!

-- 
Thanks,

David / dhildenb