Message-id: <173146150119.1734440.9442770423620311274@noble.neil.brown.name>
Date: Wed, 13 Nov 2024 12:31:41 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Jeff Layton" <jlayton@...nel.org>
Cc: "Chuck Lever" <chuck.lever@...cle.com>, "Dai Ngo" <Dai.Ngo@...cle.com>,
 "Tom Talpey" <tom@...pey.com>, "Olga Kornievskaia" <okorniev@...hat.com>,
 linux-nfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] nfsd: allow for up to 32 callback session slots

On Wed, 13 Nov 2024, Jeff Layton wrote:
> On Wed, 2024-11-13 at 11:07 +1100, NeilBrown wrote:
> > On Wed, 06 Nov 2024, Jeff Layton wrote:
> > > +	spin_lock(&ses->se_lock);
> > > +	if (target > ses->se_cb_highest_slot) {
> > > +		int i;
> > > +
> > > +		target = min(target, NFSD_BC_SLOT_TABLE_MAX);
> > > +
> > > +		/* Growing the slot table. Reset any new sequences to 1 */
> > > +		for (i = ses->se_cb_highest_slot + 1; i <= target; ++i)
> > > +			ses->se_cb_seq_nr[i] = 1;
> > 
> > Where is the justification in the RFC for resetting the sequence
> > numbers?
> > 
> 
> RFC 8881, 18.36:
> 
> 
> 
> [...]
> 
> Once the session is created, the first SEQUENCE or CB_SEQUENCE received
> on a slot MUST have a sequence ID equal to 1; if not, the replier MUST
> return NFS4ERR_SEQ_MISORDERED.

So initialising them all to 1 when the session is created, as you do in
init_session(), is clearly correct.  Reinitialising them after
target_highest_slot_id has been reduced and then increased is not
justified by the above.

> 
> There is also some verbiage in 20.10.6.1.

2.10.6.1 ??

I cannot find anything in there that justifies discarding seq ids from
slots that have been used.  Discarding cached data and allocated memory
to cache future data is certainly justified, but there is no clear
protocol by which the client and server can agree that it is time to
reset the seqid for a particular slot (or range of slots).

Can you point me to what you can find?

> 
> > The csr_target_highest_slotid from the client - which is the value passed as
> > 'target' is defined as:
> > 
> >    the highest slot ID the client would prefer the server use on a
> >    future CB_SEQUENCE operation. 
> > 
> > This is not "the highest slot ID for which the client is remembering
> > sequence numbers".
> > 
> > If we can get rid of this, then I think the need for se_lock evaporates.
> > Allocating a new slow would be
> > 
> > do {
> >  idx = ffs(ses->se_cb_slot_avail) - 1;
> > } while (is_valid(idx) && test_and_set_bit(idx, &ses->se_sb_slot_avail));
> >  
> > where is_valid(idX) is idx >= 0 && idx <= ses->se_sb_highest_slot
> > 
> 
> That certainly would be better.
> 
> Maybe it's not required to start the seqid for a new slot at 1? If a
> new slot can start its sequence counter at an arbitrary value then we
> should be able to do this.

A new slot MUST start with a seqid of 1 when the session is created.  So
the first time a slot is used in a session the seqid must be 1.  The
second time it must be 2.  etc.  But I don't see how that relates to the
code for managing se_sb_slot_avail ....

> > >  	case -NFS4ERR_SEQ_MISORDERED:
> > > -		if (session->se_cb_seq_nr != 1) {
> > > -			session->se_cb_seq_nr = 1;
> > > +		if (session->se_cb_seq_nr[cb->cb_held_slot] != 1) {
> > > +			session->se_cb_seq_nr[cb->cb_held_slot] = 1;
> > 
> > This is weird ...  why do we reset the seq_nr to 1 when we get
> > SEQ_MISORDERED??  Git logs don't shed any light :-(
> > 
> 
> 
> The above verbiage from 18.36 might hint that this is the right thing
> to do, but it's a little vague.

Maybe this code is useful for buggy clients that choose to reset the
seqid for slots that have been unused for a while...  It looks like the
Linux NFS client will reset seqids.  nfs41_set_client_slotid_locked()
records a new target bumping ->generation and
nfs41_set_server_slotid_locked() may then call nfs4_shrink_slot_table()
which discards seqid information.

I still cannot see how it is justified. 

> > > @@ -2132,11 +2135,14 @@ static void init_session(struct svc_rqst *rqstp, struct nfsd4_session *new, stru
> > >  
> > >  	INIT_LIST_HEAD(&new->se_conns);
> > >  
> > > -	new->se_cb_seq_nr = 1;
> > > +	atomic_set(&new->se_ref, 0);
> > >  	new->se_dead = false;
> > >  	new->se_cb_prog = cses->callback_prog;
> > >  	new->se_cb_sec = cses->cb_sec;
> > > -	atomic_set(&new->se_ref, 0);
> > > +
> > > +	for (idx = 0; idx < NFSD_BC_SLOT_TABLE_MAX; ++idx)
> > > +		new->se_cb_seq_nr[idx] = 1;
> > 
> > That should be "<= NFSD_BC_SLOT_TABLE_MAX"
> 
> MAX in this case is the maximum slot index, so this is correct for the
> code as it stands today. I'm fine with redefining the constant to track
> the size of the slot table instead. We could also make the existing
> code more clear by just renaming the existing constant to
> NFSD_BC_SLOT_INDEX_MAX.

What do you mean by "this" in "this is correct for.."??  The code as it
stands today is incorrect as it initialises the se_cb_seq_nr for slots
0..30 but not for slot 31.

> 
> > 
> > I don't think *_MAX is a good choice of name.  It is the maximum number
> > of slots (no) or the maximum slot number (yes).
> > I think *_SIZE would be a better name - the size of the table that we
> > allocate. 32.
> > Looking at where the const is used in current nfsd-next:
> > 
> > 		target = min(target, NFSD_BC_SLOT_TABLE_SIZE - 1
> > 
> > 	new->se_cb_highest_slot = min(battrs->maxreqs,
> > 				      NFSD_BC_SLOT_TABLE_SIZE) - 1;
> > 
> > 	for (idx = 0; idx < NFSD_BC_SLOT_TABLE_SIZE; ++idx)
> > 
> > #define NFSD_BC_SLOT_TABLE_SIZE	(sizeof(u32) * 8)
> > 
> > 	u32			se_cb_seq_nr[NFSD_BC_SLOT_TABLE_SIZE];
> > 
> > which is a slight reduction in the number of "+/-1" adjustments.
> > 
> > 

Thanks,
NeilBrown

Powered by blists - more mailing lists