在 2022/06/11 2:34, Matthew Wilcox 写道:
On Fri, Jun 10, 2022 at 01:47:02PM -0400, Kent Overstreet wrote:I think this is the fix we want - I think Yu basically had the right idea and had the off by one fix, this should be clearer though: Yu, can you confirm the fix? -- >8 -- Subject: [PATCH] filemap: Fix off by one error when marking folios accessed In filemap_read() we mark pages accessed as we read them - but we don't want to do so redundantly, if the previous read already did so. But there was an off by one error: we want to check if the current page was the same as the last page we read from, but the last page we read from was (ra->prev_pos - 1) >> PAGE_SHIFT. Reported-by: Yu Kuai <yukuai3@xxxxxxxxxx> Signed-off-by: Kent Overstreet <kent.overstreet@xxxxxxxxx> diff --git a/mm/filemap.c b/mm/filemap.c index 9daeaab360..8d5c8043cb 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2704,7 +2704,7 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, * mark it as accessed the first time. */ if (iocb->ki_pos >> PAGE_SHIFT != - ra->prev_pos >> PAGE_SHIFT) + (ra->prev_pos - 1) >> PAGE_SHIFT) folio_mark_accessed(fbatch.folios[0]); for (i = 0; i < folio_batch_count(&fbatch); i++) {This is going to mark the folio as accessed multiple times if it's a multi-page folio. How about this one?
Hi, Matthew
Thanks for the patch, it looks good to me.
BTW, I still think the fix should be commit 06c0444290ce ("mm/filemap.c:
generic_file_buffered_read() now uses find_get_pages_contig").
diff --git a/mm/filemap.c b/mm/filemap.c index 5f227b5420d7..a30587f2e598 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -2599,6 +2599,13 @@ static int filemap_get_pages(struct kiocb *iocb, struct iov_iter *iter, return err; }+static inline bool pos_same_folio(loff_t pos1, loff_t pos2, struct folio *folio)+{ + unsigned int shift = folio_shift(folio); + + return (pos1 >> shift == pos2 >> shift); +} + /** * filemap_read - Read data from the page cache. * @iocb: The iocb to read. @@ -2670,11 +2677,11 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter, writably_mapped = mapping_writably_mapped(mapping);/*- * When a sequential read accesses a page several times, only + * When a read accesses the same folio several times, only * mark it as accessed the first time. */ - if (iocb->ki_pos >> PAGE_SHIFT != - ra->prev_pos >> PAGE_SHIFT) + if (!pos_same_folio(iocb->ki_pos, ra->prev_pos - 1, + fbatch.folios[0])) folio_mark_accessed(fbatch.folios[0]);for (i = 0; i < folio_batch_count(&fbatch); i++) {.
