linux-kernel - Re: [PATCH -next] mm/filemap: fix that first page is not mark accessed in filemap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 13 Jun 2022 09:31:11 +0800
From:   Yu Kuai <yukuai3@...wei.com>
To:     Matthew Wilcox <willy@...radead.org>
CC:     Kent Overstreet <kent.overstreet@...il.com>,
        <akpm@...ux-foundation.org>, <axboe@...nel.dk>,
        <linux-fsdevel@...r.kernel.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>, <yi.zhang@...wei.com>
Subject: Re: [PATCH -next] mm/filemap: fix that first page is not mark
 accessed in filemap_read()

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