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

From: Stefan Roesch <shr@fb.com>
To: Jan Kara <jack@suse.cz>
Cc: io-uring@vger.kernel.org, kernel-team@fb.com, linux-mm@kvack.org,
	linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	david@fromorbit.com
Subject: Re: [RFC PATCH v1 15/18] mm: support write throttling for async buffered writes
Date: Fri, 13 May 2022 11:57:52 -0700	[thread overview]
Message-ID: <7b021526-d6bf-cb23-0864-689dd83d6d78@fb.com> (raw)
In-Reply-To: <20220511103819.e2irxxm2tvb3k7cc@quack3.lan>

On 5/11/22 3:38 AM, Jan Kara wrote:
> On Tue 10-05-22 13:16:30, Stefan Roesch wrote:
>> On 5/10/22 2:50 AM, Jan Kara wrote:
>>> I know that you're using fields in task_struct to propagate the delay info.
>>> But IMHO that is unnecessary (although I don't care too much). Instead we
>>> could factor out a variant of balance_dirty_pages() that returns 'pause' to
>>> sleep, 0 if no sleeping needed. Normal balance_dirty_pages() would use this
>>> for pause calculation, places wanting async throttling would only get the
>>> pause to sleep. So e.g. iomap_write_iter() would then check and if returned
>>> pause is > 0, it would abort the loop similary as we'd abort it for any
>>> other reason when NOWAIT write is aborted because we need to sleep. Iouring
>>> code then detects short write / EAGAIN and offloads the write to the
>>> workqueue where normal balance_dirty_pages() can sleep as needed.
>>>
>>> This will make sure dirty limits are properly observed and we don't need
>>> that much special handling for it.
>>>
>>
>> I like the idea of factoring out a function out balance_dirty_pages(), however
>>
>> I see two challenges:
>> - the write operation has already completed at this point,
>> - so we can't really sleep on its completion in the io-worker in io-uring
>> - we don't know how long to sleep in io-uring
>>
>> Currently balance_dirty_pages_ratelimited() is called at the end of the
>> function iomap_write_iter(). If the function
>> balance_dirty_pages_ratelimited() would instead be called at the
>> beginning of the function iomap_write_iter() we could return -EAGAIN and
>> then complete it in the io-worker.
> 
> Well, we call balance_dirty_pages_ratelimited() after each page. So it does
> not really matter much if the sleep is pushed to happen one page later.
> balance_dirty_pages_ratelimited() does ratelimiting of when
> balance_dirty_pages() are called so we have to make sure
> current->nr_dirtied is not zeroed out before we really do wait (because
> that is what determines whether we enter balance_dirty_pages() and how long
> we sleep there) but looking at the code that should work out just fine.
> 

I'll make the changes to balance_dirty_pages() for the next version of the
patch series.

> 								Honza