Re: [PATCH] SUNRPC: use congestion_wait() in svc_alloc_args()

[Mel Gorman <mgorman@suse.com>]

On Tue, Sep 07, 2021 at 08:22:39AM +1000, NeilBrown wrote:
> On Tue, 07 Sep 2021, Matthew Wilcox wrote:
> > On Mon, Sep 06, 2021 at 03:46:34PM +0000, Chuck Lever III wrote:
> > > Hi Neil-
> > > 
> > > > On Sep 6, 2021, at 12:44 AM, NeilBrown <neilb@suse.de> wrote:
> > > > 
> > > > 
> > > > Many places that need to wait before retrying a memory allocation use
> > > > congestion_wait().  xfs_buf_alloc_pages() is a good example which
> > > > follows a similar pattern to that in svc_alloc_args().
> > > > 
> > > > It make sense to do the same thing in svc_alloc_args(); This will allow
> > > > the allocation to be retried sooner if some backing device becomes
> > > > non-congested before the timeout.
> > 
> > It's adorable that you believe this is still true.
> 
> always happy to be called "adorable" !!
> 
> > 
> > https://lore.kernel.org/linux-mm/20191231125908.GD6788@bombadil.infradead.org/
> > 
> > 
> Interesting ...  a few filesystems call clear_bdi_congested(), but not
> enough to make a difference.
> 
> At least my patch won't make things worse.  And when (not if !!)
> congestion_wait() gets fixed, sunrpc will immediately benefit.
> 
> I suspect that "congestion_wait()" needs to be replaced by several
> different interfaces.
> 
> Some callers want to wait until memory might be available, which should
> be tracked entirely by MM, not by filesystems.
> Other caller are really only interested in their own bdi making progress
> and should be allowed to specify that bdi.
> 

For the available memory side, I believe the interface would involve a
waitqueue combined with something like struct capture_control except it
has a waitqueue, a zone, an order, a struct page pointer and a list_head
that is declared on stack. Reclaimers for that zone would check if there
are any such waiters and if so, add a page that has just being reclaimed
and wake the waiter.

That then would be more event driven than time driven which is usually
what mm is meant to do. Despite congestion_wait being known to be broken
for a long time, I don't recall anyone trying to actually fix it.

> And in general, it seems that that waits aren't really interested in
> congestion being eased, but in progress being made.
> 
> reclaim_progress_wait()
> bdi_progress_wait()
> 
> ??
> 
> Even if we just provided
> 
>  void reclaim_progress_wait(void)
>  {
>         schedule_timeout_uninterruptible(HZ/20);
>  }
> 

reclaim_progress_wait at least would clarify that it's waiting on a page
but ultimately, it shouldn't be time based.

-- 
Mel Gorman
SUSE Labs