From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f45.google.com (mail-pj1-f45.google.com [209.85.216.45]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6F9581953A1 for ; Mon, 17 Feb 2025 21:31:59 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.216.45 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1739827921; cv=none; b=jlByP+BkhLp1RQxBoVS7zW6OYkjEDiM2LGc3lTRs2DnUeQ7MQZHkOZhZFXqNs5NEgExGAxDB7mWlvIvUpKov8YKAeSnxmOlVnmC3CR73eKnZ1N5Pf7fSdi12u80MCWMk+Xbjk6Yjg2ME01xNIQpYlaAxd8TClLEYlEkMUA1U38U= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1739827921; c=relaxed/simple; bh=5Z3w6D6rLKLuQ6zEjNL6ikv+CipM0JHOZMCbXFfqNBQ=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=Mb9oc1IPBd9r+JJNtZGiP6Z59YB5TQyTENWAmThpsWZf5hfqWPaABebihCYswu9fh9u0fzJ5oXEN7aBt+MGXpo/nr07X/gKPG2WPP3uvilteTo98CuTAQSy+KxAT7HVO3dRf8jJEJmHC/vtXd7DxYcIpYgbyJxTRv7415Sj03i0= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com; spf=pass smtp.mailfrom=fromorbit.com; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b=iSnbtykA; arc=none smtp.client-ip=209.85.216.45 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fromorbit.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.b="iSnbtykA" Received: by mail-pj1-f45.google.com with SMTP id 98e67ed59e1d1-2fa48404207so9604167a91.1 for ; Mon, 17 Feb 2025 13:31:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1739827919; x=1740432719; darn=vger.kernel.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=iSnbtykAPgeoaamX/D5Xv39Q6ZYYIHEVE4zgMl66cwZvyQ48oaYzC/IgKLS86w/HD5 CKBcfACcqxE79g5417jqRYfPLcOSB6GqY0F0IGgHXLJSrAsw+pHW+/BrFDI6sARK9/gu Ykml65dgc1/utgt1Fmn5RhHyRVJuUcEvZU3xJchUB3WXLYnbSu+ReTOwGxknLjjSZHeX 98GJwsv/gTKG2lH8c9qcf91n5QTxQQ0DqQEQLYYDOqXZmgxu6lKi/Skz8q+fHbi/IGsU A5VQDVfWr3DZcIo3115AKKu7y5dD6S6IG87n4slcw011eIp41wnUCyXKCC04ss9B4jwe KEEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739827919; x=1740432719; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=YEBOO/LGQnTpdy4p0lCLK8gcvdKerdUVKJ5DJ9hFtzPn04xryLuhg3YnvEkCHkPXak TZv/gkgZL81808vehOEATvPRqGFGrq6WtoPJwxMqoQ5gsGQBb//9CmkPThu3IBIivRPl vvQteV/N6xUiCUAYBz/HqiQcrftjOPz9hpR0+sDSzWjrc5AnKrBiXPasgOQcCNcLfK+z XLnPOwkAFRAV4aKfILU0qw+5KjoQlX49oKsWD9lDT4Kvp3gIrv4yhkIpgt7wAglHMxbd fwWmIZTeLkKHslS6F710N5mB6gPJMYFC5deDSjsQ3zF/f+kGLRnpyDjwMiBOyvCioNjo bVlQ== X-Forwarded-Encrypted: i=1; AJvYcCVi6yPePB2oO0B+X282TB+j0+WW5+WpMnjyV9zNLI9YXapJ0XjRGfHbbNSOK7m20Okc5eI=@vger.kernel.org X-Gm-Message-State: AOJu0YznJwB+gNjW+y8uz5k2PsDMJ/YSpGWoHnp0nofzy60vrsngxUfk HPTGpmlJ0SIU4KNk5Ren0if6Cq6+ke69xvpx2Kw3o1ct8hInBVQtUt3QvvBuT44= X-Gm-Gg: ASbGncvlyTvGJncom4/uEBQdt7z7/WEi8mt0f/TabQubhNcc74YeIUwvXIBbTIcBJYw 0RK081VG1ck1AFVuo8rjrb3P5ovkQsODz6uork9sU+2QR7YAh3RObtQjBhZQCq61UI0QXpy5bHR CrRdv2iaODyqleNE09dKuATNqcnjU0wi4JoXlq3HlNmJhmLF/+QtF9cGpFIqeGWSd9ZUYZ+4bs0 0kTuV0xfecUCcX5g0A3T9MRqJSgJ75fI9YnpwyTMDEhzxMjFv+tvBcLRTjcUHjEAYHCwOoUkEQb 4bbxy2xXf8SLeu93LdpYqDM00faief3k6AMuLnZa4yluQfA8cGwATe7V1l2k+x6+D4c= X-Google-Smtp-Source: AGHT+IGWbBJAtL2UtKSWCS6Jd9Ns/0Md8Y1s79IB++xb28sgasLU0+0/qaH46F3zNCw25wym7QqlJQ== X-Received: by 2002:a17:90b:3b4b:b0:2ee:fdf3:390d with SMTP id 98e67ed59e1d1-2fc4115087fmr15333822a91.31.1739827918683; Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dread.disaster.area (pa49-186-89-135.pa.vic.optusnet.com.au. [49.186.89.135]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-220d53491d4sm76091525ad.4.2025.02.17.13.31.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dave by dread.disaster.area with local (Exim 4.98) (envelope-from ) id 1tk8iZ-00000002XxN-2C1Y; Tue, 18 Feb 2025 08:31:55 +1100 Date: Tue, 18 Feb 2025 08:31:55 +1100 From: Dave Chinner To: Yunsheng Lin Cc: Yishai Hadas , Jason Gunthorpe , Shameer Kolothum , Kevin Tian , Alex Williamson , Chris Mason , Josef Bacik , David Sterba , Gao Xiang , Chao Yu , Yue Hu , Jeffle Xu , Sandeep Dhavale , Carlos Maiolino , "Darrick J. Wong" , Andrew Morton , Jesper Dangaard Brouer , Ilias Apalodimas , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni , Simon Horman , Trond Myklebust , Anna Schumaker , Chuck Lever , Jeff Layton , Neil Brown , Olga Kornievskaia , Dai Ngo , Tom Talpey , Luiz Capitulino , Mel Gorman , kvm@vger.kernel.org, virtualization@lists.linux.dev, linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org, netdev@vger.kernel.org, linux-nfs@vger.kernel.org Subject: Re: [RFC] mm: alloc_pages_bulk: remove assumption of populating only NULL elements Message-ID: References: <20250217123127.3674033-1-linyunsheng@huawei.com> Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250217123127.3674033-1-linyunsheng@huawei.com> On Mon, Feb 17, 2025 at 08:31:23PM +0800, Yunsheng Lin wrote: > As mentioned in [1], it seems odd to check NULL elements in > the middle of page bulk allocating, and it seems caller can > do a better job of bulk allocating pages into a whole array > sequentially without checking NULL elements first before > doing the page bulk allocation. .... IMO, the new API is a poor one, and you've demonstrated it clearly in this patch. ..... > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 15bb790359f8..9e1ce0ab9c35 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -377,16 +377,17 @@ xfs_buf_alloc_pages( > * least one extra page. > */ > for (;;) { > - long last = filled; > + long alloc; > > - filled = alloc_pages_bulk(gfp_mask, bp->b_page_count, > - bp->b_pages); > + alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - refill, > + bp->b_pages + refill); > + refill += alloc; > if (filled == bp->b_page_count) { > XFS_STATS_INC(bp->b_mount, xb_page_found); > break; > } > > - if (filled != last) > + if (alloc) > continue; You didn't even compile this code - refill is not defined anywhere. Even if it did complile, you clearly didn't test it. The logic is broken (what updates filled?) and will result in the first allocation attempt succeeding and then falling into an endless retry loop. i.e. you stepped on the API landmine of your own creation where it is impossible to tell the difference between alloc_pages_bulk() returning "memory allocation failed, you need to retry" and it returning "array is full, nothing more to allocate". Both these cases now return 0. The existing code returns nr_populated in both cases, so it doesn't matter why alloc_pages_bulk() returns with nr_populated != full, it is very clear that we still need to allocate more memory to fill it. The whole point of the existing API is to prevent callers from making stupid, hard to spot logic mistakes like this. Forcing callers to track both empty slots and how full the array is itself, whilst also constraining where in the array empty slots can occur greatly reduces both the safety and functionality that alloc_pages_bulk() provides. Anyone that has code that wants to steal a random page from the array and then refill it now has a heap more complex code to add to their allocator wrapper. IOWs, you just demonstrated why the existing API is more desirable than a highly constrained, slightly faster API that requires callers to get every detail right. i.e. it's hard to get it wrong with the existing API, yet it's so easy to make mistakes with the proposed API that the patch proposing the change has serious bugs in it. -Dave. -- Dave Chinner david@fromorbit.com 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 lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id EAA22C021A9 for ; Mon, 17 Feb 2025 21:32:06 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lists.ozlabs.org; s=201707; t=1739827924; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; h=Date:To:Subject:References:In-Reply-To:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=ZbIJ3v2G+sN7hLfEoOUak0YvF3o+01hqBcyrQGOJD41Jt+mvgT/vu1oXuUYvPopXs NZlelGQQ6Wh9BlC2vGldIEAC1/AAsDWEw/budiMpUCRcNsiN1ghOm7+ix/vEynJnsu qLeV8PXqzFBX8SARuq5ioVTGldMElZJPNRCtrcVG/sm72ObsKXgUrsfqOUWWX64+Hy qXm79rEzoCoQ0KAUeevzUrGV4jq3AyO0CX+kwEQpC/wglCWYVITZI2F1TZF5WS4Yas zbsV0YaeBLWzd2eNmIjwDALjhb/CoBM8EeSMaXjvZVOnlzNnKWI0VXmHisM1L3ssvi T0EJdjC6G1fcQ== Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4YxbTJ5132z3bn8 for ; Tue, 18 Feb 2025 08:32:04 +1100 (AEDT) Authentication-Results: lists.ozlabs.org; arc=none smtp.remote-ip="2607:f8b0:4864:20::102b" ARC-Seal: i=1; a=rsa-sha256; d=lists.ozlabs.org; s=201707; t=1739827922; cv=none; b=EvNWQYUMElCPULBg/enTZzBkJ5fTAfERoTcLazkk+Ay790Gz5Ff8+wRspSAyAjmrhLvJo4ONJudGMOg5zVGJJ/XA36XV8MgOttIsIWyKzjW5el7JVz9xyqcgyH9jSdTjGU3dpS6ZCVczygdJ/uEkfs/y4ca0Okpna3HX/b0QeirV30T/pUil7JCKZTnwrkL8Ds+Eep4MWkI/Xh/GXtfsHioke/ELxDZyIBzjQDj5rPnR7hb/eJ/shaIY/rqzRSeRv0/2nijghzNZQvKkksLTM9DrETM34+v2HhbV0aLGwQDcFarc/rj2/AYEbMJ6Aa/pel2QEjZJhyEamondvZlgyA== ARC-Message-Signature: i=1; a=rsa-sha256; d=lists.ozlabs.org; s=201707; t=1739827922; c=relaxed/relaxed; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PrNwNiTGFT2PIU0u3O1LUOZD5cbqfBCExWzjvza4aP5kwKOXUoFuhY8ZZ/8rXLz+R+ApKwTQcnwQfii2U3zav4TV6iAIfla8Z/bKxaRojEMajODxZNaFBBlp5UCmCSLE0c9KI1skA8P1pMZ1yy+aF2DzOzS/3SYG/kKB1+t/9xEx5jmg5XXOhtpS+h6ftHmbj+FKBzTpng4jyzIO10S9RX2/ALnHGMRScufv0wl1M+Sgi7eoo0rUnt+AQdEfORrvqdFMP9G+kpvNYt6SpzfEASGYhrLZx84IrVEOirJBnW4Wg4RVlBx0peT03JsU32tSi6Ekag4J2gi5iSlaHwJ4HQ== ARC-Authentication-Results: i=1; lists.ozlabs.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com; dkim=pass (2048-bit key; unprotected) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=mt+sn0iN; dkim-atps=neutral; spf=pass (client-ip=2607:f8b0:4864:20::102b; helo=mail-pj1-x102b.google.com; envelope-from=david@fromorbit.com; receiver=lists.ozlabs.org) smtp.mailfrom=fromorbit.com Authentication-Results: lists.ozlabs.org; dmarc=pass (p=quarantine dis=none) header.from=fromorbit.com Authentication-Results: lists.ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=fromorbit-com.20230601.gappssmtp.com header.i=@fromorbit-com.20230601.gappssmtp.com header.a=rsa-sha256 header.s=20230601 header.b=mt+sn0iN; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=fromorbit.com (client-ip=2607:f8b0:4864:20::102b; helo=mail-pj1-x102b.google.com; envelope-from=david@fromorbit.com; receiver=lists.ozlabs.org) Received: from mail-pj1-x102b.google.com (mail-pj1-x102b.google.com [IPv6:2607:f8b0:4864:20::102b]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4YxbTF4gtNz2yDp for ; Tue, 18 Feb 2025 08:32:00 +1100 (AEDT) Received: by mail-pj1-x102b.google.com with SMTP id 98e67ed59e1d1-2fc3fa00323so5063136a91.3 for ; Mon, 17 Feb 2025 13:32:00 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=fromorbit-com.20230601.gappssmtp.com; s=20230601; t=1739827919; x=1740432719; darn=lists.ozlabs.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=mt+sn0iNHcwWYdtYctn1Ww1Be4DZDJUuYTGesoKNP6XGukyht++i54ujrbe6p4jcGH ujIEXdJ5rOWAX3J0s4rPeQQ2HW9PwvT1SwX0ArzyKkMPmBgR2ID/Hk+U8nDvwxZlZEC9 a0s60YJ6fZVd2DL7ZXOuyblfGcZcojKUm5vy7v2O7O6Kw9tcxcdVtii8Zg7YSw84vbJU WbGZSbuL5+wOg13T2bEBpjpv6H5Y0xmw6GEU7Ljlt2GAFd55lA/Jp2C7yiqNAf6itMsG aokEhnOC1YKVfJerikcYdtut0c3FG5EmhKZM5H9HhHzDJXl2N1+fahv5K97vBrvPbee0 Xu7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1739827919; x=1740432719; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=QhTfy1/ybvgYfqGEX8d/5gFaFMopC4YNbC7hGVBL/4c=; b=U35Zb8vycaKdOWZ6ObTD/lns+mrwf0cT5kBolOu1Ip0EeO0z3aofr408KYv/yTI6fa 2E4tsSSW9wZt/XnMFuW9w6Y9cCREcIH5NL+Nek5XfYkdGdgyoNT0HwPknwl5JpPNDuJe Os279Gtogk/CKr4hPBWBVrrDK2U+YWYjtxXgHEBIivZQWm5xiuwbXfKVuKVCJlQb2xNB xwQ+Q4//2+qQywdZBgjo/bjuCPdysSBeo/vIAj13TF8tPooXZfBgTDtaVnQAqTkXihMT u2dbS9NXa6pruHJ58dCaHDqGii3zMkOMK79DjoVrFGRJHWSWwkVyZel7Dzgy/4Jve6YS M7LA== X-Forwarded-Encrypted: i=1; AJvYcCW/IgYk62o40r1rNpQeBU6ougEQ85mEgIss1xD1hQnvcf3TmIME8Fiditcq1tsFcPrtz6zXbze414e1cQ==@lists.ozlabs.org X-Gm-Message-State: AOJu0YydXaC/OyOeAfQHtP1+Nu53JicKfsYaJipvWsV5CR3bZ9bHoW6P ntTE4GHj6PGeyKR/kJBDOJWcQt3vyVm5FHfb7tJLPgfw8xjPQDz5UFU5KbfDUfg= X-Gm-Gg: ASbGncsDS0j9akgfc5nlhjBAkkaz7GgtlhxS4IGBRs8KgH3EuXJsj+hSnGpUQ8FHTxp +J3PVRi3xUrrBaOYbh6nsZWh6ZP3/HSHUSHbzOFdG6liYnkArmSH3aFygNUhwl2zbJrK6EEDmwL TXhHotM/Kff49biDm6J6jpaQnXmdzBkRDKeOHMZrWdYca0H1895Zv+fTpvfvZVdZkK+fIwCMdX6 Kr9cOMfU2pP45uXLpW65SIL2tcv+qwryEn5aU6Dxq/4jtRDBo/Wxh0XWvIgxzh/66R6dO7agcpy 9aFpOCGtmcuZq9t3jhXFayrvQOgHNXQ1w9kpiI8n9ZBkbdvjC3qP1a62lh4qNJeH4RU= X-Google-Smtp-Source: AGHT+IGWbBJAtL2UtKSWCS6Jd9Ns/0Md8Y1s79IB++xb28sgasLU0+0/qaH46F3zNCw25wym7QqlJQ== X-Received: by 2002:a17:90b:3b4b:b0:2ee:fdf3:390d with SMTP id 98e67ed59e1d1-2fc4115087fmr15333822a91.31.1739827918683; Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dread.disaster.area (pa49-186-89-135.pa.vic.optusnet.com.au. [49.186.89.135]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-220d53491d4sm76091525ad.4.2025.02.17.13.31.58 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 17 Feb 2025 13:31:58 -0800 (PST) Received: from dave by dread.disaster.area with local (Exim 4.98) (envelope-from ) id 1tk8iZ-00000002XxN-2C1Y; Tue, 18 Feb 2025 08:31:55 +1100 Date: Tue, 18 Feb 2025 08:31:55 +1100 To: Yunsheng Lin Subject: Re: [RFC] mm: alloc_pages_bulk: remove assumption of populating only NULL elements Message-ID: References: <20250217123127.3674033-1-linyunsheng@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250217123127.3674033-1-linyunsheng@huawei.com> X-BeenThere: linux-erofs@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development of Linux EROFS file system List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Dave Chinner via Linux-erofs Reply-To: Dave Chinner Cc: kvm@vger.kernel.org, Neil Brown , "Darrick J. Wong" , Carlos Maiolino , Chris Mason , Eric Dumazet , Anna Schumaker , Dai Ngo , Jason Gunthorpe , Jakub Kicinski , Paolo Abeni , Kevin Tian , Olga Kornievskaia , Jesper Dangaard Brouer , Josef Bacik , virtualization@lists.linux.dev, Tom Talpey , Alex Williamson , David Sterba , linux-nfs@vger.kernel.org, Yishai Hadas , linux-mm@kvack.org, linux-erofs@lists.ozlabs.org, Ilias Apalodimas , Jeff Layton , linux-kernel@vger.kernel.org, Shameer Kolothum , linux-xfs@vger.kernel.org, Chuck Lever , linux-btrfs@vger.kernel.org, Simon Horman , netdev@vger.kernel.org, Andrew Morton , Mel Gorman , "David S. Miller" , Trond Myklebust , Luiz Capitulino Errors-To: linux-erofs-bounces+linux-erofs=archiver.kernel.org@lists.ozlabs.org Sender: "Linux-erofs" On Mon, Feb 17, 2025 at 08:31:23PM +0800, Yunsheng Lin wrote: > As mentioned in [1], it seems odd to check NULL elements in > the middle of page bulk allocating, and it seems caller can > do a better job of bulk allocating pages into a whole array > sequentially without checking NULL elements first before > doing the page bulk allocation. .... IMO, the new API is a poor one, and you've demonstrated it clearly in this patch. ..... > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 15bb790359f8..9e1ce0ab9c35 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -377,16 +377,17 @@ xfs_buf_alloc_pages( > * least one extra page. > */ > for (;;) { > - long last = filled; > + long alloc; > > - filled = alloc_pages_bulk(gfp_mask, bp->b_page_count, > - bp->b_pages); > + alloc = alloc_pages_bulk(gfp_mask, bp->b_page_count - refill, > + bp->b_pages + refill); > + refill += alloc; > if (filled == bp->b_page_count) { > XFS_STATS_INC(bp->b_mount, xb_page_found); > break; > } > > - if (filled != last) > + if (alloc) > continue; You didn't even compile this code - refill is not defined anywhere. Even if it did complile, you clearly didn't test it. The logic is broken (what updates filled?) and will result in the first allocation attempt succeeding and then falling into an endless retry loop. i.e. you stepped on the API landmine of your own creation where it is impossible to tell the difference between alloc_pages_bulk() returning "memory allocation failed, you need to retry" and it returning "array is full, nothing more to allocate". Both these cases now return 0. The existing code returns nr_populated in both cases, so it doesn't matter why alloc_pages_bulk() returns with nr_populated != full, it is very clear that we still need to allocate more memory to fill it. The whole point of the existing API is to prevent callers from making stupid, hard to spot logic mistakes like this. Forcing callers to track both empty slots and how full the array is itself, whilst also constraining where in the array empty slots can occur greatly reduces both the safety and functionality that alloc_pages_bulk() provides. Anyone that has code that wants to steal a random page from the array and then refill it now has a heap more complex code to add to their allocator wrapper. IOWs, you just demonstrated why the existing API is more desirable than a highly constrained, slightly faster API that requires callers to get every detail right. i.e. it's hard to get it wrong with the existing API, yet it's so easy to make mistakes with the proposed API that the patch proposing the change has serious bugs in it. -Dave. -- Dave Chinner david@fromorbit.com