Re: [PATCH v1 1/2] writeback: size foreign flushes by target wb dirty pages

[Jan Kara]

On Tue 08-09-26 11:23:51, Xin Yin wrote:
> Foreign dirty tracking records the bdi and wb memcg IDs of recently
> dirtied foreign inodes. When the source memcg needs foreign writeback,
> mem_cgroup_flush_foreign() queues WB_REASON_FOREIGN_FLUSH to the
> recorded target bdi_writeback.
> 
> cgroup_writeback_by_id() currently sizes this best-effort work from the
> target memcg's NR_FILE_DIRTY counter. The work is scoped to one target
> wb, so a memcg-wide dirty count can over-size a single-bdi flush when
> the target memcg has dirty pages on other devices. It can also under-size
> the flush when the target wb has dirty pages charged to other memcgs.
> 
> Over-sizing keeps the target wb busy longer and can delay later
> writeback work. Under-sizing can finish before enough pages are written
> back for the source memcg.
> 
> Use the target wb's WB_RECLAIMABLE counter and keep the existing 25%
> headroom. This keeps the budget aligned with the writeback object being
> queued.
> 
> Fixes: 97b27821b485 ("writeback, memcg: Implement foreign dirty flushing")
> Signed-off-by: Xin Yin <yinxin.x@xxxxxxxxxxxxx>

Yeah, this looks like an obvious win. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/fs-writeback.c | 17 +++++++----------
>  1 file changed, 7 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c
> index 7c75ed7e8979..7c2340a5dead 100644
> --- a/fs/fs-writeback.c
> +++ b/fs/fs-writeback.c
> @@ -1131,7 +1131,7 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  	struct cgroup_subsys_state *memcg_css;
>  	struct bdi_writeback *wb;
>  	struct wb_writeback_work *work;
> -	unsigned long dirty;
> +	long dirty;
>  	int ret;
>  
>  	/* lookup bdi and memcg */
> @@ -1160,16 +1160,13 @@ int cgroup_writeback_by_id(u64 bdi_id, int memcg_id,
>  	}
>  
>  	/*
> -	 * The caller is attempting to write out most of
> -	 * the currently dirty pages.  Let's take the current dirty page
> -	 * count and inflate it by 25% which should be large enough to
> -	 * flush out most dirty pages while avoiding getting livelocked by
> -	 * concurrent dirtiers.
> -	 *
> -	 * BTW the memcg stats are flushed periodically and this is best-effort
> -	 * estimation, so some potential error is ok.
> +	 * The caller is attempting to write out most of the target wb's
> +	 * currently dirty pages.  Size the work from the wb's reclaimable pages
> +	 * and inflate the count by 25%, which should be large enough to flush
> +	 * out most dirty pages while avoiding getting livelocked by concurrent
> +	 * dirtiers.
>  	 */
> -	dirty = memcg_page_state(mem_cgroup_from_css(memcg_css), NR_FILE_DIRTY);
> +	dirty = wb_stat_sum(wb, WB_RECLAIMABLE);
>  	dirty = dirty * 10 / 8;
>  
>  	/* issue the writeback work */
> -- 
> 2.20.1
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR