Re: [RFC PATCH v1 15/18] mm: support write throttling for async buffered writes

[Jan Kara <jack@suse.cz>]

On Tue 26-04-22 10:43:32, Stefan Roesch wrote:
> This change adds support for async write throttling in the function
> balance_dirty_pages(). So far if throttling was required, the code was
> waiting synchronously as long as the writes were throttled. This change
> introduces asynchronous throttling. Instead of waiting in the function
> balance_dirty_pages(), the timeout is set in the task_struct field
> bdp_pause. Once the timeout has expired, the writes are no longer
> throttled.
> 
> - Add a new parameter to the balance_dirty_pages() function
>   - This allows the caller to pass in the nowait flag
>   - When the nowait flag is specified, the code does not wait in
>     balance_dirty_pages(), but instead stores the wait expiration in the
>     new task_struct field bdp_pause.
> 
> - The function balance_dirty_pages_ratelimited() resets the new values
>   in the task_struct, once the timeout has expired
> 
> This change is required to support write throttling for the async
> buffered writes. While the writes are throttled, io_uring still can make
> progress with processing other requests.
> 
> Signed-off-by: Stefan Roesch <shr@fb.com>

Maybe I miss something but I don't think this will throttle writers enough.
For three reasons:

1) The calculated throttling pauses should accumulate for the task so that
if we compute that say it takes 0.1s to write 100 pages and the task writes
300 pages, the delay adds up to 0.3s properly. Otherwise the task would not
be throttled as long as we expect the writeback to take.

2) We must not allow the amount of dirty pages to exceed the dirty limit.
That can easily lead to page reclaim getting into trouble reclaiming pages
and thus machine stalls, oom kills etc. So if we are coming close to dirty
limit and we cannot sleep, we must just fail the nowait write.

3) Even with above two problems fixed I suspect results will be suboptimal
because balance_dirty_pages() heuristics assume they get called reasonably
often and throttle writes so if amount of dirty pages is coming close to
dirty limit, they think we are overestimating writeback speed and update
throttling parameters accordingly. So if io_uring code does not throttle
writers often enough, I think dirty throttling parameters will be jumping
wildly resulting in poor behavior.

So what I'd probably suggest is that if balance_dirty_pages() is called in
"async" mode, we'd give tasks a pass until dirty_freerun_ceiling(). If
balance_dirty_pages() decides the task needs to wait, we store the pause
and bail all the way up into the place where we can sleep (io_uring code I
assume), sleep there, and then continue doing write.

								Honza

-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR