Re: [PATCH -next] mm/filemap: fix that first page is not mark accessed in filemap_read()

[Matthew Wilcox]

On Sat, Jun 11, 2022 at 04:23:42PM +0800, Yu Kuai wrote:
> > 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.

Did you test it?  This is clearly a little subtle ;-)

> BTW, I still think the fix should be commit 06c0444290ce ("mm/filemap.c:
> generic_file_buffered_read() now uses find_get_pages_contig").

Hmm, yes.  That code also has problems, but they're more subtle and
probably don't amount to much.

-       iocb->ki_pos += copied;
-
-       /*
-        * When a sequential read accesses a page several times,
-        * only mark it as accessed the first time.
-        */
-       if (iocb->ki_pos >> PAGE_SHIFT != ra->prev_pos >> PAGE_SHIFT)
-               mark_page_accessed(page);
-
-       ra->prev_pos = iocb->ki_pos;

This will mark the page accessed when we _exit_ a page.  So reading
512-bytes at a time from offset 0, we'll mark page 0 as accessed on the
first read (because the prev_pos is initialised to -1).  Then on the
eighth read, we'll mark page 0 as accessed again (because ki_pos will
now be 4096 and prev_pos is 3584).  We'll then read chunks of page 1
without marking it as accessed, until we're about to step into page 2.

Marking page 0 accessed twice is bad; it'll set the referenced bit the
first time, and then the second time, it'll activate it.  So it'll be
thought to be part of the workingset when it's really just been part of
a streaming read.

And the last page we read will never be marked accessed unless it
happens to finish at the end of a page.

Before Kent started his refactoring, I think it worked:

-       pgoff_t prev_index;
-       unsigned int prev_offset;
...
-       prev_index = ra->prev_pos >> PAGE_SHIFT;
-       prev_offset = ra->prev_pos & (PAGE_SIZE-1);
...
-               if (prev_index != index || offset != prev_offset)
-                       mark_page_accessed(page);
-               prev_index = index;
-               prev_offset = offset;
...
-       ra->prev_pos = prev_index;
-       ra->prev_pos <<= PAGE_SHIFT;
-       ra->prev_pos |= prev_offset;

At least, I don't detect any bugs in this.