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.
Putting the check in __filemap_get_folio() sounds reasonable.
But I am not 100% sure it would be enough to never get PG_uncached mapped.
Will think about it more.
Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
invalidate_complete_folio2().
--
Kiryl Shutsemau / Kirill A. Shutemov
