Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Jens Axboe]

On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>>>  			}
>>>>  		}
>>>>  put_folios:
>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>>>> -			folio_put(fbatch.folios[i]);
>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>>>> +			struct folio *folio = fbatch.folios[i];
>>>> +
>>>> +			if (folio_test_uncached(folio)) {
>>>> +				folio_lock(folio);
>>>> +				invalidate_complete_folio2(mapping, folio, 0);
>>>> +				folio_unlock(folio);
>>>
>>> I am not sure it is safe. What happens if it races with page fault?
>>>
>>> The only current caller of invalidate_complete_folio2() unmaps the folio
>>> explicitly before calling it. And folio lock prevents re-faulting.
>>>
>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
>>> also mark the page accessed.
>>
>> Ok thanks, let me take a look at that and create a test case that
>> exercises that explicitly.
> 
> It might be worth generalizing it to clearing PG_uncached for any page cache
> lookups that don't come from RWF_UNCACHED.

We can do that - you mean at lookup time? Eg have __filemap_get_folio()
do:

if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
	folio_clear_uncached(folio);

or do you want this logic just in filemap_read()? Arguably it should
already clear it in the quoted code above, regardless, eg:

	if (folio_test_uncached(folio)) {
		folio_lock(folio);
		invalidate_complete_folio2(mapping, folio, 0);
		folio_clear_uncached(folio);
		folio_unlock(folio);
	}

in case invalidation fails.

-- 
Jens Axboe