> On Jan 10, 2022, at 8:03 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> 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@xxxxxxxxxx> 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
