Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Matthew Wilcox" <willy@infradead.org>
Cc:     "Chuck Lever III" <chuck.lever@oracle.com>,
        "Bruce Fields" <bfields@fieldses.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>,
        "Mel Gorman" <mgorman@suse.com>, "Linux-MM" <linux-mm@kvack.org>
Subject: Re: [PATCH] SUNRPC: use congestion_wait() in svc_alloc_args()
In-reply-to: <YTZ4E0Zh6F/WSpy0@casper.infradead.org>
References: <163090344807.19339.10071205771966144716@noble.neil.brown.name>,
 <848A6498-CFF3-4C66-AE83-959F8221E930@oracle.com>,
 <YTZ4E0Zh6F/WSpy0@casper.infradead.org>
Date:   Tue, 07 Sep 2021 08:22:39 +1000
Message-id: <163096695999.2518.10383290668057550257@noble.neil.brown.name>
Precedence: bulk

On Tue, 07 Sep 2021, Matthew Wilcox wrote:
> On Mon, Sep 06, 2021 at 03:46:34PM +0000, Chuck Lever III wrote:
> > Hi Neil-
> >=20
> > > On Sep 6, 2021, at 12:44 AM, NeilBrown <neilb@suse.de> wrote:
> > >=20
> > >=20
> > > 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().
> > >=20
> > > 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.
>=20
> It's adorable that you believe this is still true.

always happy to be called "adorable" !!

>=20
> https://lore.kernel.org/linux-mm/20191231125908.GD6788@bombadil.infradead.o=
rg/
>=20
>=20
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.

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);
 }

that would be an improvement.  It could be called from svc_alloc_args()
and various other places that want to wait before retrying an
allocation, and it would be no worse than current behaviour.
Then if anyone had a clever idea of how to get a notification when
progress had been made, there would be an obvious place to implement
that idea.

NeilBrown