Message-ID: <acf8d86338c881b6837d85399bfca406f1e1c0a3.camel@kernel.org>
Date: Wed, 13 Nov 2024 10:15:49 -0500
From: Jeff Layton <jlayton@...nel.org>
To: NeilBrown <neilb@...e.de>
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, 2024-11-13 at 12:31 +1100, NeilBrown wrote:
> 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.
> 

But, once the client and server have forgotten about those slots after
shrinking the slot table, aren't they effectively new? IOW, once you've
shrunk the slot table, the slots are effectively "freed". Growing it
means that you have to allocate new ones. The fact that this patch just
keeps them around is an implementation detail.

 
> > 
> > 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?
>

I guess I'm stuck on this idea that shrinking the slot table
effectively frees those slots, so if you grow it again later, you have
to consider those slots to be new.

> > 
> > > 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 ....
> 

It doesn't. The se_lock was just a simple way to deal with the table
resizing.

> > > >  	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. 
> 

Fair enough. I'm fine with doing this locklessly if the starting seqid
values truly don't matter. I fear they do though.


> > > > @@ -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.
> 

My bad, you're correct. I'll plan to fix that up once I'm a little
clearer on what the next iteration needs to look like.

> > 
> > > 
> > > 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

-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists