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 AD15AD3ABF3 for ; Mon, 11 Nov 2024 19:10:41 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 215586B0095; Mon, 11 Nov 2024 14:10:41 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 1C55C6B0098; Mon, 11 Nov 2024 14:10:41 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 0669F6B0099; Mon, 11 Nov 2024 14:10:41 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id DCD6B6B0095 for ; Mon, 11 Nov 2024 14:10:40 -0500 (EST) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id 83190417C8 for ; Mon, 11 Nov 2024 19:10:40 +0000 (UTC) X-FDA: 82774753572.09.27A7F51 Received: from mail-yw1-f171.google.com (mail-yw1-f171.google.com [209.85.128.171]) by imf01.hostedemail.com (Postfix) with ESMTP id EEBB04001C for ; Mon, 11 Nov 2024 19:10:06 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SeJTDC7Z; spf=pass (imf01.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.128.171 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1731352151; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=IOpvGE4R4YUIYnuR/EnaJKnSdsQZQjG6B6aPhLJX7WU=; b=TEs5jLdQ8alynhd14h5Cdt8BYL8r+CPDyzZsZhwReN//rXFiXCeZE+3JsqPneTrDTPd8dA Ku+EVB6IWiur9LQtkcnosiicTX/ZG6XOC15Q8igJBA9HJxnnaT63wB0YyyVgAypyHJ9J/h Vjb0FfJYOdDVmBrkz88po3AS1u67QMk= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1731352151; a=rsa-sha256; cv=none; b=3A4MsThDyR1pwp+jXqtSDeL20J7KUB1xISCtcXZghLi4IKbrNPE5OfV6owYGvcQWQM1o3t Woi5ZBdOIaAAfwaVAOuGUrDAQ6Dk56S+umEBailtjGQXgEJH0TbW7tpZtxMJvT7tFz0jbJ NDz5J8frvx1GbTsG06fOA5SDODsEWKg= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=SeJTDC7Z; spf=pass (imf01.hostedemail.com: domain of laoar.shao@gmail.com designates 209.85.128.171 as permitted sender) smtp.mailfrom=laoar.shao@gmail.com; dmarc=pass (policy=none) header.from=gmail.com Received: by mail-yw1-f171.google.com with SMTP id 00721157ae682-6ea339a41f1so40166187b3.2 for ; Mon, 11 Nov 2024 11:10:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1731352238; x=1731957038; darn=kvack.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=IOpvGE4R4YUIYnuR/EnaJKnSdsQZQjG6B6aPhLJX7WU=; b=SeJTDC7ZDHMhaXLYKS1U2ZnOMdQCY6T1mzaQw42ivyQf6bS7zWJY+BYkQWb2ora6+K 35nN9UAUAcifd4qBrqOSNb8fQkca3wFkw6lufIaCcqqWNMPXNyei2UNV65uIcmvBayb8 qOccAwHYdwWPyF1PZI+jzKAwnTefJw11e8Xnjpb9KUnnhgFMW54Rp7k1KU4PVDkxK+Iu pbI3ZO2plav17X2ir0BewCeiImrIyhto6+k7J5r7V4HQPzjF47sTna4Q1IJj33knBgTa ffcoPLEMD1oujtTr+FV9IJc/l98s6bG8534sj9y7gvVVL+MhCuhIXUAoBC3ez/RtDRmC E7BA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1731352238; x=1731957038; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=IOpvGE4R4YUIYnuR/EnaJKnSdsQZQjG6B6aPhLJX7WU=; b=LNwdHuwwbnCpR/PUi7QR1jj8zo5BrvRBSWs6i61h/cY33zlJyyVL3QwJ0n2LRPpddj tLjutD8+F+baLnSepJmur0XMYbxiwBWBErkAqt3vhLElPJ1ecjLldCmSBBUQ/JgniEJO v93/B8RdglkeI1zv0pnMqDt5xldI4B69puw73QB4TG7I5PunV2FyqVqotAYmTS/a1eTX +mhvPAigeOFumGSMEd9ES6X07RLL6fRi14yADH6yxAjH5WLR/4lauyxoZeLIFfLH6PoJ IsImqHSCIY7QPuTigVW/I4FpG5Pj+Rr4Fu1ZnitOVQ3OHo+YAAFFgIBbZqn/l7Brn3A1 ZXNA== X-Forwarded-Encrypted: i=1; AJvYcCUomBr+K7fC4vFlGKc1BgV1mhk3MAN0HkIzkwoe1ZxIknk4FLHT3a8TdDcP3RB2GwQz0713pvq+hQ==@kvack.org X-Gm-Message-State: AOJu0Ywbx/ij8V7zsbvxrzjQ8kDr285qoMRzwQN8YGmmLmwAQ2uuEBta 6I+5Jj2c+APR0A1zDeH0m63NPg+vgedOtdzgrEnTuy+R5CT1ryiuKAprfQGgzELNhT0kp3VmFpg 3RWn+CdVtZwvMRCD8fLB5EOgF4o0= X-Google-Smtp-Source: AGHT+IGIJ6eNd9YSxv4NJT08aD6CqtFKIPZ0iX854Fch+qxeGA/rBryQz21EMssmAZrYV4sVjqtv8tUrf8V5AOqchsQ= X-Received: by 2002:a05:690c:6413:b0:6e3:14b0:ff86 with SMTP id 00721157ae682-6eadde5b02fmr127539757b3.27.1731352237744; Mon, 11 Nov 2024 11:10:37 -0800 (PST) MIME-Version: 1.0 References: <20241108141710.9721-1-laoar.shao@gmail.com> <85cfc467-320f-4388-b027-2cbad85dfbed@redhat.com> In-Reply-To: From: Yafang Shao Date: Tue, 12 Nov 2024 03:10:01 +0800 Message-ID: Subject: Re: [PATCH v2] mm/readahead: Fix large folio support in async readahead To: David Hildenbrand Cc: willy@infradead.org, akpm@linux-foundation.org, linux-mm@kvack.org, stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam10 X-Stat-Signature: apmdgn68jysab9r1kso8mpz8j8zqwyyn X-Rspamd-Queue-Id: EEBB04001C X-Rspam-User: X-HE-Tag: 1731352206-259063 X-HE-Meta: U2FsdGVkX18hcqVJRhtV1+esJqGOwVOjUX61lz3JYefAhFWFH1vwya6luy0yY/oT2/YM3a6Y4HHvnU3Djab42RqdsCLqQuyM0JJHE3x3NOQUse3EdD1BYi8TsPN2M1Qh2ElnDKgXZMiSE41ap5iIfBjcAyAlDeLzNjBT1PMUpzaUfGdcj7TdR5fUnxeRv1SFGH+3LYHCgGLjlfJ4MlW+DnDiuxxJWgBq+yFWxSdCHzJKbOQl3j5KrE4Y4vvvT3D8wfLgsu1j0nTb/HWHasQc0LrSfmOxtYeZ1PoxTa+clKQLIzsZO6ceCecbIcjmCLLgrvYtR3d0pRyege8dL2DmnkXdaXyOA3Z3JTEuDBFiyOfFugxaMs7ZMjJjuKxyGnyx15XKZy1gb6gdBOBKsmSvjYnY7lM2UbrgVq5N5iikhCzjfmYPu85zc9puBSyp4SNXcm/uLGsc9Is7lCX2x3P2WykAAQLV8s3WsT/56b7tHXDLBKQKznmdXigkNvFpHtlzg+lPK/TGnRkBFRD3MKgrXZ2dWKiSebtdXhnQiobxDIkP1NzRCBzd80MzHeZIkHsuoHUADu56Ppl4bsSYdMd9gZyhEHdvhQAou8+3hqXcZK/WGIdP/9YS4a1mFGgi/y9eM1PBBaBTFqaSv9+HBKKfXHiHKstZqNpIqclJfX/LW0lEf+0DojncHqXLy6Hbyivua2xWHpReOzIWvHR1yaw9yEYFCQyz9Rbe2o6XUP9a9w2iqdRy3f9Aq9+mYEYWTikuXP4twJwpMiJE1R9CZiNtM4KpyuDU3KZ785/R3YQMvL1GM/XPae8oPgIh6zGo1X5j6tonKTX3f4RE+PKNAfvvtkLg9Lr2KukajAwPeQRKyGcKfuGm82T59cQ8N+lAHjgkxY2oaFJ/YsE7tsn+ljcT5EEZ35Lm5aQS5wSi+zKw7szjx5bfhRPYmawKO006ZN+yVuuYXsopHPz8gEl+Zna Ls2z0SdC Q7eDEglryY9t9Tq/mj99foc78Lcc6Bjy9pRQ1EaYUDGjSTobrqSO/3E/Or3/VT4932aPv+L7HbVghO/ntYqGdzaRwXIPqxpDk05FPOmLqigopO/U1PtRwJFavTdKGDeou6TX54aSm+fyMIRCWaehKuZE7tvRSsZmRm/bm5NR2bsBEh8FszdBax3tXEZ2EF3cNRCdA7f6k6aVYcc012fOdiBRQXy9aXtW+GmWHlL8jke59nM6JZ1GIQzf49FB2NN/MPKxXMFDFpl3FgcKuE4dMr/+viYjcdBPZT9H3yreIxARXPegE0VYWhxIlI3IZBc5FuQiOJ9ssWulj5j32LaslVQk5BzojpeNw1Q10spDzP6W9Vb4lH4acBilPkQ== 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: List-Subscribe: List-Unsubscribe: On Tue, Nov 12, 2024 at 2:31=E2=80=AFAM David Hildenbrand wrote: > > On 11.11.24 17:08, Yafang Shao wrote: > > On Mon, Nov 11, 2024 at 11:05=E2=80=AFPM David Hildenbrand wrote: > >> > >> On 11.11.24 15:28, Yafang Shao wrote: > >>> On Mon, Nov 11, 2024 at 6:33=E2=80=AFPM David Hildenbrand wrote: > >>>> > >>>> On 08.11.24 15:17, Yafang Shao wrote: > >>>>> When testing large folio support with XFS on our servers, we observ= ed that > >>>>> only a few large folios are mapped when reading large files via mma= p. > >>>>> After a thorough analysis, I identified it was caused by the > >>>>> `/sys/block/*/queue/read_ahead_kb` setting. On our test servers, th= is > >>>>> parameter is set to 128KB. After I tune it to 2MB, the large folio = can > >>>>> work as expected. However, I believe the large folio behavior shoul= d not be > >>>>> dependent on the value of read_ahead_kb. It would be more robust if= the > >>>>> kernel can automatically adopt to it. > >>>> > >>>> Now I am extremely confused. > >>>> > >>>> Documentation/ABI/stable/sysfs-block: > >>>> > >>>> "[RW] Maximum number of kilobytes to read-ahead for filesystems on t= his > >>>> block device." > >>>> > >>>> > >>>> So, with your patch, will we also be changing the readahead size to > >>>> exceed that, or simply allocate larger folios and not exceeding the > >>>> readahead size (e.g., leaving them partially non-filled)? > >>> > >>> Exceeding the readahead size for the MADV_HUGEPAGE case is > >>> straightforward; this is what the current patch accomplishes. > >>> > >> > >> Okay, so this only applies with MADV_HUGEPAGE I assume. Likely we shou= ld > >> also make that clearer in the subject. > >> > >> mm/readahead: allow exceeding configured read_ahead_kb with MADV_HUGEP= AGE > >> > >> > >> If this is really a fix, especially one that deserves CC-stable, I > >> cannot tell. Willy is the obvious expert :) > >> > >>>> > >>>> If you're also changing the readahead behavior to exceed the > >>>> configuration parameter it would sound to me like "I am pushing the > >>>> brake pedal and my care brakes; fix the brakes to adopt whether to b= rake > >>>> automatically" :) > >>>> > >>>> Likely I am missing something here, and how the read_ahead_kb parame= ter > >>>> is used after your patch. > >>> > >>> The read_ahead_kb parameter continues to function for > >>> non-MADV_HUGEPAGE scenarios, whereas special handling is required for > >>> the MADV_HUGEPAGE case. It appears that we ought to update the > >>> Documentation/ABI/stable/sysfs-block to reflect the changes related t= o > >>> large folios, correct? > >> > >> Yes, how it related to MADV_HUGEPAGE. I would assume that it would get > >> ignored, but ... > >> > >> ... staring at get_next_ra_size(), it's not quite ignored, because we > >> still us it as a baseline to detect how much we want to bump up the > >> limit when the requested size is small? (*2 vs *4 etc) :/ > >> > >> So the semantics are really starting to get weird, unless I am missing > >> something important. > >> > >> [...] > >> > >>> Perhaps a more straightforward solution would be to implement it > >>> directly at the callsite, as demonstrated below? > >> > >> Likely something into this direction might be better, but Willy is the > >> expert that code. > >> > >>> > >>> diff --git a/mm/readahead.c b/mm/readahead.c > >>> index 3dc6c7a128dd..187efae95b02 100644 > >>> --- a/mm/readahead.c > >>> +++ b/mm/readahead.c > >>> @@ -642,7 +642,11 @@ void page_cache_async_ra(struct readahead_contro= l *ractl, > >>> 1UL << order); > >>> if (index =3D=3D expected) { > >>> ra->start +=3D ra->size; > >>> - ra->size =3D get_next_ra_size(ra, max_pages); > >>> + /* > >>> + * Allow the actual size to exceed the readahead wind= ow for a > >>> + * large folio. > >> > >> "a large folio" -> "with MADV_HUGEPAGE" ? Or can this be hit on > >> different paths that are not covered in the patch description? > > > > This branch may also be triggered by other large folios that are not > > necessarily order-9. Therefore, I=E2=80=99ve referred to it as a 'large= folio' > > rather than associating it specifically with MADV_HUGEPAGE. If we were > > to handle only the MADV_HUGEPAGE case, we would proceed as outlined in > > the initial RFC patch[0]. However, following Willy's recommendation, I > > implemented it this way, as he likely has a deeper understanding of > > the intended behavior. > > Sorry, but this code is getting quite confusing, especially with such > misleading "large folio" comments. > > Even without MADV_HUGEPAGE we will be allocating large folios, as > emphasized by Willy [1]. So the only thing MADV_HUGEPAGE controls is > *which* large folios we allocate. .. as Willy says [2]: "We were only > intending to breach the 'max' for the MADV_HUGE case, not for all cases." > > I have no idea how *anybody* should derive from the code here that we > treat MADV_HUGEPAGE in a special way. > > Simply completely confusing. > > My interpretation of "I don't know if we should try to defend a stupid > sysadmin against the consequences of their misconfiguration like this" > means" would be "drop this patch and don't change anything". Without this change, large folios won=E2=80=99t function as expected. Currently, to support MADV_HUGEPAGE, you=E2=80=99d need to set readahead_kb= to 2MB, 4MB, or more. However, many applications run without MADV_HUGEPAGE, and a larger readahead_kb might not be optimal for them. > > No changes to API, no confusing code. New features like large folios can often create confusion with existing rules or APIs, correct? > > Maybe pr_info_once() when someone uses MADV_HUGEPAGE with such backends > to tell the sysadmin that something stupid is happening ... It's not a flawed setup; it's just that this new feature doesn=E2=80=99t wo= rk well with the existing settings, and updating those settings to accommodate it isn't always feasible. --=20 Regards Yafang