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

["NeilBrown" <neilb@suse.de>]

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