Re: [PATCH v1 1/1] mm: start background writeback based on per-wb threshold for strictlimit BDIs

[Jan Kara <jack@suse.cz>]

On Thu 26-03-26 16:46:29, Joanne Koong wrote:
> The proactive nr_dirty > gdtc->bg_thresh check in balance_dirty_pages()
> only checks the global dirty threshold to start background writeback
> while the writer is still free-running, but for strictlimit BDIs (eg
> fuse), the per-wb dirty count can exceed the per-wb background threshold
> while the global threshold is not yet exceeded, so background writeback
> for this case never gets proactively started.
> 
> Add a per-wb threshold check for strictlimit BDIs so that background
> writeback is started when wb_dirty exceeds wb_bg_thresh, which drains
> dirty pages before the writer hits the throttle wall, matching the
> proactive behavior that the global check provides for non-strictlimit
> BDIs.
> 
> fio runs on fuse show about a 3-4% improvement in perf for buffered
> writes:
> fio --name=writeback_test --ioengine=psync --rw=write --bs=128k \
>     --size=2G --numjobs=4 --ramp_time=10 --runtime=20 \
>     --time_based --group_reporting=1 --direct=0
> 
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

Looks good. Thanks for improving these! Feel free to add:

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

								Honza

> ---
>  mm/page-writeback.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index c1a4b32af1a7..30f3d5a6270f 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1835,7 +1835,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  			balance_domain_limits(mdtc, strictlimit);
>  		}
>  
> -		if (nr_dirty > gdtc->bg_thresh && !writeback_in_progress(wb))
> +		if (!writeback_in_progress(wb) &&
> +		    (nr_dirty > gdtc->bg_thresh ||
> +		     (strictlimit && gdtc->wb_dirty > gdtc->wb_bg_thresh)))
>  			wb_start_background_writeback(wb);
>  
>  		/*
> @@ -1862,15 +1864,9 @@ static int balance_dirty_pages(struct bdi_writeback *wb,
>  		 * 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
> +		 * doesn't account for the memcg-based throttling case. 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
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR