Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <f4c9b6fa-ef75-55b0-53dc-4830d6aef94e@oracle.com>
Date:   Thu, 13 Jan 2022 11:51:43 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.5.0
Subject: Re: [PATCH RFC v9 2/2] nfsd: Initial implementation of NFSv4
 Courteous Server
Content-Language: en-US
To:     "J. Bruce Fields" <bfields@fieldses.org>
Cc:     chuck.lever@oracle.com, jlayton@redhat.com,
        viro@zeniv.linux.org.uk, linux-nfs@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
References: <1641840653-23059-1-git-send-email-dai.ngo@oracle.com>
 <1641840653-23059-3-git-send-email-dai.ngo@oracle.com>
 <20220112194054.GD10518@fieldses.org>
 <11e9d7a9-f2f3-47a9-c76f-dc2b9010d303@oracle.com>
 <20220113154206.GA32679@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20220113154206.GA32679@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?UUV3dTNCVEhkbm43Z0VsWGRjMTFNMEx3c1ZYYkRib0YzTVlEM1Zjb0FRNUR4?=
 =?utf-8?B?Mk04bjNOWHROQzMzaU9RUHZJaCs1QitwYkdBQ0U5cFVPV01zUHdSWTlmMExy?=
 =?utf-8?B?TVE5K25HR2tiaTNLbElXZ2dJZlNsNjJnZEV0Y0VCTkwyajFTMDdKWHB3aHZj?=
 =?utf-8?B?THREQWVFZkkveGgxMDY1Uldrbnl5aEk0YURUVXRsUkpEY1ZxdWtHdzQ5VFFS?=
 =?utf-8?B?VVZUSjcrUG1yV21qQVk4bEpwbitvZ3FzVTl3NDFJSVkwVFg3OCs3R1N1Q1dQ?=
 =?utf-8?B?RStUa2h6bGRSS1hBaHFWOVNnbjYrd1VaWUhvZy8rQmR6aE5zSEV2M3U2cUV1?=
 =?utf-8?B?TjIvdFJJYVYwaUF0b3RCbGgzZHB3QUVGWmN2Q2ZPMDdqemJHbDc2bjZSejZK?=
 =?utf-8?B?M2ZlQjJJV040RCsxRDM5QnJMYjNNcjFrNmdLMnBVTGMxTjVMNHRWeFJUaTcv?=
 =?utf-8?B?V25xT1YvVC9RK092a0V3bkJTS0ZQSkhDZWlKUmdVVVNIY252TzBUYUxpSlVZ?=
 =?utf-8?B?MDhsYktkRncrdGFwL2x3dnpkQ3R5THJOa0dJSW8rVWxCL0dENVVrdHAzaWRO?=
 =?utf-8?B?U01QMHJMSi9qUW5HenVEV2xYWTMrUDV4M1N5ejhic0RaTjR6VHF1UFg0eHQ4?=
 =?utf-8?B?RTZuOFJXRHlNOEwrZjV6ZkQ5VzduRHFKRjd5TDdUZk5mcEVXOVRDL01TSWY4?=
 =?utf-8?B?TEcwZzNpdGswUm9HNWtpSytkanE4MnZXOXJyYlFnRDhXcmQ4Rm1iT1lrSmJj?=
 =?utf-8?B?cTY3eTQrVy9zbTFYaFFjNXdaREhKeGo5OGZqNGljVUlCU01hajdKYjlBaXM1?=
 =?utf-8?B?dkhjSnhuYVNQaVpyYnJ3SjZUSUhqUjRwUVh4WVh4MFdGR1lZTDV3UDNVNUNt?=
 =?utf-8?B?YTZKQ2ptem5YM1YwMzZGWWUzemZRY2JpVDhGZllMakF1Y3NIU1hWNzFlMEhk?=
 =?utf-8?B?SEgzR09rWU81NkVsT2xWbDRnQmZwSFh6VGEzd0plRjRFNEpuUXpUdGhCbkJa?=
 =?utf-8?B?MmJNdndFbXptVko0enZqUHhNWHA3RS84NnVXbFpGMkRHaGtLcWFnRHVqa2Vr?=
 =?utf-8?B?V3RGSUlmZlNjbTRoV0owcGdzUHJZN01RRUZQZVA0MjdBbWV3WGxia3dKK0Zx?=
 =?utf-8?B?bDJ0WEY4bkhMbEdXY1RMbTNKWWFpS3V6SEJvcWJpYm5aOTYrdzlVL0R2OXBF?=
 =?utf-8?B?Nm1vYUJnNCtMQlYrOTVFU3daaHpZYytYelBWZ0Znb1BpNWhpZEVqbCtDU1F0?=
 =?utf-8?B?Y0xjQkhMVm5JMDFhWXFFT2dlWndERVc5Y2k4R1BuTm5jY0VYWTFRVDQ5RHpa?=
 =?utf-8?B?UUk0Z1ZwQ3Z4YmhYQTB5M1pBazZYenRsbHZDbUY2RlcySzhFcUZSMzBFTlla?=
 =?utf-8?B?Qmc3dGQ2aHdlWG1MNFlpTXAyQmhaeEpUM3Z0M0hDTWtBTzgvODhsd2pGQ3o1?=
 =?utf-8?B?a0l0SjVRb1Q1MjRSaldqd3FIZ0JrOVdIbit5cHprdEJ6RGhSdXBoYmhXWHEz?=
 =?utf-8?B?VFJlTTdHTWhVdkkzU1RPTVBSYXN2cFpScWt2aW9DYXNCejg0QzMrN1Fodm14?=
 =?utf-8?B?VjNmc0xDRUFFUWp4bU1XdkpheGRBUHNuVHdrSWhIY0JOUzBHd3ZFeE1jQmRr?=
 =?utf-8?B?OGVyajZnU3Z3ZlYrMmJjbkI4WE4rUU5ZVzdCSjN3akhhR0FlQWN0dlduNnRz?=
 =?utf-8?B?aVE2OEpidlYxaEE5YUlnSXUzc3lTV3F4eWc1azM3b0JTajJiLzNVSzhmNUVE?=
 =?utf-8?B?VGVDektQRTZqeTBnVEp0SzdWczRkc2twVE1ZbjE5QlU4N29EUjhXR3JvQjFs?=
 =?utf-8?B?eVZJS2pmaEoxalVQWU5idzhybm04aW5QeHA5cXlWV1c4WEFQTmpKT2E0Qmp1?=
 =?utf-8?B?aG42Ui9hYnAzYVBJUEoxV0pJZDFMSk5HT3kxQjM2Q3VtL2FiRTIwWU9tc3Vx?=
 =?utf-8?B?RzlkNHFCOGtwVnlHbnFsMk5jUTlWZm9JM0pBVHZQVjVxY0M1QUx6MW9OYkFh?=
 =?utf-8?B?d0tSdWY0bGtyRHlnUXVNUXhxaEZzdHN3NmowVXdpNmF5UGZiaWlmV2xlTm1u?=
 =?utf-8?B?TlNpaUVIaVVVR1orcEFEalZKNlA0c2lKOWpLdExhRFNzWS9CS2dpSURKOG5Y?=
 =?utf-8?B?YmFZWklOVW1lcDNzeXBNcmlLNW96Vzkyb2pqWUMvWndHN1pJU2F5RC8zRzl4?=
 =?utf-8?Q?CWk3BmDDj4vh69I199lMB4w=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 09487300-d674-4ddb-4c18-08d9d6ce20bd
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jan 2022 19:51:45.2862
 (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: QzRTyrRUDv8bRsRDA0GzuT2JZhpL32P1bdUuzb9BM/W94gowA9zyvn2x8XRaX+QSV7PXQ1X7UKQ4+t5s3snDFA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB2872
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10226 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 mlxscore=0 adultscore=0
 suspectscore=0 mlxlogscore=999 bulkscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2201130124
X-Proofpoint-ORIG-GUID: h38XQxICEhO87J_pdNFahKEVgvlgSDyg
X-Proofpoint-GUID: h38XQxICEhO87J_pdNFahKEVgvlgSDyg
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 1/13/22 7:42 AM, J. Bruce Fields wrote:
> On Thu, Jan 13, 2022 at 12:51:57AM -0800, dai.ngo@oracle.com wrote:
>> On 1/12/22 11:40 AM, J. Bruce Fields wrote:
>>> On Mon, Jan 10, 2022 at 10:50:53AM -0800, Dai Ngo wrote:
>>>> +		}
>>>> +		if (!state_expired(&lt, clp->cl_time)) {
>>>> +			spin_unlock(&clp->cl_cs_lock);
>>>>   			break;
>>>> +		}
>>>> +		id = 0;
>>>> +		spin_lock(&clp->cl_lock);
>>>> +		stid = idr_get_next(&clp->cl_stateids, &id);
>>>> +		if (stid && !nfs4_anylock_conflict(clp)) {
>>>> +			/* client still has states */
>>> I'm a little confused by that comment.  I think what you just checked is
>>> that the client has some state, *and* nobody is waiting for one of its
>>> locks.  For me, that comment just conufses things.
>> will remove.
>>
>>>> +			spin_unlock(&clp->cl_lock);
>>> Is nn->client_lock enough to guarantee that the condition you just
>>> checked still holds?  (Honest question, I'm not sure.)
>> nfs4_anylock_conflict_locked scans cl_ownerstr_hashtbl which is protected
>> by the cl_lock.
> That doesn't answer the question.  Which, I confess, was muddled (I
> should have said "clp->cl_cs_lock", not "nn->client_lock".)
>
> Let me try it a different way.  You just checked that the client has
> some state, and that nobody is waiting for one of its locks.
>
> After you drop the cl_lock, how do you know that both of those things
> are still true?

After we drop the lock, if the client now has no state then it just
remains in memory until the courtesy client timeout expires then we
get rid of it.

For the race condition of lock conflict, we use the client->cl_cs_lock
to synchronize the laundromat and and lm_lock_conflict/nfsd4_fl_lock_conflict.
If the locking thread acquires the cl_cs_lock before the laundromat does
then the thread will be blocked and laundromat detects there is blocker
and expires the client. If the laundromat acquires the cl_cs_lock first
then NFSD4_COURTESY_CLIENT is set and nfsd4_fl_lock_conflict detects
this flag and sets the client to NFSD4_DESTROY_COURTESY_CLIENT.

-Dai