lkml.org 
[lkml]   [2022]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH -next] mm/filemap: fix that first page is not mark accessed in filemap_read()
From
Date
在 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.

\
 
 \ /
  Last update: 2022-06-13 03:32    [W:0.063 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site