Re: [PATCH 03/17] writeback: rework the loop termination condition in write_cache_pages

[Jan Kara]

On Mon 18-12-23 16:35:39, Christoph Hellwig wrote:
> Rework we deal with the cleanup after the writepage call.  First handle
        ^^ the way

> the magic AOP_WRITEPAGE_ACTIVATE separately from real error returns to
> get it out of the way of the actual error handling.  Then merge the
> code to set ret for integrity vs non-integrity writeback.  For
> non-integrity writeback the loop is terminated on the first error, so
> ret will never be non-zero.  Then use a single block to check for
> non-integrity writewack to consolidate the cases where it returns for
> either an error or running off the end of nr_to_write.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Otherwise looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  mm/page-writeback.c | 62 +++++++++++++++++++++------------------------
>  1 file changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 8e312d73475646..7ed6c2bc8dd51c 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -2474,43 +2474,39 @@ int write_cache_pages(struct address_space *mapping,
>  			error = writepage(folio, wbc, data);
>  			nr = folio_nr_pages(folio);
>  			wbc->nr_to_write -= nr;
> -			if (unlikely(error)) {
> -				/*
> -				 * Handle errors according to the type of
> -				 * writeback. There's no need to continue for
> -				 * background writeback. Just push done_index
> -				 * past this page so media errors won't choke
> -				 * writeout for the entire file. For integrity
> -				 * writeback, we must process the entire dirty
> -				 * set regardless of errors because the fs may
> -				 * still have state to clear for each page. In
> -				 * that case we continue processing and return
> -				 * the first error.
> -				 */
> -				if (error == AOP_WRITEPAGE_ACTIVATE) {
> -					folio_unlock(folio);
> -					error = 0;
> -				} else if (wbc->sync_mode != WB_SYNC_ALL) {
> -					ret = error;
> -					done_index = folio->index + nr;
> -					done = 1;
> -					break;
> -				}
> -				if (!ret)
> -					ret = error;
> +
> +			/*
> +			 * 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;
>  			}
>  
>  			/*
> -			 * We stop writing back only if we are not doing
> -			 * integrity sync. In case of integrity sync we have to
> -			 * keep going until we have written all the pages
> -			 * we tagged for writeback prior to entering this loop.
> +			 * 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.
>  			 */
> -			done_index = folio->index + nr;
> -			if (wbc->nr_to_write <= 0 &&
> -			    wbc->sync_mode == WB_SYNC_NONE) {
> -				done = 1;
> -				break;
> +			if (error && !ret)
> +				ret = error;
> +			if (wbc->sync_mode == WB_SYNC_NONE) {
> +				if (ret || wbc->nr_to_write <= 0) {
> +					done_index = folio->index + nr;
> +					done = 1;
> +					break;
> +				}
>  			}
>  		}
>  		folio_batch_release(&fbatch);
> -- 
> 2.39.2
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR