Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b60da517-1179-1588-ee11-a9c60400da4a@oracle.com>
Date:   Fri, 8 Apr 2022 14:00:55 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.7.0
Subject: Re: [PATCH RFC v20 09/10] NFSD: Update laundromat to handle courtesy
 client
Content-Language: en-US
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     chuck.lever@oracle.com, linux-nfs@vger.kernel.org
References: <1649285133-16765-1-git-send-email-dai.ngo@oracle.com>
 <1649285133-16765-10-git-send-email-dai.ngo@oracle.com>
 <20220408163135.GA6423@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20220408163135.GA6423@fieldses.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bmF0YWJtREF6aXFFYXRlSnVvZmgxbUF5ZWYzY2RQMS9XQmVnZXF1YkNXMUZp?=
 =?utf-8?B?TmMyZFV5eXlma01QSGRhMnZjRHdkVUxyazRWdXRubWtHaC81NXkvd3RNYXRY?=
 =?utf-8?B?UjJSQ2hYWC9lYTA4TklWUkJGZWk0VEd1TEtNRmxva2FGVExBK0Zwdkw2a1FG?=
 =?utf-8?B?UFZsM0JmNzU2WUZPcjFYVUprZ2UxblQ5OE9rTVZNa2ljZ24xR1RoSk9PbUF3?=
 =?utf-8?B?VmtFb2lFMVZqbjBWZ1hpd3lxai92dFVaSEdpbjZ1VGp3RjNLY0NxMXRIV0d3?=
 =?utf-8?B?enJKaHVBdjYrRXJIbFdyNUQvai9CbGlVc1FON1RtRzFXRlRuakJCNU9aRlRl?=
 =?utf-8?B?eXo1WVFJcU9maUF4RU4wT2xha0hzZkRmTCtEZ2lXVGlZeXMwQUMxSHNCUXlW?=
 =?utf-8?B?UWh3ZmFGMHdvT0JyRkhxUXFmMy9mZVdmcGFyYWoraUFIcjZrc01qMlF0QWJx?=
 =?utf-8?B?eTMyZ0l4VEJKamF1WXFWUnNYUnAwNHVVa0drd2M0d0REKyt6UERVZGVDeGxk?=
 =?utf-8?B?Zyt5eEtiR29NbEdTcjg2czBIVy9oQmk5Yjd6RGxENm9VSXZabFh2UEx3bTNy?=
 =?utf-8?B?cjR4WjhnUk0wcVk1N2JYejJFUU1JSkRBTDRUTXZmbUt2QVJDMFpuMEhhQnNN?=
 =?utf-8?B?RkJaSkdtczRhQXA1RkZ1R0pSMmhBMGFpZjRHemtWZXZyRmtNQkZ4UCs1Z3J0?=
 =?utf-8?B?aTFoR1VMdjFNVVU3d1dwSzNNSDdQcXAxV2tVdmszQlA2eThjSThDckY4aGJn?=
 =?utf-8?B?aFUzV1NERHFJamFJMXI2NEVCdW00YnpNK0M2V083NUU3RUVXNTcyRVlFSExT?=
 =?utf-8?B?SStaRTlES0FEQ0lGUElHR0wyMmpaaFFvVk1WTmpKaHNscXl0bHQxVmlLRngz?=
 =?utf-8?B?TDk0TXlXMUlaUkNORDg1L1U0N1NmQWkwVFRLbWpad0hhbUVkbXZsUmZPUTJp?=
 =?utf-8?B?UWYxeVIySWZRY0RNNWdvaE93NUMwTENjMlV1QU1wYk81K0FpOUZnaDBDbWd2?=
 =?utf-8?B?MzFWMmFTSURpbUVrY05qb25zb2g4SXo3RC8zakJWdkJ0cld3QlBJK0UwQ1c3?=
 =?utf-8?B?OVZLMUNSSEVYeVV5aHNCVzdmNlRTRHRBd2haNnA4Mk84VGRQZk1jS0pROFNl?=
 =?utf-8?B?dGM0QWlOQS9pSnhlUFdaNjhrMkJQRXFQWitZbVpHVjlOVXlzcWpiNkt3dEFE?=
 =?utf-8?B?NDFreGVNNDZDeEZRV1liQm45VTFKdGdBWStMVmg1R2lCd2NtSnFxK1AwZHBL?=
 =?utf-8?B?UzBHZEN2cVZYUzdjUWxkakt6Nm9Gc01iaTVGbXUwWEVSKzRZL2JMK0pxTmJm?=
 =?utf-8?B?NFVyQll2N0NLaGYwRnlFc002OUROSDh1RWNTNXVEOWI4bWlFWXZBMTg3UFFr?=
 =?utf-8?B?U25ybVYyb2NwTVFwdm5rTWJCTmlLeVB3VlNQZ2RQV3VQNG1KWHlMaURBMnZj?=
 =?utf-8?B?WG5zSnpsdnF1eE5maURvRzAvNjg1VHVpMFltN0NUdEVFYktabnZZMEZaNk9T?=
 =?utf-8?B?c2cxVjBHeisrL0VGNVNXeWVBak1lNmMrakIyRy9RaXB3R0RiNzNIRmZJT09j?=
 =?utf-8?B?MUVJVTV5Q1pwN1FoSHAyTmxaTzVqNlk4M2pmV0pDMUNxTStxVHVXMFBZTVpJ?=
 =?utf-8?B?NUtmTm85QmRGMmxOWncyekVxaC81NVZVdFBWVVVZS0dma09SUzVZRFZGS0Zq?=
 =?utf-8?B?dkRCWCtaSlpQTGhkekxtcjdWTHRYdXZycTQvQm5KVm9KT2FvRks4UEd2YVp1?=
 =?utf-8?B?d1hGNEM5cWl5U1BrczUrc1FjV052b1lOVjdBTFBvY2R4UXpET24zL0w4U1Ri?=
 =?utf-8?B?dzlkd0ErZ1gzZktnWk9ma1pLU0p6eGtuUWxJZnp1MHMrLzdxTHE3RFdsWHJ1?=
 =?utf-8?B?L3JoaVZ3M1B5Z2lhUG41SmxSWDJpb0lHM0dPV0ljNzkyWEZ4SldwUCtiRUhF?=
 =?utf-8?B?SzdGVE5vWmVXNHpvZXoySy93T01hVDVPaERBakJwUXVOTml1UkQ4Q0Q0N0Q2?=
 =?utf-8?B?bXRPY1cyUmJMMVBpMUhsamx3QXpydkJLblpKMUpoK0tsdnVuKzQwVE1sem4z?=
 =?utf-8?B?SDc5VUNlNmEvbEFxOHpzRDltcUkySlJPUUNXR2VxNWxPQXNnTXpGSzdtc081?=
 =?utf-8?B?UjVaTkhUMWpucEZkMk8va1ZNUklJOURzS1RzNXhKRlJmYnNtM3MzUFowbnVp?=
 =?utf-8?B?cGpPb0NEZmU0ZGl0WlBwelE5L21HNlFKTUZFTmlnL0t5S3hMYUtTYmk1Q3hT?=
 =?utf-8?B?ZExrNFFYYjlFT0tYQ0VwTVR1aVFIdk9ETFJValBUQUJ6dnVUa2lLc1NxSjNt?=
 =?utf-8?B?WU4vcFBRMWs0bjk1cHlteGRMMWgzZklOeGJsVlk4RGY5eW9IN0xIUT09?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 368b68f9-70a3-4d76-b8b8-08da19a2e17b
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Apr 2022 21:00:58.6143
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 2LAGk9lI+b67J9x6tKsQXURMkLlaJvqakA6B3wjkMKvAxEQeJXS7Zn0wygjeqgSDWr+K+ztr85p+KheWU3f3ew==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR1001MB2291
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.858
 definitions=2022-04-08_08:2022-04-08,2022-04-08 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 malwarescore=0 adultscore=0
 suspectscore=0 spamscore=0 mlxlogscore=999 bulkscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204080107
X-Proofpoint-ORIG-GUID: _NQv6va7OvOKUm0C8QLdwmKss3zZi2YT
X-Proofpoint-GUID: _NQv6va7OvOKUm0C8QLdwmKss3zZi2YT
X-Spam-Status: No, score=-5.6 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 4/8/22 9:31 AM, J. Bruce Fields wrote:
> On Wed, Apr 06, 2022 at 03:45:32PM -0700, Dai Ngo wrote:
>>   static void
>>   nfs4_get_client_reaplist(struct nfsd_net *nn, struct list_head *reaplist,
>>   				struct laundry_time *lt)
>>   {
>>   	struct list_head *pos, *next;
>>   	struct nfs4_client *clp;
>> +	bool cour;
>> +	struct list_head cslist;
>>   
>>   	INIT_LIST_HEAD(reaplist);
>> +	INIT_LIST_HEAD(&cslist);
>>   	spin_lock(&nn->client_lock);
>>   	list_for_each_safe(pos, next, &nn->client_lru) {
>>   		clp = list_entry(pos, struct nfs4_client, cl_lru);
>>   		if (!state_expired(lt, clp->cl_time))
>>   			break;
>> -		if (mark_client_expired_locked(clp))
>> +
>> +		if (!client_has_state(clp))
>> +			goto exp_client;
>> +
>> +		if (clp->cl_cs_client_state == NFSD4_CLIENT_EXPIRED)
>> +			goto exp_client;
>> +		cour = (clp->cl_cs_client_state == NFSD4_CLIENT_COURTESY);
>> +		if (cour && ktime_get_boottime_seconds() >=
>> +				(clp->cl_time + NFSD_COURTESY_CLIENT_TIMEOUT))
>> +			goto exp_client;
>> +		if (nfs4_anylock_blockers(clp)) {
>> +exp_client:
>> +			if (mark_client_expired_locked(clp))
>> +				continue;
>> +			list_add(&clp->cl_lru, reaplist);
>>   			continue;
>> -		list_add(&clp->cl_lru, reaplist);
>> +		}
>> +		if (!cour) {
>> +			spin_lock(&clp->cl_cs_lock);
>> +			clp->cl_cs_client_state = NFSD4_CLIENT_COURTESY;
>> +			spin_unlock(&clp->cl_cs_lock);
>> +			list_add(&clp->cl_cs_list, &cslist);
>> +		}
>>   	}
>>   	spin_unlock(&nn->client_lock);
>> +
>> +	while (!list_empty(&cslist)) {
>> +		clp = list_first_entry(&cslist, struct nfs4_client, cl_cs_list);
>> +		list_del_init(&clp->cl_cs_list);
>> +		spin_lock(&clp->cl_cs_lock);
>> +		/*
>> +		 * Client might have re-connected. Make sure it's
>> +		 * still in courtesy state before removing its record.
>> +		 */
> Good to be careful, but, then....
>
>> +		if (clp->cl_cs_client_state != NFSD4_CLIENT_COURTESY) {
>> +			spin_unlock(&clp->cl_cs_lock);
>> +			continue;
>> +		}
>> +		spin_unlock(&clp->cl_cs_lock);
> .... I'm not seeing what prevents a client from reconnecting right here,
> after we drop cl_cs_lock but before we call
> nfsd4_client_record_remove().

Thanks Bruce for pointing this out. I will fix it with a wait lock
in the next patch. This seems heavy but I have not been able to
fix it without the wait lock. This is should be a rare condition so
it would not cause much overhead.

>
>> +		nfsd4_client_record_remove(clp);
>> +	}
>>   }
>>   
>>   static time64_t
>> @@ -5794,6 +5876,13 @@ nfs4_laundromat(struct nfsd_net *nn)
>>   		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>>   		if (!state_expired(&lt, dp->dl_time))
>>   			break;
>> +		spin_lock(&clp->cl_cs_lock);
>> +		if (clp->cl_cs_client_state == NFSD4_CLIENT_COURTESY) {
>> +			clp->cl_cs_client_state = NFSD4_CLIENT_EXPIRED;
>> +			spin_unlock(&clp->cl_cs_lock);
> In an earlier patch you set the client to EXPIRED as soon as we got a
> lease break,

I don't understand, we only set EXPIRED when we want to expire the
courtesy client.

> so isn't this unnecessary?

When the client is checked to see if it it can be in COURTESY_CLIENT
state, we did not check if there is any delegation recall callback is
pending, we only check for lock blockers) so that is why we check it
here and expire it.
Maybe I did not give the right answer since I'm not clear of the
question yet.

>
> I guess there could be a case like:
>
> 	1. lease break comes in
> 	2. client fails to renew, transitions to courtesy
> 	3. delegation callback times out
>
> though it'd seem better to catch that at step 2 if we can.

I will wait for the above issue to be resolved first to better
understand your concern here.

Thanks,
-Dai

> --b.
>
>> +			continue;
>> +		}
>> +		spin_unlock(&clp->cl_cs_lock);
>>   		WARN_ON(!unhash_delegation_locked(dp));
>>   		list_add(&dp->dl_recall_lru, &reaplist);
>>   	}
>> -- 
>> 2.9.5