Re: [PATCH RFC v9 2/2] nfsd: Initial implementation of NFSv4 Courteous Server

[Chuck Lever III <chuck.lever@oracle.com>]

> On Jan 10, 2022, at 8:03 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> Thank you Chuck for your review, please see reply below:
> 
> On 1/10/22 3:17 PM, Chuck Lever III wrote:
>> Hi Dai-
>> 
>> Still getting the feel of the new approach, but I have
>> made some comments inline...
>> 
>> 
>>> On Jan 10, 2022, at 1:50 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>> 
>>> 	seq_printf(m, "\nminor version: %d\n", clp->cl_minorversion);
>>> @@ -2809,8 +2835,17 @@ find_clp_in_name_tree(struct xdr_netobj *name, struct rb_root *root)
>>> 			node = node->rb_left;
>>> 		else if (cmp < 0)
>>> 			node = node->rb_right;
>>> -		else
>>> -			return clp;
>>> +		else {
>>> +			spin_lock(&clp->cl_cs_lock);
>>> +			if (!test_bit(NFSD4_DESTROY_COURTESY_CLIENT,
>>> +					&clp->cl_flags)) {
>>> +				clear_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags);
>>> +				spin_unlock(&clp->cl_cs_lock);
>>> +				return clp;
>>> +			}
>>> +			spin_unlock(&clp->cl_cs_lock);
>>> +			return NULL;
>>> +		}
>>> 	}
>>> 	return NULL;
>>> }
>>> @@ -2856,6 +2891,14 @@ find_client_in_id_table(struct list_head *tbl, clientid_t *clid, bool sessions)
>>> 		if (same_clid(&clp->cl_clientid, clid)) {
>>> 			if ((bool)clp->cl_minorversion != sessions)
>>> 				return NULL;
>>> +			spin_lock(&clp->cl_cs_lock);
>>> +			if (test_bit(NFSD4_DESTROY_COURTESY_CLIENT,
>>> +					&clp->cl_flags)) {
>>> +				spin_unlock(&clp->cl_cs_lock);
>>> +				continue;
>>> +			}
>>> +			clear_bit(NFSD4_COURTESY_CLIENT, &clp->cl_flags);
>>> +			spin_unlock(&clp->cl_cs_lock);
>> I'm wondering about the transition from COURTESY to active.
>> Does that need to be synchronous with the client tracking
>> database?
> 
> Currently when the client transits from active to COURTESY,
> we do not remove the client record from the tracking database
> so on the reverse we do not need to add it back.
> 
> I think this is something you and Bruce have been discussing
> on whether when we should remove and add the client record from
> the database when the client transits from active to COURTESY
> and vice versa. With this patch we now expire the courtesy clients
> asynchronously in the background so the overhead/delay from
> removing the record from the database does not have any impact
> on resolving conflicts.

As I recall, our idea was to record the client as expired when
it transitions from active to COURTEOUS so that if the server
happens to reboot, it doesn't allow a courteous client to
reclaim locks the server may have already given to another
active client.

So I think the server needs to do an nfsdtrack upcall when
transitioning from active -> COURTEOUS to prevent that edge
case. That would happen only in the laundromat, right?

So when a COURTEOUS client comes back to the server, the server
will need to persistently record the transition from COURTEOUS
to active.

--
Chuck Lever