On 11/11/24 8:51 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
>> 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.
>
> The point is to leave the folio in page cache if there's a
> non-RWF_UNCACHED user of it.
Right. The uncached flag should be ephemeral, hitting it should be
relatively rare. But if it does happen, yeah we should leave the page in
cache.
> Putting the check in __filemap_get_folio() sounds reasonable.
OK will do.
> But I am not 100% sure it would be enough to never get PG_uncached mapped.
> Will think about it more.
Thanks!
> Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
> invalidate_complete_folio2().
Isn't that a bit rough? Maybe just a:
if (WARN_ON_ONCE(folio_mapped(folio)))
return;
would do? I'm happy to do either one, let me know what you prefer.
--
Jens Axboe
