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 40F1AC46CD2 for ; Tue, 30 Jan 2024 14:22:12 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B4B756B0078; Tue, 30 Jan 2024 09:22:11 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id AFC266B007B; Tue, 30 Jan 2024 09:22:11 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9EA616B0080; Tue, 30 Jan 2024 09:22:11 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 90A306B0078 for ; Tue, 30 Jan 2024 09:22:11 -0500 (EST) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 3A27E1A069E for ; Tue, 30 Jan 2024 14:22:11 +0000 (UTC) X-FDA: 81736192062.04.0B03911 Received: from verein.lst.de (verein.lst.de [213.95.11.211]) by imf13.hostedemail.com (Postfix) with ESMTP id 57E232001E for ; Tue, 30 Jan 2024 14:22:09 +0000 (UTC) Authentication-Results: imf13.hostedemail.com; dkim=none; spf=pass (imf13.hostedemail.com: domain of hch@lst.de designates 213.95.11.211 as permitted sender) smtp.mailfrom=hch@lst.de; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1706624529; 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; bh=kNqxEzRBCqwlu7J4B5D52+46vB6Y1AlhY8FzvdMptrI=; b=HGGzKsC28zuQ7uJS8WnsQ2pyIeyw0DJ5IAeOo20FIW9AIHMOnRZrrR4/ENRTTicTwgUFYq tcbKzQp2FSy2s6VDu6w/qr48WI6+h05YxeUhkMPNtVZbSmYf8FH6o8oSEaDIN8hw4LkeUz 8hsQSKe0j1+E4Iv9GaqLyY+5XCsJj4o= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1706624529; a=rsa-sha256; cv=none; b=mhTEVdp3x/+oXJC1Brz5rkO6mDb2NKLqDkFuel4Bw8SL6GsjFfoz6G5+XqBFTUt4hvl/Lh wHABgYGfyAgowb1TD5VUbbfJ7Vp4LyhS7AgF/Wg8fqTKI5NTlOlV6QiHF2Mgv+no6XOkL5 xhGgV6QB3OcBbazb9SQ25IOoVQvvbJ8= ARC-Authentication-Results: i=1; imf13.hostedemail.com; dkim=none; spf=pass (imf13.hostedemail.com: domain of hch@lst.de designates 213.95.11.211 as permitted sender) smtp.mailfrom=hch@lst.de; dmarc=none Received: by verein.lst.de (Postfix, from userid 2407) id 9A33B227A87; Tue, 30 Jan 2024 15:22:05 +0100 (CET) Date: Tue, 30 Jan 2024 15:22:05 +0100 From: Christoph Hellwig To: Jan Kara Cc: Christoph Hellwig , linux-mm@kvack.org, Matthew Wilcox , Jan Kara , David Howells , Brian Foster , Christian Brauner , "Darrick J. Wong" , linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 19/19] writeback: simplify writeback iteration Message-ID: <20240130142205.GB31330@lst.de> References: <20240125085758.2393327-1-hch@lst.de> <20240125085758.2393327-20-hch@lst.de> <20240130104605.2i6mmdncuhwwwfin@quack3> <20240130141601.GA31330@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240130141601.GA31330@lst.de> User-Agent: Mutt/1.5.17 (2007-11-01) X-Rspamd-Queue-Id: 57E232001E X-Rspam-User: X-Stat-Signature: kdicbc5cyeewdbq3qnr3ba9pd93yr67e X-Rspamd-Server: rspam03 X-HE-Tag: 1706624529-696717 X-HE-Meta: U2FsdGVkX1+8r4DGw1K4M3p0uEAcWjiIav9ekzUsgUScrb0QaWrp1Lu0FYng5lNL7Qj+2ACKHwcUJtsYXnf6Cia1MEzfIeH1Nh5/PgbTjMM2Uj7yvjY8VOZx60cDlyl0CnVS61FzotUmsoKnNxAzgHWZtmI0/JaNSpjIkMxoOgA+6UbkyHQMAwEbZEwJ6TO2EANTLdEZVfKRxxcc2Urw1EVCmY2BzG2RxtYp/fpuTm2NCF/InZ8bQqjmkknJn1/oYYCe3bb0XMYvtKIHNoGHqLNQ+t/dC5emgTls5Q9quej1K+Jh+9phCGYvJaZ9ikmu4z4MasASKeFdfsKMuSk2LhRKd4Mo/GUmMOjo24Tnu/nB1fMr8z+Tl2+Mr2okp35vCr/NKJ00Vco07uKLbqoqO182YGvITdc6jsDetUrta+Jyo+QEUPjRtkMoCZYbYdLkJrzx6pWBtTh21Mn23SjWgbGqCVRErgn4h9v0b23djGSNSLpj9wcfsUcw3cWbeVYS/na+EjwxEB59AJ3cHUnYfI7dpFbsZHHrcQ3yt9v7INqISVWZtMMvxbOxYtC37MKc6M36lcRIqvXFTgPearUA1KyEDOK+4uDWQlTjAohSp9cfIdnkIOO+k10DGLCylh4cp166ndVYHE8a35BPSAGSMJntud+AVdReE+eSsYib1Z9ZwVRuZdEn4MIDvZgCuyavIM9SWjt6y5pc+y3z9A/eBQjIn+M/rqJ2f1xlik34jLSYqP/oc0a3Rv9ciybutOZYCL6wN7t8Ayhw7DGfu2g5iTv0uTiBsGb1yyOktdnj+i6X87p4uz6j2tRyK0TuEBX59xfBt+AMW0ey97iZrzphm5hDLEHfNof5UhQU9aKKHgizQPfvZbiFmYNFMffGb3cc2fwUxS2ePG7qUlNtkg4mNeguaTPpbJhcl6TpeGNnKoViRXNk8oUt7r0kTn27LlJ1HgLe+qmDIKqapVhAlII mJ6zBJb0 wTE9zEuFLfIq0FyMFccUmM/DHZYCsUzpvErsfH+dBSWB4BKI= 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, Jan 30, 2024 at 03:16:01PM +0100, Christoph Hellwig wrote: > On Tue, Jan 30, 2024 at 11:46:05AM +0100, Jan Kara wrote: > > Looking at it now I'm thinking whether we would not be better off to > > completely dump the 'error' argument of writeback_iter() / > > writeback_iter_next() and just make all .writepage implementations set > > wbc->err directly. But that means touching all the ~20 writepage > > implementations we still have... > > Heh. I actually had an earlier version that looked at wbc->err in > the ->writepages callers. But it felt a bit too ugly. > > > > + */ > > > + if (wbc->sync_mode == WB_SYNC_NONE && > > > + (wbc->err || wbc->nr_to_write <= 0)) > > > + goto finish; > > > > I think it would be a bit more comprehensible if we replace the goto with: > > folio_batch_release(&wbc->fbatch); > > if (wbc->range_cyclic) > > mapping->writeback_index = > > folio->index + folio_nr_pages(folio); > > *error = wbc->err; > > return NULL; > > I agree that keeping the logic on when to break and when to set the > writeback_index is good, but duplicating the batch release and error > assignment seems a bit suboptimal. Let me know what you think of the > alternatŃ–ve variant below. And now for real: diff --git a/mm/page-writeback.c b/mm/page-writeback.c index d8fcbac2d72310..3e4aa5bfe75819 100644 --- a/mm/page-writeback.c +++ b/mm/page-writeback.c @@ -2475,10 +2475,23 @@ struct folio *writeback_iter(struct address_space *mapping, * For background writeback just push done_index past this folio * so that we can just restart where we left off and media * errors won't choke writeout for the entire file. + * + * For range cyclic writeback we need to remember where we + * stopped so that we can continue there next time we are + * called. If we hit the last page and there is more work + * to be done, wrap back to the start of the file. + * + * For non-cyclic writeback we always start looking up at the + * beginning of the file if we are called again, which can only + * happen due to -ENOMEM from the file system. */ if (wbc->sync_mode == WB_SYNC_NONE && - (wbc->err || wbc->nr_to_write <= 0)) + (wbc->err || wbc->nr_to_write <= 0)) { + if (wbc->range_cyclic) + mapping->writeback_index = + folio->index + folio_nr_pages(folio); goto finish; + } } else { if (wbc->range_cyclic) wbc->index = mapping->writeback_index; /* prev offset */ @@ -2492,31 +2505,15 @@ struct folio *writeback_iter(struct address_space *mapping, } folio = writeback_get_folio(mapping, wbc); - if (!folio) + if (!folio) { + if (wbc->range_cyclic) + mapping->writeback_index = 0; goto finish; + } return folio; finish: folio_batch_release(&wbc->fbatch); - - /* - * For range cyclic writeback we need to remember where we stopped so - * that we can continue there next time we are called. If we hit the - * last page and there is more work to be done, wrap back to the start - * of the file. - * - * For non-cyclic writeback we always start looking up at the beginning - * of the file if we are called again, which can only happen due to - * -ENOMEM from the file system. - */ - if (wbc->range_cyclic) { - WARN_ON_ONCE(wbc->sync_mode != WB_SYNC_NONE); - if (wbc->err || wbc->nr_to_write <= 0) - mapping->writeback_index = - folio->index + folio_nr_pages(folio); - else - mapping->writeback_index = 0; - } *error = wbc->err; return NULL; }