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 22083C48291 for ; Mon, 5 Feb 2024 11:39:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id AA16C6B0078; Mon, 5 Feb 2024 06:39:44 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id A51B06B007B; Mon, 5 Feb 2024 06:39:44 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8CABE6B007D; Mon, 5 Feb 2024 06:39:44 -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 7B0286B0078 for ; Mon, 5 Feb 2024 06:39:44 -0500 (EST) Received: from smtpin05.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 1F913C0237 for ; Mon, 5 Feb 2024 11:39:44 +0000 (UTC) X-FDA: 81757555488.05.2A6FDEE Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.223.131]) by imf25.hostedemail.com (Postfix) with ESMTP id 651F7A0015 for ; Mon, 5 Feb 2024 11:39:41 +0000 (UTC) Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=DiWSnUbU; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=6lQcoCAj; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=DiWSnUbU; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=6lQcoCAj; dmarc=none; spf=pass (imf25.hostedemail.com: domain of jack@suse.cz designates 195.135.223.131 as permitted sender) smtp.mailfrom=jack@suse.cz ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1707133182; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=fIuM3Q6VzAnAPyVLX/OqZgZYlF68gRw2R0OsX5yqsfw=; b=OdhVRYYt6rYAKhuQC/35iZ6EDkeQvg/b0az632K+9Or2jkU6S4IP/ZzShe1b1B0MYgY+Rz KOoKFDUB30Nxvki6wTdHRQ/+W7MoX1KjVfYSwc7RdM0u9vHm5I9+OJanPsERIaVItq6+m+ n9TiTsDlUIp7C9/8MbGpvZLRL1826Rw= ARC-Authentication-Results: i=1; imf25.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=DiWSnUbU; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=6lQcoCAj; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=DiWSnUbU; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=6lQcoCAj; dmarc=none; spf=pass (imf25.hostedemail.com: domain of jack@suse.cz designates 195.135.223.131 as permitted sender) smtp.mailfrom=jack@suse.cz ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1707133182; a=rsa-sha256; cv=none; b=w7IRmE1wk61aV4y4SBncReK6DuyZDerzS5XRTcQ8/gyVreLMA38MhoCWv/VQ5aprbh5eWE F+CncDI/IKpouIQrtGv+uPKvsSNWxyAbWWjjcjtJ2yOTuG2uR78F6uq/T8do2Cg0R3nPyQ RlipZDLC4VjTZ+JFqF7ThigHbkMkXAE= Received: from imap1.dmz-prg2.suse.org (imap1.dmz-prg2.suse.org [IPv6:2a07:de40:b281:104:10:150:64:97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 849FB1F8BB; Mon, 5 Feb 2024 11:39:39 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1707133179; 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=fIuM3Q6VzAnAPyVLX/OqZgZYlF68gRw2R0OsX5yqsfw=; b=DiWSnUbU33o+QdXrbCoPoyiOxTxQAWeX++TkNFX3HMbVhYD83OnnGoLlh5ct2bR3i5ddgZ FKSSe4x2K/snmwpt+IHnWYpr8Kqinebr0tCxEXPYDW9YrROGhTJD1hPr5rUXL0ZLq7DW8F mo8dE4pdt/yq77JGMTvIUMl/83elFOQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1707133179; 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=fIuM3Q6VzAnAPyVLX/OqZgZYlF68gRw2R0OsX5yqsfw=; b=6lQcoCAj0LI1ZrFqiK+bB1kD5fg8wqKi1BpR0BSZUP2J7qTPXC+LnSZ9po6VTpx7PmGDXo x6qBGb8z89cEZqDA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1707133179; 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=fIuM3Q6VzAnAPyVLX/OqZgZYlF68gRw2R0OsX5yqsfw=; b=DiWSnUbU33o+QdXrbCoPoyiOxTxQAWeX++TkNFX3HMbVhYD83OnnGoLlh5ct2bR3i5ddgZ FKSSe4x2K/snmwpt+IHnWYpr8Kqinebr0tCxEXPYDW9YrROGhTJD1hPr5rUXL0ZLq7DW8F mo8dE4pdt/yq77JGMTvIUMl/83elFOQ= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1707133179; 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=fIuM3Q6VzAnAPyVLX/OqZgZYlF68gRw2R0OsX5yqsfw=; b=6lQcoCAj0LI1ZrFqiK+bB1kD5fg8wqKi1BpR0BSZUP2J7qTPXC+LnSZ9po6VTpx7PmGDXo x6qBGb8z89cEZqDA== Received: from imap1.dmz-prg2.suse.org (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by imap1.dmz-prg2.suse.org (Postfix) with ESMTPS id 71D44136F5; Mon, 5 Feb 2024 11:39:39 +0000 (UTC) Received: from dovecot-director2.suse.de ([2a07:de40:b281:106:10:150:64:167]) by imap1.dmz-prg2.suse.org with ESMTPSA id eziDG/vIwGWGQwAAD6G6ig (envelope-from ); Mon, 05 Feb 2024 11:39:39 +0000 Received: by quack3.suse.cz (Postfix, from userid 1000) id 0EF07A0809; Mon, 5 Feb 2024 12:39:31 +0100 (CET) Date: Mon, 5 Feb 2024 12:39:31 +0100 From: Jan Kara To: Christoph Hellwig Cc: 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 12/13] writeback: add a writeback iterator Message-ID: <20240205113931.m4sictdqr3gd5rtl@quack3> References: <20240203071147.862076-1-hch@lst.de> <20240203071147.862076-13-hch@lst.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20240203071147.862076-13-hch@lst.de> X-Rspamd-Queue-Id: 651F7A0015 X-Rspam-User: X-Rspamd-Server: rspam05 X-Stat-Signature: r1cthck3ag1h8idxkibznhe3bazy4xc8 X-HE-Tag: 1707133181-624198 X-HE-Meta: U2FsdGVkX1/aOwNbO9pRZSv2ejNYvzxyGJvo8tJ6ZdgYjiPfbyW8apHn2biQvu+5slDLTNpkgU/CU5Zta1BsZda9j69FpkUh4igAvHPLiVjZwDoaWlAA4fF+NqsLmYdVLT/VnDo2/G0yKxlW0QwprozVTJ2CFJY74HMnSt6yo1GDwIWH10W7X38camF3oenasxQ3jHd8dwDYD2o1iai/PSiURop3WDO45ehpNcvdtX6nCtbEp/H1Fq5bddMza8hDnx3NOW3hyFtdqbArHKTYdfnUAsosGUQazuvpJqOLQp4JFgPV4RtGhvWO/veF2UzY+JDk1657A7x7/Yle0Oc5HRk/CFvhntn8JfQKAeaIaua1Wf0zo2yRivP2UlVG0Gk/9I0li1NZgz8xjYSESrmbOILWvOSbJQp9nYEwamfn3GflxuEbVwzlBbagu9fUvlfOH2YwpNA61/QWxTUMPrXWTJWNwRQ+Ht/YbIGtjgYJkGQOEQ41JECW5BBLzgVp/RwpTZ/KredZqVI8WkCDbkfZl5L1JgFslwSucuJk3940MiQYXbmH8/NdrEOKn1mHkQDl7ToXB6Jq1fg7NXgHJj6xbLOMpHjRqyF8hiCdfqpDdx0fi9wfeFzzEiswjOQgN2ZgsEoaChQjEOFf3TsYfCx+M33hpsAC5GCB8lTL04BvO9PwAGGsynyNRY3H3FjElvHMPAwbVI76O9YeYW6gGg94rmaKc9PbKBRn+8DaYYHcQYsklhpDJiUp4K3xWa7d3J6Lzvmo86NOiI5bDOvuvJmkg427F7CrgAWuUMkfEawMIAZztgryCIm2oJgOyrBZcUu2iNtd/MWh/+t854f2uiASZSifpk07CvN+mWjcacrB0T54rj8pBpw5iCRYe+1GmSewz2AOWLqfQbP/PGd9G+INqbveJUBft+EJ96HcPl0r2RBp/71a1AlvMtR4kQR5DoSaIAmkSKcz00lbetXEg15 ehxuMfUV WeYv6EPkDpDJ67ADVhMaHeCoUUynjV9pS4Hfl9oKmg9nHXEG3sKgX1U3w/y9lCOYaYZjmiA5lcauCiV3BN+1z4WGdiGewdobfCxVs0g1UpCxB3Gvm+I4EtINpjdjr2YHkmcfeUMbzWhKNYbTQYsR1Zt3m2juVowYR1TrWYHbxqvBfirz7DSoHH9u4Un1pHNe8ZMQ26WcjmsJu7q4D7fGgpXFmVUxQoH+KHD/SPxNnyK/fs9KWp2a0Ol1mpvqdIp6+lbP1igPfd0AFKVMTYASpGmMDN9O+NoZBgCIWPj/wttebPIhjRMV/vTGWcw== 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 Sat 03-02-24 08:11:46, Christoph Hellwig wrote: > Refactor the code left in write_cache_pages into an iterator that the > file system can call to get the next folio for a writeback operation: > > struct folio *folio = NULL; > > while ((folio = writeback_iter(mapping, wbc, folio, &error))) { > error = ; > } > > The twist here is that the error value is passed by reference, so that > the iterator can restore it when breaking out of the loop. > > Handling of the magic AOP_WRITEPAGE_ACTIVATE value stays outside the > iterator and needs is just kept in the write_cache_pages legacy wrapper. > in preparation for eventually killing it off. > > Heavily based on a for_each* based iterator from Matthew Wilcox. > > Signed-off-by: Christoph Hellwig Looks good. Feel free to add: Reviewed-by: Jan Kara Honza > --- > include/linux/writeback.h | 4 + > mm/page-writeback.c | 192 ++++++++++++++++++++++---------------- > 2 files changed, 118 insertions(+), 78 deletions(-) > > diff --git a/include/linux/writeback.h b/include/linux/writeback.h > index f67b3ea866a0fb..9845cb62e40b2d 100644 > --- a/include/linux/writeback.h > +++ b/include/linux/writeback.h > @@ -82,6 +82,7 @@ struct writeback_control { > /* internal fields used by the ->writepages implementation: */ > struct folio_batch fbatch; > pgoff_t index; > + int saved_err; > > #ifdef CONFIG_CGROUP_WRITEBACK > struct bdi_writeback *wb; /* wb this writeback is issued under */ > @@ -366,6 +367,9 @@ int balance_dirty_pages_ratelimited_flags(struct address_space *mapping, > > bool wb_over_bg_thresh(struct bdi_writeback *wb); > > +struct folio *writeback_iter(struct address_space *mapping, > + struct writeback_control *wbc, struct folio *folio, int *error); > + > typedef int (*writepage_t)(struct folio *folio, struct writeback_control *wbc, > void *data); > > diff --git a/mm/page-writeback.c b/mm/page-writeback.c > index 3abb053e70580e..5fe4cdb7dbd61a 100644 > --- a/mm/page-writeback.c > +++ b/mm/page-writeback.c > @@ -2325,18 +2325,18 @@ void __init page_writeback_init(void) > } > > /** > - * tag_pages_for_writeback - tag pages to be written by write_cache_pages > + * tag_pages_for_writeback - tag pages to be written by writeback > * @mapping: address space structure to write > * @start: starting page index > * @end: ending page index (inclusive) > * > * This function scans the page range from @start to @end (inclusive) and tags > - * all pages that have DIRTY tag set with a special TOWRITE tag. The idea is > - * that write_cache_pages (or whoever calls this function) will then use > - * TOWRITE tag to identify pages eligible for writeback. This mechanism is > - * used to avoid livelocking of writeback by a process steadily creating new > - * dirty pages in the file (thus it is important for this function to be quick > - * so that it can tag pages faster than a dirtying process can create them). > + * all pages that have DIRTY tag set with a special TOWRITE tag. The caller > + * can then use the TOWRITE tag to identify pages eligible for writeback. > + * This mechanism is used to avoid livelocking of writeback by a process > + * steadily creating new dirty pages in the file (thus it is important for this > + * function to be quick so that it can tag pages faster than a dirtying process > + * can create them). > */ > void tag_pages_for_writeback(struct address_space *mapping, > pgoff_t start, pgoff_t end) > @@ -2434,69 +2434,68 @@ static struct folio *writeback_get_folio(struct address_space *mapping, > } > > /** > - * write_cache_pages - walk the list of dirty pages of the given address space and write all of them. > + * writeback_iter - iterate folio of a mapping for writeback > * @mapping: address space structure to write > - * @wbc: subtract the number of written pages from *@wbc->nr_to_write > - * @writepage: function called for each page > - * @data: data passed to writepage function > + * @wbc: writeback context > + * @folio: previously iterated folio (%NULL to start) > + * @error: in-out pointer for writeback errors (see below) > * > - * If a page is already under I/O, write_cache_pages() skips it, even > - * if it's dirty. This is desirable behaviour for memory-cleaning writeback, > - * but it is INCORRECT for data-integrity system calls such as fsync(). fsync() > - * and msync() need to guarantee that all the data which was dirty at the time > - * the call was made get new I/O started against them. If wbc->sync_mode is > - * WB_SYNC_ALL then we were called for data integrity and we must wait for > - * existing IO to complete. > - * > - * To avoid livelocks (when other process dirties new pages), we first tag > - * pages which should be written back with TOWRITE tag and only then start > - * writing them. For data-integrity sync we have to be careful so that we do > - * not miss some pages (e.g., because some other process has cleared TOWRITE > - * tag we set). The rule we follow is that TOWRITE tag can be cleared only > - * by the process clearing the DIRTY tag (and submitting the page for IO). > - * > - * To avoid deadlocks between range_cyclic writeback and callers that hold > - * pages in PageWriteback to aggregate IO until write_cache_pages() returns, > - * we do not loop back to the start of the file. Doing so causes a page > - * lock/page writeback access order inversion - we should only ever lock > - * multiple pages in ascending page->index order, and looping back to the start > - * of the file violates that rule and causes deadlocks. > + * This function returns the next folio for the writeback operation described by > + * @wbc on @mapping and should be called in a while loop in the ->writepages > + * implementation. > * > - * Return: %0 on success, negative error code otherwise > + * To start the writeback operation, %NULL is passed in the @folio argument, and > + * for every subsequent iteration the folio returned previously should be passed > + * back in. > + * > + * If there was an error in the per-folio writeback inside the writeback_iter() > + * loop, @error should be set to the error value. > + * > + * Once the writeback described in @wbc has finished, this function will return > + * %NULL and if there was an error in any iteration restore it to @error. > + * > + * Note: callers should not manually break out of the loop using break or goto > + * but must keep calling writeback_iter() until it returns %NULL. > + * > + * Return: the folio to write or %NULL if the loop is done. > */ > -int write_cache_pages(struct address_space *mapping, > - struct writeback_control *wbc, writepage_t writepage, > - void *data) > +struct folio *writeback_iter(struct address_space *mapping, > + struct writeback_control *wbc, struct folio *folio, int *error) > { > - int ret = 0; > - int error; > - struct folio *folio; > - pgoff_t end; /* Inclusive */ > - > - if (wbc->range_cyclic) { > - wbc->index = mapping->writeback_index; /* prev offset */ > - end = -1; > - } else { > - wbc->index = wbc->range_start >> PAGE_SHIFT; > - end = wbc->range_end >> PAGE_SHIFT; > - } > - if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages) > - tag_pages_for_writeback(mapping, wbc->index, end); > - > - folio_batch_init(&wbc->fbatch); > + if (!folio) { > + folio_batch_init(&wbc->fbatch); > + wbc->saved_err = *error = 0; > > - for (;;) { > - folio = writeback_get_folio(mapping, wbc); > - if (!folio) > - break; > + /* > + * For range cyclic writeback we remember where we stopped so > + * that we can continue where we stopped. > + * > + * For non-cyclic writeback we always start at the beginning of > + * the passed in range. > + */ > + if (wbc->range_cyclic) > + wbc->index = mapping->writeback_index; > + else > + wbc->index = wbc->range_start >> PAGE_SHIFT; > > - error = writepage(folio, wbc, data); > + /* > + * To avoid livelocks when other processes dirty new pages, we > + * first tag pages which should be written back and only then > + * start writing them. > + * > + * For data-integrity writeback we have to be careful so that we > + * do not miss some pages (e.g., because some other process has > + * cleared the TOWRITE tag we set). The rule we follow is that > + * TOWRITE tag can be cleared only by the process clearing the > + * DIRTY tag (and submitting the page for I/O). > + */ > + if (wbc->sync_mode == WB_SYNC_ALL || wbc->tagged_writepages) > + tag_pages_for_writeback(mapping, wbc->index, > + wbc_end(wbc)); > + } else { > wbc->nr_to_write -= folio_nr_pages(folio); > > - if (error == AOP_WRITEPAGE_ACTIVATE) { > - folio_unlock(folio); > - error = 0; > - } > + WARN_ON_ONCE(*error > 0); > > /* > * For integrity writeback we have to keep going until we have > @@ -2510,33 +2509,70 @@ int write_cache_pages(struct address_space *mapping, > * wbc->nr_to_write or encounter the first error. > */ > if (wbc->sync_mode == WB_SYNC_ALL) { > - if (error && !ret) > - ret = error; > + if (*error && !wbc->saved_err) > + wbc->saved_err = *error; > } else { > - if (error || wbc->nr_to_write <= 0) > + if (*error || wbc->nr_to_write <= 0) > goto done; > } > } > > - /* > - * 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. > - */ > - folio_batch_release(&wbc->fbatch); > - if (wbc->range_cyclic) > - mapping->writeback_index = 0; > - return ret; > + folio = writeback_get_folio(mapping, wbc); > + if (!folio) { > + /* > + * To avoid deadlocks between range_cyclic writeback and callers > + * that hold pages in PageWriteback to aggregate I/O until > + * the writeback iteration finishes, we do not loop back to the > + * start of the file. Doing so causes a page lock/page > + * writeback access order inversion - we should only ever lock > + * multiple pages in ascending page->index order, and looping > + * back to the start of the file violates that rule and causes > + * deadlocks. > + */ > + if (wbc->range_cyclic) > + mapping->writeback_index = 0; > + > + /* > + * Return the first error we encountered (if there was any) to > + * the caller. > + */ > + *error = wbc->saved_err; > + } > + return folio; > > done: > folio_batch_release(&wbc->fbatch); > if (wbc->range_cyclic) > mapping->writeback_index = folio->index + folio_nr_pages(folio); > + return NULL; > +} > + > +/** > + * write_cache_pages - walk the list of dirty pages of the given address space and write all of them. > + * @mapping: address space structure to write > + * @wbc: subtract the number of written pages from *@wbc->nr_to_write > + * @writepage: function called for each page > + * @data: data passed to writepage function > + * > + * Return: %0 on success, negative error code otherwise > + * > + * Note: please use writeback_iter() instead. > + */ > +int write_cache_pages(struct address_space *mapping, > + struct writeback_control *wbc, writepage_t writepage, > + void *data) > +{ > + struct folio *folio = NULL; > + int error; > + > + while ((folio = writeback_iter(mapping, wbc, folio, &error))) { > + error = writepage(folio, wbc, data); > + if (error == AOP_WRITEPAGE_ACTIVATE) { > + folio_unlock(folio); > + error = 0; > + } > + } > + > return error; > } > EXPORT_SYMBOL(write_cache_pages); > -- > 2.39.2 > -- Jan Kara SUSE Labs, CR