Re: [PATCH v2 1/1] mm: reinstate unconditional writeback start in balance_dirty_pages()

[Jan Kara <jack@suse.cz>]

On Thu 26-03-26 14:51:27, Joanne Koong wrote:
> Commit 64dd89ae01f2 ("mm/block/fs: remove laptop_mode") removed this
> unconditional writeback start from balance_dirty_pages():
> 
>        if (unlikely(!writeback_in_progress(wb)))
> 	       wb_start_background_writeback(wb);
> 
> This logic needs to be reinstated to prevent performance regressions for
> strictlimited BDIs and memcg setups. The problem occurs because:
> 
> a) For strictlimited BDIs, throttling is calculated using per-wb
> thresholds. The per-wb threshold can be exceeded even when the global
> dirty threshold was not exceeded (nr_dirty < gdtc->bg_thresh)
> 
> b) For memcg-based throttling, memcg uses its own dirty count /
> thresholds and can trigger throttling even when the global threshold
> isn't exceeded
> 
> Without the unconditional writeback start, IO is throttled as it waits
> for dirty pages to be written back but there is no writeback running.
> This leads to severe stalls. On fuse, buffered write performance
> dropped from 1400 MiB/s to 2000 KiB/s.
> 
> Reinstate the unconditional writeback start so that writeback is
> guaranteed to be running whenever IO needs to be throttled.
> 
> Fixes: 64dd89ae01f2 ("mm/block/fs: remove laptop_mode")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

Looks good. Feel free to add:

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

								Honza

> ---
>  mm/page-writeback.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 601a5e048d12..c1a4b32af1a7 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1858,6 +1858,27 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  			break;
>  		}
>  
> +		/*
> +		 * Unconditionally start background writeback if it's not
> +		 * already in progress. We need to do this because the global
> +		 * dirty threshold check above (nr_dirty > gdtc->bg_thresh)
> +		 * doesn't account for these cases:
> +		 *
> +		 * a) strictlimit BDIs: throttling is calculated using per-wb
> +		 * thresholds. The per-wb threshold can be exceeded even when
> +		 * nr_dirty < gdtc->bg_thresh
> +		 *
> +		 * b) memcg-based throttling: memcg uses its own dirty count and
> +		 * thresholds and can trigger throttling even when global
> +		 * nr_dirty < gdtc->bg_thresh
> +		 *
> +		 * Writeback needs to be started else the writer stalls in the
> +		 * throttle loop waiting for dirty pages to be written back
> +		 * while no writeback is running.
> +		 */
> +		if (unlikely(!writeback_in_progress(wb)))
> +			wb_start_background_writeback(wb);
> +
>  		mem_cgroup_flush_foreign(wb);
>  
>  		/*
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR