All of lore.kernel.org
 help / color / mirror / Atom feed
From: "J. Bruce Fields" <bfields@redhat.com>
To: NeilBrown <neilb@suse.de>
Cc: linux-nfs@vger.kernel.org
Subject: Re: [PATCH 2/2] nfsd: give out fewer session slots as limit approaches
Date: Thu, 19 Sep 2019 13:17:30 -0400	[thread overview]
Message-ID: <20190919171730.GB333@pick.fieldses.org> (raw)
In-Reply-To: <20190919162211.GA333@pick.fieldses.org>

On Thu, Sep 19, 2019 at 12:22:11PM -0400, J. Bruce Fields wrote:
> Looking....  Oh, I overlooked this before:
> 
> 	https://tools.ietf.org/html/rfc5661#page-522
> 
> 	The server creates the session by recording the parameter values
> 	used (including whether the CREATE_SESSION4_FLAG_PERSIST flag is
> 	set and has been accepted by the server) and allocating space
> 	for the session reply cache (if there is not enough space, the
> 	server returns NFS4ERR_NOSPC).
> 
> So, slightly odd use of NOSPC, but that's the right error there.

Looking back some more... last year Manjunath Patil sent a patch that
fixed that and I somehow ignored the part where he actually cited that
spec language and said, no, ENOSPC isn't for that:

	https://lore.kernel.org/linux-nfs/20180622175416.GA7119@fieldses.org/

But, Trond seems to disagree with the spec here anyway:

	There are a range of errors which we may need to handle by
	destroying the session, and then creating a new one (mainly the
	ones where the client and server slot handling get out of sync).
	That's why returning NFS4ERR_NOSPC in response to CREATE_SESSION
	is unhelpful, and is why the only sane response by the client
	will be to treat is as a temporary error.

Other todo's from that thread:

	- The protocol allows us to recall existing slots from clients.
	  We can use that when we get close to the limit.  Trond wrote
	  patches to do that some time ago, though said he'd take a
	  different approach now:

		https://lore.kernel.org/linux-nfs/CAABAsM6vDOaudUZYWH23oGiWGqX5Bd1YbCDnL6L=pxzMXgZzaw@mail.gmail.com/

	- I seem to recall the client is asking for rather large slots.
	  It could be a careful look at the xdr code would show we don't
	  need as much?

	- Maybe we should just keep allowing small sessions (1 slot?)
	  even past the limit.  Worst case the subsequent kmalloc fails.

--b.

> 
> >  Also, I'd like to suggest that the '1/3' heuristic be change to 1/16.
> >  Assuming 30 slots get handed out normally (which my testing shows -
> >  about 2k each, with an upper limit of 64k):
> >    When 90 slots left, we hand out
> >     30 (now 60 left)
> >     20 (now 40 left)
> >     13 (now 27 left)
> >      9 (now 18 left)
> >      6 (now 12 left)
> >      4 (now 8 left)
> >      2 (now 6 left)
> >      2 (now 4 left)
> >      1
> >      1
> >      1
> >      1
> >  which is a rapid decline as clients are added.
> >  With 16, we hand out 30 at a time until 480 slots are left (30Meg)
> >  then: 30 28 26 24 23 21 20 19 18 6 15 15 14 13 12 11 10 10 9 9 8 8 7 7
> >     6 6 5 5 5 5 4 4 4 3 3 3 3 3 3 2 2 2 2 2 2 2 2 1 1 1 1 1 1 1 1 1 1 1 1 1
> >     1 1
> >  slots per session
> > 
> >  Am I convincing?
> > 
> > To make it more concrete: this is what I'm thinking of.  Which bits do
> > you like?
> 
> Except for the error return, it looks good to me.
> 
> --b.
> 
> > 
> > Thanks,
> > NeilBrown
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 7857942c5ca6..5d11ceaee998 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -1573,11 +1573,15 @@ static u32 nfsd4_get_drc_mem(struct nfsd4_channel_attrs *ca)
> >  	total_avail = nfsd_drc_max_mem - nfsd_drc_mem_used;
> >  	avail = min((unsigned long)NFSD_MAX_MEM_PER_SESSION, total_avail);
> >  	/*
> > -	 * Never use more than a third of the remaining memory,
> > +	 * Never use more than a 1/16 of the remaining memory,
> >  	 * unless it's the only way to give this client a slot:
> >  	 */
> > -	avail = clamp_t(unsigned long, avail, slotsize, total_avail/3);
> > +	avail = clamp_t(unsigned long, avail, slotsize, total_avail/16);
> >  	num = min_t(int, num, avail / slotsize);
> > +	if (nfsd_drc_mem_used + num * slotsize > nfsd_drc_max_mem)
> > +		/* Completely out of space - sorry */
> > +		num = 0;
> > +
> >  	nfsd_drc_mem_used += num * slotsize;
> >  	spin_unlock(&nfsd_drc_lock);
> >  
> > @@ -3172,7 +3176,7 @@ static __be32 check_forechannel_attrs(struct nfsd4_channel_attrs *ca, struct nfs
> >  	 */
> >  	ca->maxreqs = nfsd4_get_drc_mem(ca);
> >  	if (!ca->maxreqs)
> > -		return nfserr_jukebox;
> > +		return nfserr_resource;
> >  
> >  	return nfs_ok;
> >  }
> 
> 


  reply	other threads:[~2019-09-19 17:17 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-09-25 13:21 [PATCH 0/2] tweak knfsd session slot table sizing J. Bruce Fields
2017-09-25 13:21 ` [PATCH 1/2] nfsd: increase DRC cache limit J. Bruce Fields
2017-09-25 13:21 ` [PATCH 2/2] nfsd: give out fewer session slots as limit approaches J. Bruce Fields
2019-09-19  1:08   ` NeilBrown
2019-09-19 16:22     ` J. Bruce Fields
2019-09-19 17:17       ` J. Bruce Fields [this message]
2019-09-19 18:41         ` J. Bruce Fields
2019-09-20  6:15           ` [PATCH 1/2] nfsd: handle drc over-allocation gracefully NeilBrown
2019-09-20  6:33             ` [PATCH 1/2 - vers2] " NeilBrown
2019-09-20  6:36               ` [PATCH - 2/2] nfsd: degraded slot-count more gracefully as allocation nears exhaustion NeilBrown
2019-09-20 16:28               ` [PATCH 1/2 - vers2] nfsd: handle drc over-allocation gracefully J. Bruce Fields

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190919171730.GB333@pick.fieldses.org \
    --to=bfields@redhat.com \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neilb@suse.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.