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 7FF18C3DA4A for ; Thu, 8 Aug 2024 21:24:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 098136B009A; Thu, 8 Aug 2024 17:24:17 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 048766B009C; Thu, 8 Aug 2024 17:24:16 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E79256B009E; Thu, 8 Aug 2024 17:24:16 -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 C9CD56B009A for ; Thu, 8 Aug 2024 17:24:16 -0400 (EDT) Received: from smtpin07.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay08.hostedemail.com (Postfix) with ESMTP id 7181B140181 for ; Thu, 8 Aug 2024 21:24:16 +0000 (UTC) X-FDA: 82430356512.07.3448813 Received: from mail-pj1-f46.google.com (mail-pj1-f46.google.com [209.85.216.46]) by imf21.hostedemail.com (Postfix) with ESMTP id A6F0D1C0019 for ; Thu, 8 Aug 2024 21:24:14 +0000 (UTC) Authentication-Results: imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BZpPuGwd; spf=pass (imf21.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.46 as permitted sender) smtp.mailfrom=andrii.nakryiko@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=1723152222; 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=gEQ3fDRPE5tarteAmLXwRvAJxIkK+5tTVa+taW/N4OA=; b=XRhUQMF0exlhcJAbua17FMbaljlAlhdgXh7dlIaD1NXKxB54/WswIsFHV1lMSig93wLm8/ jdQ0f0onU7Fs/z6B8fUHoXnACPRNCLbqg+yTPkpo1SzikD0PjzDxpcH2pIo9gUFWbyuuxd KcWoxqPM+X31gMTZochqfaF5fU9evAc= ARC-Authentication-Results: i=1; imf21.hostedemail.com; dkim=pass header.d=gmail.com header.s=20230601 header.b=BZpPuGwd; spf=pass (imf21.hostedemail.com: domain of andrii.nakryiko@gmail.com designates 209.85.216.46 as permitted sender) smtp.mailfrom=andrii.nakryiko@gmail.com; dmarc=pass (policy=none) header.from=gmail.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1723152222; a=rsa-sha256; cv=none; b=ImreYWF6XWwA3cF2eZkAUs1ZwsNiZ3NTZ7rWiLqFl6fyTOEBUx4dQO6oKboBQ6c0mxXS0y Nt/mg71ZxITB94bm5gVhMc1YNJYXFeJt48sICgAs7ffMaqjbK5gi6MRmP+kuhYrPQNePE9 cIF/dk9aqNuXbUBX1yyM+eADQYUrLoo= Received: by mail-pj1-f46.google.com with SMTP id 98e67ed59e1d1-2caff99b1c9so1222860a91.3 for ; Thu, 08 Aug 2024 14:24:14 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1723152253; x=1723757053; 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=gEQ3fDRPE5tarteAmLXwRvAJxIkK+5tTVa+taW/N4OA=; b=BZpPuGwdGixsaoCy57/Tp2JEq0Nsd5yemFwYHPPZaKf+/sb79lSYCb2gPYocDNhkG2 +2XwNwWh+kznnThL7eHMEpNG1WQW4bm3lRku67IHPWmyyDR0sz71gi34J6Fh8+r0xgjv iLhzbSytRrEW+6yK1u5MNpPEfB5yO1ZfDS5hWHt/tN3QD+7Jwm+//vxZFt31ccFHOhC9 kOzRkxIOPydXLUlrOSpq9AqdlH4+D8HRiNxe0r90K68n0ihOaWOW5euFdppHZHcSuJCH 9ax1dYZOH257vgk83T/tp11RblYCDjj54A1eNR8HC+ZKGud7KeE3R0sfIcccH32FdKI5 VSCw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723152253; x=1723757053; 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=gEQ3fDRPE5tarteAmLXwRvAJxIkK+5tTVa+taW/N4OA=; b=bDJ2/v6DDiN3+osfccyfMpLWu+Y8qtEeuxFrlE6af8612kDUGcxAD7fGqjRSyn5Zez Tdgrziz5ZgWv5/DUFnZ+7+TJfJtWaqdvFIzfziZC2xjhpokAVpFMB2Yl0VTrMev0INv5 eKlHlE13DvmKGF02O8p6/51G875dkQt+c+qk/ah/ZKOIENRR9hJOXoI3DUAV3Vm86vAk NrHpUym0N5SMNz2MBWNPv3iPZ8IJkUwPIGABQSI9aq98UkcodKWZRfQpDEVHzePxUMqo 48aQ3OpDfgsS9XBSoRK8eh93e+9VRwnRnnsPpZ+2EFCi8v/W4pR0hCx45nOB1Qg00Dq4 hb2w== X-Forwarded-Encrypted: i=1; AJvYcCU/eOXRwIdS3Kbz48JgmGaMoBZCy+UPvOe6u/Zs/688DWnp0dxZUt/F1IZ1OOqC6PnVXgn4cmxpWJv6wL/NNj+ayCY= X-Gm-Message-State: AOJu0Yy9uHiWOSCdYNgMg/s1fiYCXNFJRs4sKxiZvsCuzQXoV+LM0TwD 7uxQsG5J+SiA0gEReyFKRVfm0l0M/itR8VGG5ig78BrVSE8vZwFtuFOatvF79ctE7LBUsSQBv6u 4ajg5fIJzWSg26X2n3vfKrfTSxlM= X-Google-Smtp-Source: AGHT+IEctXxI4GtfsbdvgU18kpz1czwdQNe5VudddT1bGZDUNSoKC4wJNkZPBYg4PSmVrTNyE8PVkahfhKPmWMOZJU4= X-Received: by 2002:a17:90a:f682:b0:2c3:2557:3de8 with SMTP id 98e67ed59e1d1-2d1c34585d8mr3673610a91.33.1723152253191; Thu, 08 Aug 2024 14:24:13 -0700 (PDT) MIME-Version: 1.0 References: <20240807234029.456316-1-andrii@kernel.org> <20240807234029.456316-7-andrii@kernel.org> In-Reply-To: From: Andrii Nakryiko Date: Thu, 8 Aug 2024 14:23:59 -0700 Message-ID: Subject: Re: [PATCH v4 bpf-next 06/10] lib/buildid: implement sleepable build_id_parse() API To: Jann Horn Cc: Shakeel Butt , Andrii Nakryiko , bpf@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, adobriyan@gmail.com, hannes@cmpxchg.org, ak@linux.intel.com, osandov@osandov.com, song@kernel.org, linux-fsdevel@vger.kernel.org, willy@infradead.org, Omar Sandoval Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Rspamd-Server: rspam03 X-Rspam-User: X-Rspamd-Queue-Id: A6F0D1C0019 X-Stat-Signature: 5ujb8m7bryrssim5iqzajm5is8878dax X-HE-Tag: 1723152254-725288 X-HE-Meta: U2FsdGVkX1/eh+Ieo8oOniZbcusFZ/3z299cNlQle8/vMLPbgCDGXXcZt+N2togI3GTMVpvohLyBIeXqB5oiMWRyv2K/dHDgAcPGDSdKIAUaHAPod8hVkqaXSdvCwibv/nTNhCh3Ty6CnQhO50oizD+smSd6xSAmYqWJLwcLKYgGAGjxlVV8VjuFN0CQEz+AhqpAIh5GHFhi7Wix8f3VUOHxx4cC2XNm2njfSgK04s0mexfiPsaRF9aQBQYTNclj8Nw+wkuoype+0url/0Qdbwo/aCJFdx8egr7HoFS8cZKkUrAYNhqnG/IdKOgWi59Zr2MLln54sJ8guSl6T8stBBH2zfzy09YZXhtM89/zVdB92FHIW30/pImEEr3bcz8qMGvfw+2OlEUjToagocVINp79Z+vNQqYpw70LS1mAa1qVAn7w+pNTRJONGxgDgdwmQPsD+BArEcbqD78p+EzZtNouv21gHibSsE4c0+BUS6+XOHz1otm+B7NjPiZuGLlBfsj6QhroqIGnj8lWlcM9txCLBgIHssuQBrbHHfALu58omUBFV7e3XJW6c+EPYHj6lVy8kqGlLO4w3dloF3r7KnnzjL7b0PG+ewmG0UK7eH0CGmjZ3PGiGYRUOfXtBSrnVvvZ11p6yhv6hgPUlAbyBrRGk24Vxr8gnhMCH/pfREoTMmfqloBbEi/N77615vaR96009lEpWHfCfba1DN8tx+ycPweVgRsPjpOTiE+9/jC9LM8QiE9vb1BVAFz34k+QhnaKHnaIU2LVi0wAvHWYtly08l9b1oJMOnXfHe00elhNfoKc4f3bkwhvbey3AkDoIlgKENz0MljQCKr35P48OvMMHZiaXOUzZ7EP4Kh6MDtlZdLVt1R1QBvEDBBFAIJA+LE6zh+Kj8ZyCbhu/itKj06wu6yBcMPMd1mbys0OWRq+4HyQRnNUKcF9kSOf92ZC7UyF9tJrL5PR/NZtLis BVh/YFOB 2qF8RO/ddHiYJNIR7X69HqBE9XFkwjr2LBwBKXb55qi215UYlu4ZctaMi4a8UbalLHRTfXxxVwB0vznLMOIvk0R6vwp57QjKZJZ1bZAthZTZ+rDKovOahtdoXt5FDMBb8hWbtMLVxWjhXwquFsJ/1HVk2Ijsn+TEsycqC3K9fGyDN/kU+YMsScZ6oBtzWlXn/41Dc4jBlMxwj+OZc1xU3RwEFCppDnoDREauklCcuAZjA1r+ouf4B37sfAVsaYlw/m/9EdeqXN1p7BDpOoyPRVdYlVxkHUQ0QD02XfFyjitzp8jKFg73JtuRQATdg8M+zKJvkiWkywBaITNvOMhKz3NPlnD07TKL9aowN/5k36tl7yW/P7U7wD3Z/h2eFryC5iJgQsTi5rttBAM/yiKIBC2F/SWO0sFGoyL/5WC1D+eoz2BSdhcbPxCpzB2G7wgsY7qz6pZBBCrbdD9RF/1yN7doetIBazxgKLxQS9QWq7Prc3m9JjoIdQon16KGuXD2UgNQ4dRo1riaFyJI7YyJfWMTOsNhjR6lEBI0uStjayKfSKkB6g1La3fIs5JBWbi+7mRXCNd/D17zAbnya67i1uXlGh8GGNyJM4wMN 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 Thu, Aug 8, 2024 at 1:58=E2=80=AFPM Jann Horn wrote: > > On Thu, Aug 8, 2024 at 10:16=E2=80=AFPM Andrii Nakryiko > wrote: > > On Thu, Aug 8, 2024 at 11:40=E2=80=AFAM Shakeel Butt wrote: > > > > > > On Wed, Aug 07, 2024 at 04:40:25PM GMT, Andrii Nakryiko wrote: > > > > Extend freader with a flag specifying whether it's OK to cause page > > > > fault to fetch file data that is not already physically present in > > > > memory. With this, it's now easy to wait for data if the caller is > > > > running in sleepable (faultable) context. > > > > > > > > We utilize read_cache_folio() to bring the desired folio into page > > > > cache, after which the rest of the logic works just the same at fol= io level. > > > > > > > > Suggested-by: Omar Sandoval > > > > Cc: Shakeel Butt > > > > Cc: Johannes Weiner > > > > Signed-off-by: Andrii Nakryiko > > > > --- > > > > lib/buildid.c | 44 ++++++++++++++++++++++++++++---------------- > > > > 1 file changed, 28 insertions(+), 16 deletions(-) > > > > > > > > diff --git a/lib/buildid.c b/lib/buildid.c > > > > index 5e6f842f56f0..e1c01b23efd8 100644 > > > > --- a/lib/buildid.c > > > > +++ b/lib/buildid.c > > > > @@ -20,6 +20,7 @@ struct freader { > > > > struct folio *folio; > > > > void *addr; > > > > loff_t folio_off; > > > > + bool may_fault; > > > > }; > > > > struct { > > > > const char *data; > > > > @@ -29,12 +30,13 @@ struct freader { > > > > }; > > > > > > > > static void freader_init_from_file(struct freader *r, void *buf, u= 32 buf_sz, > > > > - struct address_space *mapping) > > > > + struct address_space *mapping, boo= l may_fault) > > > > { > > > > memset(r, 0, sizeof(*r)); > > > > r->buf =3D buf; > > > > r->buf_sz =3D buf_sz; > > > > r->mapping =3D mapping; > > > > + r->may_fault =3D may_fault; > > > > } > > > > > > > > static void freader_init_from_mem(struct freader *r, const char *d= ata, u64 data_sz) > > > > @@ -63,6 +65,11 @@ static int freader_get_folio(struct freader *r, = loff_t file_off) > > > > freader_put_folio(r); > > > > > > > > r->folio =3D filemap_get_folio(r->mapping, file_off >> PAGE_S= HIFT); > > > > + > > > > + /* if sleeping is allowed, wait for the page, if necessary */ > > > > + if (r->may_fault && (IS_ERR(r->folio) || !folio_test_uptodate= (r->folio))) > > > > + r->folio =3D read_cache_folio(r->mapping, file_off >>= PAGE_SHIFT, NULL, NULL); > > > > > > Willy's network fs comment is bugging me. If we pass NULL for filler, > > > the kernel will going to use fs's read_folio() callback. I have check= ed > > > read_folio() for fuse and nfs and it seems like for at least these tw= o > > > filesystems the callback is accessing file->private_data. So, if the = elf > > > file is on these filesystems, we might see null accesses. > > > > > > > Isn't that just a huge problem with the read_cache_folio() interface > > then? That file is optional, in general, but for some specific FS > > types it's not. How generic code is supposed to know this? > > I think you have to think about it the other way around. The file is Fair enough: > @file: Passed to filler function, may be NULL if not required. But then you look at mapping_read_folio_gfp() which *always* unconditionally passes NULL for filler and file, and that makes you think that file is some special *extra* parameter. But regardless, as you pointed out, I won't have to take extra ref, so my concerns about performance are wrong. I'll pass the file. > required, unless you know the filler function that will be used > doesn't use the file. Which you don't know when you're coming from > generic code, so generic code has to pass in a file. > > As far as I can tell, most of the callers of read_cache_folio() (via > read_mapping_folio()) are inside filesystem implementations, not > generic code, so they know what the filler function will do. You're > generic code, so I think you have to pass in a file. > Yep, I guess this is a bit of trailblazing use case. I was confused by some other helpers passing NULL for file unconditionally, which made me think that NULL is a supported default use case. Clearly I was wrong. > > Or maybe it's a bug with the nfs_read_folio() and fuse_read_folio() > > implementation that they can't handle NULL file argument? > > netfs_read_folio(), for example, seems to be working with file =3D=3D N= ULL > > just fine. > > > > Matthew, can you please advise what's the right approach here? I can, > > of course, always get file refcount, but most of the time it will be > > just an unnecessary overhead, so ideally I'd like to avoid that. But > > if I have to check each read_folio callback implementation to know > > whether it's required or not, then that's not great... > > Why would you need to increment the file refcount? As far as I can > tell, all your accesses to the file would happen under > __build_id_parse(), which is borrowing the refcounted reference from > vma->vm_file; the file can't go away as long as your caller is holding > the mmap lock. Yep, agreed.