Re: [PATCH v1] mm: fix writeback regression for strictlimit BDIs

[Jan Kara <jack@suse.cz>]

On Wed 25-03-26 17:13:37, Joanne Koong wrote:
> Commit 64dd89ae01f2 ("mm/block/fs: remove laptop_mode") removed this
> unconditional writeback kick from balance_dirty_pages():
> 
>        if (unlikely(!writeback_in_progress(wb)))
> 	       wb_start_background_writeback(wb);
> 
> Earlier in balance_dirty_pages(), background writeback is started if the
> global dirty count exceeds the global background threshold (nr_dirty >
> gdtc->bg_thresh). However for BDIs with BDI_CAP_STRICTLIMIT set (eg
> fuse), throttling is calculated using the per-wb threshold, not global
> thresholds. This means the per-wb threshold can be exceeded while the
> global nr_dirty is below gdtc->bg_thresh. This causes two problems:
> 
> a) background writeback is never proactively kicked off. The flusher
> should be kicked off while the writer is still free-running, so that
> dirty pages are drained before the writer hits the throttle threshold.
> For strictlimit BDIs with global nr_dirty < gdtc->bg_thresh, this never
> kicks off the flusher, so dirty pages pile up unchecked until the per-wb
> freerun ceiling gets hit and IO is throttled
> 
> b) while IO is throttled, the flusher is still not started, which means
> the writer basically sits in the throttle loop sleeping while waiting
> for dirty pages to be cleaned but no writeback is running
> 
> This leads to severe stalls and degraded throughput. On fuse, buffered
> write performance drops from 1400 MiB/s to 2000 KiB/s.
> 
> This fixes the issue by kicking off the flusher if wb_dirty exceeds
> wb_bg_thresh for strictlimit BDIs. This restores performance back to its
> original baseline.
> 
> Fixes: 64dd89ae01f2 ("mm/block/fs: remove laptop_mode")
> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>

Good spotting! I think your change makes sense but I don't think it is
enough. The problem is that writeback throttling depends also on memcg
setup so with your fix we still have a problem that when throttling due to
memcg limits the flush work won't be queued early enough / at all. So I
think the best fix is to perhaps do your change so that background
writeback is started earlier for strictlimit bdis but also return back the
unconditional starting of writeback (with properly updated comment) when we
find out we're going to throttle the task for whatever reason.

								Honza

> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 601a5e048d12..3f89b08b11b4 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);
>  
>  		/*
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR