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: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "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: <848A6498-CFF3-4C66-AE83-959F8221E930@oracle.com>
References: <163090344807.19339.10071205771966144716@noble.neil.brown.name>,
 <848A6498-CFF3-4C66-AE83-959F8221E930@oracle.com>
Date:   Tue, 07 Sep 2021 07:52:07 +1000
Message-id: <163096512753.2518.2763320775379374551@noble.neil.brown.name>
Precedence: bulk

On Tue, 07 Sep 2021, 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.
> > 
> > Every call to congestion_wait() in the entire kernel passes BLK_RW_ASYNC
> > as the first argument, so we should so.
> > 
> > The second argument - an upper limit for waiting - seem fairly
> > arbitrary.  Many places use "HZ/50" or "HZ/10".  As there is no obvious
> > choice, it seems reasonable to leave the maximum time unchanged.
> > 
> > If a service using svc_alloc_args() is terminated, it may now have to
> > wait up to the full 500ms before termination completes as
> > congestion_wait() cannot be interrupted.  I don't believe this will be a
> > problem in practice, though it might be justification for using a
> > smaller timeout.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > 
> > I happened to notice this inconsistency between svc_alloc_args() and
> > xfs_buf_alloc_pages() despite them doing very similar things, so thought
> > I'd send a patch.
> > 
> > NeilBrown
> 
> When we first considered the alloc_pages_bulk() API, the SUNRPC
> patch in that series replaced this schedule_timeout(). Mel
> suggested we postpone that to a separate patch. Now is an ideal
> time to consider this change again. I've added the MM folks for
> expert commentary.
> 
> I would rather see a shorter timeout, since that will be less
> disruptive in practice and today's systems shouldn't have to wait
> that long for free memory to become available. DEFAULT_IO_TIMEOUT
> might be a defensible choice -- it will slow down this loop
> effectively without adding a significant delay.

DEFAULT_IO_TIMEOUT is local to f2fs, so might not be the best choice.
I could be comfortable with any number from 1 to HZ, and have no basis
on how to make a choice - which is why I deliberately avoided making
one.
Ideally, the full timeout would (almost) never expire in practice.
Ideally, the interface would not even ask that we supply a timeout.
But are not currently at the ideal ;-(

Thanks,
NeilBrown