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

[Jens Axboe <axboe@kernel.dk>]

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.

>> diff --git a/mm/swap.c b/mm/swap.c
>> index 835bdf324b76..f2457acae383 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
>>   */
>>  void folio_mark_accessed(struct folio *folio)
>>  {
>> +	if (folio_test_uncached(folio))
>> +		return;
> 
> 	if (folio_test_uncached(folio)) {
> 		if (folio_mapped(folio))
> 			folio_clear_uncached(folio);
> 		else
> 			return;
> 	}

Noted, thanks!

-- 
Jens Axboe