On Sun 01-10-23 00:10:29, Lorenzo Stoakes wrote:
> The existing comments in filemap_fault() suggest that, after either a minor
> fault has occurred and filemap_get_folio() found a folio in the page cache,
> or a major fault arose and __filemap_get_folio(FGP_CREATE...) did the job
> (having relied on do_sync_mmap_readahead() or filemap_read_folio() to read
> in the folio), the only possible reason it could not be uptodate is because
> of an error.
>
> This is not so, as if, for instance, the fault occurred within a VMA which
> had the VM_RAND_READ flag set (via madvise() with the MADV_RANDOM flag
> specified), this would cause even synchronous readahead to fail to read in
> the folio.
>
> I confirmed this by dropping page caches and faulting in memory madvise()'d
> this way, observing that this code path was reached on each occasion.
>
> Clarify the comments to include this case, and additionally update the
> comment recently added around the invalidate lock logic to make it clear
> the comment explicitly refers to the minor fault case.
>
> In addition, while we're here, refer to folios rather than pages.
>
> Signed-off-by: Lorenzo Stoakes <lstoakes@xxxxxxxxx>
After the alignment fixup the patch looks good to me. Feel free to add:
Reviewed-by: Jan Kara <jack@xxxxxxx>
Honza
> ---
> mm/filemap.c | 19 +++++++++++++------
> 1 file changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/mm/filemap.c b/mm/filemap.c
> index d285ec5f9162..959694a2ade7 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -3308,21 +3308,28 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
> VM_BUG_ON_FOLIO(!folio_contains(folio, index), folio);
>
> /*
> - * We have a locked page in the page cache, now we need to check
> - * that it's up-to-date. If not, it is going to be due to an error.
> + * We have a locked folio in the page cache, now we need to check
> + * that it's up-to-date. If not, it is going to be due to an error,
> + * or because readahead was otherwise unable to retrieve it.
> */
> if (unlikely(!folio_test_uptodate(folio))) {
> /*
> - * The page was in cache and uptodate and now it is not.
> - * Strange but possible since we didn't hold the page lock all
> - * the time. Let's drop everything get the invalidate lock and
> - * try again.
> + * If the invalidate lock is not held, the folio was in cache and
> + * uptodate and now it is not. Strange but possible since we
> + * didn't hold the page lock all the time. Let's drop everything,
> + * get the invalidate lock and try again.
> */
> if (!mapping_locked) {
> folio_unlock(folio);
> folio_put(folio);
> goto retry_find;
> }
> +
> + /*
> + * OK, the folio is really not uptodate. This can be because the
> + * VMA has the VM_RAND_READ flag set, or because an error
> + * arose. Let's read it in directly.
> + */
> goto page_not_uptodate;
> }
>
> --
> 2.42.0
>
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
