Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
References: <20220607083714.183788-1-apopple@nvidia.com>
 <Yp9aQk66fkP8MdOS@casper.infradead.org>
User-agent: mu4e 1.6.9; emacs 27.1
From:   Alistair Popple <apopple@nvidia.com>
To:     Matthew Wilcox <willy@infradead.org>
Cc:     akpm@linux-foundation.org, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm/filemap.c: Always read one page in
 do_sync_mmap_readahead()
Date:   Wed, 08 Jun 2022 16:35:16 +1000
In-reply-to: <Yp9aQk66fkP8MdOS@casper.infradead.org>
Message-ID: <87sfof8x1w.fsf@nvdebian.thelocal>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?P1d651l75MrKYJwo9ZzQEhSpQAd3Hij+0THbKxzrkw7JIDYNNd3NbhUPQd1F?=
 =?us-ascii?Q?4Kvr20iwKha6b2UnWKlLIR2DZi9sHOZkqkj8Np0oks5SSsOXd1ic65oDnQZq?=
 =?us-ascii?Q?Gq7CLjjWgSCANemC3piL5/ZXuZSRrxUGeoqzPWbkLGV0dwINp/MXj0BRj2eb?=
 =?us-ascii?Q?+Eq0CwqVEaBw2/wphZhcVofCRn/ABGtDhW3y6IxMKegEEf9WBnSgZaRahamI?=
 =?us-ascii?Q?jskWbXThl4vVNd0bnvBDHOGICPAmzQ5YB0emZwbYDnCLaD7spP4G48zx7G/c?=
 =?us-ascii?Q?R44ed9jHNurbOgXPHSwVv03V0wAoFjEZ/pE3n1vJzwWOHUtnZh5iexiTUdH4?=
 =?us-ascii?Q?w3BaS9H+WepxLKb5dulQXngnEFWF1coxfsbwxAy18EWUp2Eh3wzYGHeFTG2h?=
 =?us-ascii?Q?Jg+Ck1jqC9+wamrE3jI4P6+JhsOQPJiTubx7KbiDz/d0AXOYqvGW6eZVSQXx?=
 =?us-ascii?Q?2JWEUhuN22RiazTVPrZUT+CiuRRw5KoJUvACHzB2pe6Au8pNi0SxDPY17pLv?=
 =?us-ascii?Q?nKP05JLSr4SG2YSzhS09+TRBG449fRsfq/X4EHnU5LvLi5KrP2CipMKNBnDX?=
 =?us-ascii?Q?hfJnmC+9SY0AekXn9dBd6HPYOlQjUJCKba7cito237ekTb3f75xmnYdzK/Yh?=
 =?us-ascii?Q?RtvJwyT+UJroUfna1JHVqegEi5RJOYeJX/mbhkvkkSlvjEFkVmry7AAJTZ3D?=
 =?us-ascii?Q?zJcwGoKrgjgPbsCgF76yLZC8wggR7zXEQSsOx1xX1hnIFrNxQy9Z6AThIurH?=
 =?us-ascii?Q?Ug9x0S+c8h2FZ6gkIcJRQ2dD067A2+QvwF1HWF/X+ZN+3i+Zx1Ml4gdEFSgn?=
 =?us-ascii?Q?Fe9MZrULxjiTCmoR3xNb3AqnSvpqHRe/9Y5yvMlaLO0nUo7oyq1h1LnFqNCF?=
 =?us-ascii?Q?gyCzadmsVEnDbPzaWyfmH05qWvP6rx9lnJrM0yH3UaoYYb49i8eJ/Jg98lTn?=
 =?us-ascii?Q?MLoTkx3ZqGhDKBw4a/NtwtO4oKRMZh66caiEKVDSrXlagmW/XEO65QC+oPFr?=
 =?us-ascii?Q?j0GmVMOpd8bly0BBDrGsAdH5JkXxLZmtgty/3kWb5XzBnr4z7w3BnllGDauI?=
 =?us-ascii?Q?Cq2gIpG/8EmT9CYB55HxRsHGWbW09DFYZqNVLfFsL1WCxLGQpiUtdqO6LGvm?=
 =?us-ascii?Q?PHf1N2Zg2oFQGMoO32K36y3XlEv5DNPu/SBMlKGnJqAI7BmRWYAV3XfsaUXJ?=
 =?us-ascii?Q?m1qnzJ2xNvYFFL+OfMjU9nlrLi0Xrk6O56hnELF6xkxvz8Tbj/esXiXIaL6Q?=
 =?us-ascii?Q?w+nWL4iz+IlB8t8vvC2EhcqgWMdwhFgGQRY/Ub7LCvLoGrw8KEv/lZqsA6vt?=
 =?us-ascii?Q?P2LfStKfRGVuBTLIOfDp1AtEU4C6Y82VssumpZHf+pdvPzTni7YcCi14TUtK?=
 =?us-ascii?Q?q6cOjKgO8FkmzHwyYjOFkQ+mbvteRG+LAFCRimwff1aHDcZgpVqy8oHYgnLr?=
 =?us-ascii?Q?Y+xwYkbNTi57AOkrTWDbEy9185undmGMqckFPazhJRTekPLk8NWpfRyRWIay?=
 =?us-ascii?Q?kO39eR4L9K92GMwo7SgLOMtcysYUYf/0Boqa3mcvgel0J+3/60P1oFX3F5GK?=
 =?us-ascii?Q?/vhogktHsP3lHV3BfU4AlzDAblN1kiR/9rjy6JeyNz8fKnIRYzuFMYzDsh3q?=
 =?us-ascii?Q?FskQdMXoO1q8AV06xCsqI9EJlx/LTD/LoJIkGyPX7K7tqsDCDWVA2onwXv5J?=
 =?us-ascii?Q?w5MiOHa1+zRJG69uZVktROAXzE9s0b6V0wWsfH9+sYXYbWWli417bv5iOrXK?=
 =?us-ascii?Q?jfEYmjPFtw=3D=3D?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1b0415a5-a8ad-4b12-50dc-08da491bc200
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Jun 2022 06:54:39.5497
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NycnqZKlJmD6Lg1q2FW7bcJUH4z9GqV4XlPJKpbi3+Y8FfebmgWqVKFJ/yq8NUC9GYQ4iFrnqYSENOAmf8pmeQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR12MB5931
X-Spam-Status: No, score=-3.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
	DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


Matthew Wilcox <willy@infradead.org> writes:

> On Tue, Jun 07, 2022 at 06:37:14PM +1000, Alistair Popple wrote:
>> ---
>>  include/linux/pagemap.h |  7 +++---
>>  mm/filemap.c            | 47 +++++++++++++----------------------------
>>  2 files changed, 18 insertions(+), 36 deletions(-)
>
> Love the diffstat ;-)
>
>> @@ -3011,14 +3001,8 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>>  	}
>>  #endif
>>
>> -	/* If we don't want any read-ahead, don't bother */
>> -	if (vmf->vma->vm_flags & VM_RAND_READ)
>> -		return fpin;
>> -	if (!ra->ra_pages)
>> -		return fpin;
>> -
>> +	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>  	if (vmf->vma->vm_flags & VM_SEQ_READ) {
>> -		fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>>  		page_cache_sync_ra(&ractl, ra->ra_pages);
>>  		return fpin;
>>  	}
>
> Good.  Could even pull the maybe_unlock_mmap_for_io() all the way to the
> top of the file and remove it from the VM_HUGEPAGE case?

Good idea. Also while I'm here is there a reason we don't update
ra->start or mmap_miss for the VM_HUGEPAGE case?

>> @@ -3029,19 +3013,20 @@ static struct file *do_sync_mmap_readahead(struct vm_fault *vmf)
>>  		WRITE_ONCE(ra->mmap_miss, ++mmap_miss);
>>
>>  	/*
>> -	 * Do we miss much more than hit in this file? If so,
>> -	 * stop bothering with read-ahead. It will only hurt.
>> +	 * mmap read-around. If we don't want any read-ahead or if we miss more
>> +	 * than we hit don't bother with read-ahead and just read a single page.
>>  	 */
>> -	if (mmap_miss > MMAP_LOTSAMISS)
>> -		return fpin;
>> +	if ((vmf->vma->vm_flags & VM_RAND_READ) ||
>> +	    !ra->ra_pages || mmap_miss > MMAP_LOTSAMISS) {
>> +		ra->start = vmf->pgoff;
>> +		ra->size = 1;
>> +		ra->async_size = 0;
>> +	} else {
>
> I'd put the:
> 		/* mmap read-around */
> here
>
>> +		ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2);
>> +		ra->size = ra->ra_pages;
>> +		ra->async_size = ra->ra_pages / 4;
>> +	}
>>
>> -	/*
>> -	 * mmap read-around
>> -	 */
>> -	fpin = maybe_unlock_mmap_for_io(vmf, fpin);
>> -	ra->start = max_t(long, 0, vmf->pgoff - ra->ra_pages / 2);
>> -	ra->size = ra->ra_pages;
>> -	ra->async_size = ra->ra_pages / 4;
>>  	ractl._index = ra->start;
>>  	page_cache_ra_order(&ractl, ra, 0);
>>  	return fpin;
>> @@ -3145,9 +3130,7 @@ vm_fault_t filemap_fault(struct vm_fault *vmf)
>>  			filemap_invalidate_lock_shared(mapping);
>>  			mapping_locked = true;
>>  		}
>> -		folio = __filemap_get_folio(mapping, index,
>> -					  FGP_CREAT|FGP_FOR_MMAP,
>> -					  vmf->gfp_mask);
>> +		folio = filemap_get_folio(mapping, index);
>>  		if (!folio) {
>>  			if (fpin)
>>  				goto out_retry;
>
> I think we also should remove the filemap_invalidate_lock_shared()
> here, no?

Right, afaik filemap_invalidate_lock_shared() is needed when
instantiating pages in the page cache during fault, which this patch
does via page_cache_ra_order() in do_sync_mmap_readahead() so I think
you're right about removing it for filemap_get_folio().

However do_sync_mmap_readahead() is the way normal (ie. !VM_RAND_READ)
pages would get instantiated today. So shouldn't
filemap_invalidate_lock_shared() be called before
do_sync_mmap_readahead() anyway? Or am I missing something?

> We also need to handle the !folio case differently.  Before, if it was
> gone, that was definitely an OOM.  Now if it's gone it might have been
> truncated, or removed due to memory pressure, or it might be an OOM
> situation where readahead didn't manage to create the folio.

Good point, thanks for catching that.