Re: [PATCH v6 05/16] iomap: Add async buffered write support

[Jan Kara <jack@suse.cz>]

On Fri 27-05-22 08:37:05, Dave Chinner wrote:
> On Thu, May 26, 2022 at 10:38:29AM -0700, Stefan Roesch wrote:
> > This adds async buffered write support to iomap.
> > 
> > This replaces the call to balance_dirty_pages_ratelimited() with the
> > call to balance_dirty_pages_ratelimited_flags. This allows to specify if
> > the write request is async or not.
> > 
> > In addition this also moves the above function call to the beginning of
> > the function. If the function call is at the end of the function and the
> > decision is made to throttle writes, then there is no request that
> > io-uring can wait on. By moving it to the beginning of the function, the
> > write request is not issued, but returns -EAGAIN instead. io-uring will
> > punt the request and process it in the io-worker.
> > 
> > By moving the function call to the beginning of the function, the write
> > throttling will happen one page later.
> 
> Won't it happen one page sooner? I.e. on single page writes we'll
> end up throttling *before* we dirty the page, not *after* we dirty
> the page. IOWs, we can't wait for the page that we just dirtied to
> be cleaned to make progress and so this now makes the loop dependent
> on pages dirtied by other writers being cleaned to guarantee
> forwards progress?
> 
> That seems like a subtle but quite significant change of
> algorithm...

So I'm convinced the difference will be pretty much in the noise because of
how many dirty pages there have to be to even start throttling processes
but some more arguments are:

* we ratelimit calls to balance_dirty_pages() based on number of pages
  dirtied by the current process in balance_dirty_pages_ratelimited()

* balance_dirty_pages() uses number of pages dirtied by the current process
  to decide about the delay.

So the only situation where I could see this making a difference would be
if dirty limit is a handful of pages and even there I have hard time to see
how exactly. So I'm ok with the change and in the case we see it causes
problems somewhere, we'll think how to fix it based on the exact scenario.

I guess the above two points are the reason why Stefan writes about throttling
one page later because we count only number of pages dirtied until this
moment so the page dirtied by this iteration of loop in iomap_write_iter()
will get reflected only by the call to balance_dirty_pages_ratelimited() in
the next iteration (or the next call to iomap_write_iter()).

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