Received-SPF: pass (google.com: domain of linux-kernel+bounces-8303-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Thu, 21 Dec 2023 12:46:43 +0100
From: Jan Kara <jack@suse.cz>
To: Christoph Hellwig <hch@lst.de>
Cc: linux-mm@kvack.org, "Matthew Wilcox (Oracle)" <willy@infradead.org>,
	Jan Kara <jack@suse.com>, David Howells <dhowells@redhat.com>,
	Brian Foster <bfoster@redhat.com>, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 14/17] writeback: Factor writeback_iter_next() out of
 write_cache_pages()
Message-ID: <20231221114643.y5opxgchscyj7rwn@quack3>
References: <20231218153553.807799-1-hch@lst.de>
 <20231218153553.807799-15-hch@lst.de>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231218153553.807799-15-hch@lst.de>

On Mon 18-12-23 16:35:50, Christoph Hellwig wrote:
> From: "Matthew Wilcox (Oracle)" <willy@infradead.org>
> 
> Pull the post-processing of the writepage_t callback into a
> separate function.  That means changing writeback_finish() to
> return NULL, and writeback_get_next() to call writeback_finish()
> when we naturally run out of folios.

The part about writeback_finish() does not seem to be true anymore.
Otherwise feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  mm/page-writeback.c | 85 ++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 40 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 9d37dd5e58ffb6..0771f19950081f 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2449,8 +2449,10 @@ static struct folio *writeback_get_folio(struct address_space *mapping,
>  
>  	for (;;) {
>  		folio = writeback_get_next(mapping, wbc);
> -		if (!folio)
> +		if (!folio) {
> +			writeback_finish(mapping, wbc, 0);
>  			return NULL;
> +		}
>  		folio_lock(folio);
>  		if (likely(should_writeback_folio(mapping, wbc, folio)))
>  			break;
> @@ -2477,6 +2479,46 @@ static struct folio *writeback_iter_init(struct address_space *mapping,
>  	return writeback_get_folio(mapping, wbc);
>  }
>  
> +static struct folio *writeback_iter_next(struct address_space *mapping,
> +		struct writeback_control *wbc, struct folio *folio, int error)
> +{
> +	unsigned long nr = folio_nr_pages(folio);
> +
> +	wbc->nr_to_write -= nr;
> +
> +	/*
> +	 * Handle the legacy AOP_WRITEPAGE_ACTIVATE magic return value.
> +	 * Eventually all instances should just unlock the folio themselves and
> +	 * return 0;
> +	 */
> +	if (error == AOP_WRITEPAGE_ACTIVATE) {
> +		folio_unlock(folio);
> +		error = 0;
> +	}
> +
> +	if (error && !wbc->err)
> +		wbc->err = error;
> +
> +	/*
> +	 * For integrity sync  we have to keep going until we have written all
> +	 * the folios we tagged for writeback prior to entering the writeback
> +	 * loop, even if we run past wbc->nr_to_write or encounter errors.
> +	 * This is because the file system may still have state to clear for
> +	 * each folio.   We'll eventually return the first error encountered.
> +	 *
> +	 * 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.
> +	 */
> +	if (wbc->sync_mode == WB_SYNC_NONE &&
> +	    (wbc->err || wbc->nr_to_write <= 0)) {
> +		writeback_finish(mapping, wbc, folio->index + nr);
> +		return NULL;
> +	}
> +
> +	return writeback_get_folio(mapping, wbc);
> +}
> +
>  /**
>   * 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
> @@ -2517,47 +2559,10 @@ int write_cache_pages(struct address_space *mapping,
>  
>  	for (folio = writeback_iter_init(mapping, wbc);
>  	     folio;
> -	     folio = writeback_get_folio(mapping, wbc)) {
> -		unsigned long nr;
> -
> +	     folio = writeback_iter_next(mapping, wbc, folio, error))
>  		error = writepage(folio, wbc, data);
> -		nr = folio_nr_pages(folio);
> -		wbc->nr_to_write -= nr;
> -
> -		/*
> -		 * Handle the legacy AOP_WRITEPAGE_ACTIVATE magic return value.
> -		 * Eventually all instances should just unlock the folio
> -		 * themselves and return 0;
> -		 */
> -		if (error == AOP_WRITEPAGE_ACTIVATE) {
> -			folio_unlock(folio);
> -			error = 0;
> -		}
> -
> -		if (error && !wbc->err)
> -			wbc->err = error;
>  
> -		/*
> -		 * For integrity sync  we have to keep going until we have
> -		 * written all the folios we tagged for writeback prior to
> -		 * entering this loop, even if we run past wbc->nr_to_write or
> -		 * encounter errors.  This is because the file system may still
> -		 * have state to clear for each folio.   We'll eventually return
> -		 * the first error encountered.
> -		 *
> -		 * 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.
> -		 */
> -		if (wbc->sync_mode == WB_SYNC_NONE &&
> -		    (wbc->err || wbc->nr_to_write <= 0)) {
> -			writeback_finish(mapping, wbc, folio->index + nr);
> -			return error;
> -		}
> -	}
> -
> -	writeback_finish(mapping, wbc, 0);
> -	return 0;
> +	return wbc->err;
>  }
>  EXPORT_SYMBOL(write_cache_pages);
>  
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR