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

[Yu Kuai <yukuai3@huawei.com>]

在 2022/06/12 1:42, 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 ;-)

Yes, I confirmed that with this patch, small sequential read will mark
page accessed. However, multi-page folio is not tested yet.

> 
>> 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.

You are right, I didn't think of that situation.
> 
> 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.

Sure, thanks for your explanation.