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 X-Spam-Level: X-Spam-Status: No, score=-17.2 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 95C28C433FE for ; Tue, 14 Sep 2021 16:45:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 20F3B610A6 for ; Tue, 14 Sep 2021 16:45:10 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 20F3B610A6 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id A878D6B0071; Tue, 14 Sep 2021 12:45:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id A3708900002; Tue, 14 Sep 2021 12:45:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8D8216B0073; Tue, 14 Sep 2021 12:45:09 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0067.hostedemail.com [216.40.44.67]) by kanga.kvack.org (Postfix) with ESMTP id 77AD76B0071 for ; Tue, 14 Sep 2021 12:45:09 -0400 (EDT) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 23A2F181E9979 for ; Tue, 14 Sep 2021 16:45:09 +0000 (UTC) X-FDA: 78586753938.24.92D2FFB Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf10.hostedemail.com (Postfix) with ESMTP id B8801600198B for ; Tue, 14 Sep 2021 16:45:08 +0000 (UTC) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 76C6520141; Tue, 14 Sep 2021 16:45:07 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1631637907; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=ozFdqBIwmwyJglrohMvn3NkJIfp+ikVZXcbLX7txgcw=; b=c+tWXz3iFiMt2fBT2y8zj5/DP3kTS+clAtQNQjW/Q0M6SxwQAy6krw8G8j0cJ8C2E/b09i awJaPyl97itLD9c6bLIbr1CN+kU50My+7nVpVgBukC5bx/DrxIAwFzjQQkm1Bg2V8RK7QV BDTYLlCiPqBuvc5JJPd+Ai4Uy25bW3s= Received: from suse.com (unknown [10.163.32.246]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id B15CCA3B94; Tue, 14 Sep 2021 16:45:06 +0000 (UTC) Date: Tue, 14 Sep 2021 17:45:04 +0100 From: Mel Gorman To: NeilBrown Cc: Dave Chinner , Andrew Morton , Theodore Ts'o , Andreas Dilger , "Darrick J. Wong" , Matthew Wilcox , linux-xfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-nfs@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 6/6] XFS: remove congestion_wait() loop from xfs_buf_alloc_pages() Message-ID: <20210914164504.GS3828@suse.com> References: <163157808321.13293.486682642188075090.stgit@noble.brown> <163157838440.13293.12568710689057349786.stgit@noble.brown> <20210914020837.GH2361455@dread.disaster.area> <163158695921.3992.9776900395549582360@noble.neil.brown.name> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <163158695921.3992.9776900395549582360@noble.neil.brown.name> User-Agent: Mutt/1.10.1 (2018-07-13) X-Stat-Signature: bmdqkidyqmzt668e7bwnnbkafzdjeud1 Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=c+tWXz3i; dmarc=pass (policy=quarantine) header.from=suse.com; spf=pass (imf10.hostedemail.com: domain of mgorman@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=mgorman@suse.com X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: B8801600198B X-HE-Tag: 1631637908-840426 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 Tue, Sep 14, 2021 at 12:35:59PM +1000, NeilBrown wrote: > On Tue, 14 Sep 2021, Dave Chinner wrote: > > On Tue, Sep 14, 2021 at 10:13:04AM +1000, NeilBrown wrote: > > > Documentation commment in gfp.h discourages indefinite retry loops on > > > ENOMEM and says of __GFP_NOFAIL that it > > > > > > is definitely preferable to use the flag rather than opencode > > > endless loop around allocator. > > > > > > congestion_wait() is indistinguishable from > > > schedule_timeout_uninterruptible() in practice and it is not a good way > > > to wait for memory to become available. > > > > > > So instead of waiting, allocate a single page using __GFP_NOFAIL, then > > > loop around and try to get any more pages that might be needed with a > > > bulk allocation. This single-page allocation will wait in the most > > > appropriate way. > > > > > > Signed-off-by: NeilBrown > > > --- > > > fs/xfs/xfs_buf.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > > > index 5fa6cd947dd4..1ae3768f6504 100644 > > > --- a/fs/xfs/xfs_buf.c > > > +++ b/fs/xfs/xfs_buf.c > > > @@ -372,8 +372,8 @@ xfs_buf_alloc_pages( > > > > > > /* > > > * Bulk filling of pages can take multiple calls. Not filling the entire > > > - * array is not an allocation failure, so don't back off if we get at > > > - * least one extra page. > > > + * array is not an allocation failure, so don't fail or fall back on > > > + * __GFP_NOFAIL if we get at least one extra page. > > > */ > > > for (;;) { > > > long last = filled; > > > @@ -394,7 +394,7 @@ xfs_buf_alloc_pages( > > > } > > > > > > XFS_STATS_INC(bp->b_mount, xb_page_retries); > > > - congestion_wait(BLK_RW_ASYNC, HZ / 50); > > > + bp->b_pages[filled++] = alloc_page(gfp_mask | __GFP_NOFAIL); > > > > This smells wrong - the whole point of using the bulk page allocator > > in this loop is to avoid the costly individual calls to > > alloc_page(). > > > > What we are implementing here fail-fast semantics for readahead and > > fail-never for everything else. If the bulk allocator fails to get > > a page from the fast path free lists, it already falls back to > > __alloc_pages(gfp, 0, ...) to allocate a single page. So AFAICT > > there's no need to add another call to alloc_page() because we can > > just do this instead: > > > > if (flags & XBF_READ_AHEAD) > > gfp_mask |= __GFP_NORETRY; > > else > > - gfp_mask |= GFP_NOFS; > > + gfp_mask |= GFP_NOFS | __GFP_NOFAIL; > > > > Which should make the __alloc_pages() call in > > alloc_pages_bulk_array() do a __GFP_NOFAIL allocation and hence > > provide the necessary never-fail guarantee that is needed here. > > That is a nice simplification. > Mel Gorman told me > https://lore.kernel.org/linux-nfs/20210907153116.GJ3828@suse.com/ > that alloc_pages_bulk ignores GFP_NOFAIL. I added that to the > documentation comment in an earlier patch. > > I had a look at the code and cannot see how it would fail to allocate at > least one page. Maybe Mel can help.... > If there are already at least one page an the array and the first attempt at bulk allocation fails, it'll simply return. It's an odd corner case that may never apply but it's possible. That said, I'm of the opinion that __GFP_NOFAIL should not be expanded and instead congestion_wait should be deleted and replaced with something triggered by reclaim making progress. -- Mel Gorman SUSE Labs