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

[Kirill A. Shutemov]

On Mon, Nov 11, 2024 at 08:57:17AM -0700, Jens Axboe wrote:
> 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.

I suggested BUG_ON() because current caller has it. But, yeah, WARN() is
good enough.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov