From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B1883C433EF for ; Sat, 11 Jun 2022 17:42:55 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AA1EB6B0151; Sat, 11 Jun 2022 13:42:54 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A51536B0153; Sat, 11 Jun 2022 13:42:54 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 918C78D0115; Sat, 11 Jun 2022 13:42:54 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0017.hostedemail.com [216.40.44.17]) by kanga.kvack.org (Postfix) with ESMTP id 7EAAC6B0151 for ; Sat, 11 Jun 2022 13:42:54 -0400 (EDT) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 4D33D34146 for ; Sat, 11 Jun 2022 17:42:54 +0000 (UTC) X-FDA: 79566675468.12.16A5248 Received: from casper.infradead.org (casper.infradead.org [90.155.50.34]) by imf10.hostedemail.com (Postfix) with ESMTP id C2754C0068 for ; Sat, 11 Jun 2022 17:42:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=casper.20170209; h=In-Reply-To:Content-Type:MIME-Version: References:Message-ID:Subject:Cc:To:From:Date:Sender:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description; bh=cy4LqclDCzXO19Ll6jpbh9YluwE5VQrU+JF2sYO2jE0=; b=IxeQ9knTGfuPFY2EQO3cFHURNk 8Aq729adAtGTjemhV30dzAco9EIMZh7j/WvantcPtZcAMhMNIPxKuaGYOXVzVodW8cCJuFcGPJvdz 3QhZzc4YjmenhkYU1RNPeSB/3NQAUJLLXclhkJLs4g8y3YZhRxIr2xqWTvvmDLCWSIwJSDGoO3DUa he3gxqAngeEH8UKmIFvz6Nw/8zGYCDk+w2n763osH8eOIwcEaK5uV3XAdM5oW25NaZpZG1a71zz9i jzRYnpIMmls8+HfSypjUz43b/PCTd3Y7GlL9fnHttL5rcxmTekuN0llIvQl7xeqnMDThcspXRnPnR mcBtAr5A==; Received: from willy by casper.infradead.org with local (Exim 4.94.2 #2 (Red Hat Linux)) id 1o058D-00FPYV-Al; Sat, 11 Jun 2022 17:42:41 +0000 Date: Sat, 11 Jun 2022 18:42:41 +0100 From: Matthew Wilcox To: Yu Kuai Cc: Kent Overstreet , akpm@linux-foundation.org, axboe@kernel.dk, linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org, yi.zhang@huawei.com Subject: Re: [PATCH -next] mm/filemap: fix that first page is not mark accessed in filemap_read() Message-ID: References: <20220602082129.2805890-1-yukuai3@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1654969373; a=rsa-sha256; cv=none; b=l312gwViAYV5dcHgQtroYztJvV/qyUmuxFwUcu1mYE74FZsxAXoQFJuP21DDabSSOpYeCE 3PmmtkcjKDLqR75RFneRTkUxGTt3aSvE8Ga2MA8G4yzXSc6aW6OJlbhmFTh+LNuMwj5DSf Pf/IVq4d7kxdS3SxvCpTrhRo2tPTKzs= ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=IxeQ9knT; spf=none (imf10.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1654969373; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=cy4LqclDCzXO19Ll6jpbh9YluwE5VQrU+JF2sYO2jE0=; b=ROAefpxbxXseUgwspHomQuvxAsz3OuRdh5KV30e+fmBOTVDFsKmZ6TonhSZtxlS5HmI5PG HeJa6YtYp3OCPDA/otRXB3+vvnw6NLJdfu9RPzplqP/xHbt94RPx3I/JTWsk4b81m5qRDT TdBZ8cJaMj84XiZBGZC12Q7yTjF7aDQ= X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: C2754C0068 X-Stat-Signature: msfdauuqkg7qu63sh15ug4w66obdptzt X-Rspam-User: Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=infradead.org header.s=casper.20170209 header.b=IxeQ9knT; spf=none (imf10.hostedemail.com: domain of willy@infradead.org has no SPF policy when checking 90.155.50.34) smtp.mailfrom=willy@infradead.org; dmarc=none X-HE-Tag: 1654969372-378087 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 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.