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: <f3e41200-13b6-837c-3eef-0fe60b64f693@oracle.com>
Date:   Tue, 15 Mar 2022 11:56:38 -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 v16 03/11] NFSD: Add lm_lock_expired call out
Content-Language: en-US
To:     Chuck Lever III <chuck.lever@oracle.com>,
        Bruce Fields <bfields@fieldses.org>
Cc:     Jeff Layton <jlayton@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
References: <1647051215-2873-1-git-send-email-dai.ngo@oracle.com>
 <1647051215-2873-4-git-send-email-dai.ngo@oracle.com>
 <20220315150245.GA19168@fieldses.org>
 <1e1ff6a1-86cf-99d4-13ad-45352e58fe73@oracle.com>
 <483E025E-E72E-4F2C-BF98-66DE12F94909@oracle.com>
From:   dai.ngo@oracle.com
In-Reply-To: <483E025E-E72E-4F2C-BF98-66DE12F94909@oracle.com>
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?VTJYL0J1Q0duZXJhdXNjQXlFOUVObUJMUDFIMm9SZGhiUUppQkNhazZ0UUdR?=
 =?utf-8?B?VHNReHJQNFN6Rk9ZeVJGb0RTcVE5R3YxcXAwcGFiTVNWdEZKcWZZWkFqVTV3?=
 =?utf-8?B?Q2pqRk9NYlhnUE1VcWhPYkF1TkxoVXZNVlcwdTlwYnpLWjhmU3B3M1EzSUox?=
 =?utf-8?B?V0ZWNlNpTmJTZzJDVUh6blFFYWhicU44VUlRSmR5YU1peWYzMUhlckxDNFV2?=
 =?utf-8?B?aDlvZUN6TFNrQlhNa21KRU1mOFR4MHdxT3didXYydC9NaUNqQ0x6KzV2a3hO?=
 =?utf-8?B?YzFrNWdPWmZLYXVnN2FWVUl6SmNIRWVwV2FNM1NhUW4rZ2ZDWDBaSno4Vi9I?=
 =?utf-8?B?RXpJS0YreTBwdzk0OGFQeENPV1dtMHZnVmh3ai9xczNyODV5WG9JVFd6dllC?=
 =?utf-8?B?VXZyYnZXQnk2azI0eXZYalpkUVE0TjdzTlhpbHpQeDVyVUNvaHI4R1hSZnpx?=
 =?utf-8?B?WDJGaGFOTTFUWng4ZXdCS05IZnVvR2I3dlVoQUFWTHFFWFY2STFsYldkY0Z5?=
 =?utf-8?B?TUpJYy9wdFc5WEYyZk5FUDJLKzZod01EZTkzOCtrZWhyem5iSmY2NFRKTXBl?=
 =?utf-8?B?bnVFbUVoeUcyQUdRblg0Z0ZhQXJPbVBrSTczQUVsUTdQdzE5WVpaRVFHLzBq?=
 =?utf-8?B?SFMwZFBvOHRXeHNtTjVVZTJ4WW5EdkRMdkcyeGIzeUc4SkVRblp5N2R2TjBy?=
 =?utf-8?B?dlowWmwyL3dzSlA1U3NqWVVQM0RCeU9jTXkxb2dkV1hiZEhZWVNjZjd6bHBT?=
 =?utf-8?B?ZFRLbnBtdUp1RmcvaDNhUi9FZmo3WnhuSk1kZ0dlQ3B4OFBRNmRrS0dFZnhG?=
 =?utf-8?B?NG40VzhnSEQyL28yZ0VrVk5kdUFBQXhQWDZjNndFQ0JxUkZ2Rm1nM0lOMFho?=
 =?utf-8?B?QkRraEZqdmVBNm15S3JFUzRZVHh2MHZNdXg5OHBTcklyd3UyTFFRMk5sL2tz?=
 =?utf-8?B?U2dGQmFaZDV3bjNzYkMxeUxFVitvRHRlZC9weTJaMW8wOW1YeVVqRXl3Vmpa?=
 =?utf-8?B?V2dMZ1lTRW5nb3JNMFpuVHpKU3dzbVFvaHlHQ0I5dlFERWtldVQ1MDU2b3Fk?=
 =?utf-8?B?SHVYak91c0dOVHVZbEhZNkhHNnlsYndBckJKRjNxRjN0MklhUm80c3lHVUlM?=
 =?utf-8?B?VzNRb2FGV3d1UlZLeFJaMFJ5VU5lZjNsUjMvazVzSHY2aWhydDhsbWVTU1VV?=
 =?utf-8?B?Vk11eENxTHFjVkxpYUV2RkxpUXpWbHRrQklITVhSRGozN24rVTVsNVV6bUMz?=
 =?utf-8?B?b2hUSFFuWlRQcitNKzNNdXl2SitsT0xLUXpBMlZEQUJzbkYrOTl0YjJOc0RM?=
 =?utf-8?B?aFNuVXRQNGVSSlQ1dXREWVlxMEFHQ2docEUzc213MnV6TktJeml4OTVwaVpV?=
 =?utf-8?B?Y3V1T0tnRzVrd0QzVE44NE5KR0k0dkROeTVTaTA5dUg3a3h3L1FCWktEMHFL?=
 =?utf-8?B?aTRody93M1c2L2daUVhwbUpCdHlTZHJBZHlTOHZaSkx3UnNrNUVvcEFEUlJU?=
 =?utf-8?B?cHEycnI1VGRaOFN3d2dvTkpuUWcrNWsrTU5qamcxMEt3VE1lK0RZMjhXUjc5?=
 =?utf-8?B?UHFtb2xIWkNuUmlGNkViSU9rNkh3cUJMZ3pIZElkZUc1RXNiaEpzSDE2eVpr?=
 =?utf-8?B?L2Q1NXBzcjhSK0diNnV0M09GbDB1SXNsN2R2dlZlQWw3Q1FoQ21ETlRTOElO?=
 =?utf-8?B?aHNUQ2p5cTFHYU14NXpWNEZQbjNTSi9wbU1kRXFOMU4rUVVza0pncFg5VVls?=
 =?utf-8?B?cjR1YmFxQjljZWs4bUNBUDFyVkpFZ2NvR2d2ZHEzMWYxcFN1bU9RbGg4MnI5?=
 =?utf-8?B?dlpOTzdoWC9pQi91WkVEUDVTZFVBUVpIeUhZbllocXhZVTNrNm1rd2VyVmNH?=
 =?utf-8?B?QUdWODVGaVhVaWxYTGpFM2FWbytwUXM4TGdJRkNycWk3OE04NzBxMFpodjQ1?=
 =?utf-8?Q?Rl9TmpKlD1U6aWbsbe+7aB5ShOkUDUf9?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 47343b96-c09b-4b57-c29b-08da06b58a43
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Mar 2022 18:56:40.6764
 (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: VkWPCfpa9j/5o6/f/ZOrbDMjYNKSmEfDWKMoMDJGZeguqE/29aSKTck7DOJ0CoGIZUmF1uVsheV96sk9QRj1og==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR10MB3139
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10287 signatures=693139
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 spamscore=0
 malwarescore=0 adultscore=0 bulkscore=0 mlxscore=0 mlxlogscore=999
 phishscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2203150111
X-Proofpoint-GUID: c5XWzNYbn6fXuXCHquaAUxHVOEZ_45SJ
X-Proofpoint-ORIG-GUID: c5XWzNYbn6fXuXCHquaAUxHVOEZ_45SJ
X-Spam-Status: No, score=-2.8 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 3/15/22 10:39 AM, Chuck Lever III wrote:
>
>> On Mar 15, 2022, at 12:26 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>>
>> On 3/15/22 8:02 AM, J. Bruce Fields wrote:
>>> On Fri, Mar 11, 2022 at 06:13:27PM -0800, Dai Ngo wrote:
>>>> Add callout function nfsd4_lm_lock_expired for lm_lock_expired.
>>>> If lock request has conflict with courtesy client then expire the
>>>> courtesy client and return no conflict to caller.
>>>>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>>   fs/nfsd/nfs4state.c | 37 +++++++++++++++++++++++++++++++++++++
>>>>   1 file changed, 37 insertions(+)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index a65d59510681..583ac807e98d 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -6578,10 +6578,47 @@ nfsd4_lm_notify(struct file_lock *fl)
>>>>   	}
>>>>   }
>>>>   +/**
>>>> + * nfsd4_lm_lock_expired - check if lock conflict can be resolved.
>>>> + *
>>>> + * @fl: pointer to file_lock with a potential conflict
>>>> + * Return values:
>>>> + *   %false: real conflict, lock conflict can not be resolved.
>>>> + *   %true: no conflict, lock conflict was resolved.
>>>> + *
>>>> + * Note that this function is called while the flc_lock is held.
>>>> + */
>>>> +static bool
>>>> +nfsd4_lm_lock_expired(struct file_lock *fl)
>>>> +{
>>>> +	struct nfs4_lockowner *lo;
>>>> +	struct nfs4_client *clp;
>>>> +	bool rc = false;
>>>> +
>>>> +	if (!fl)
>>>> +		return false;
>>>> +	lo = (struct nfs4_lockowner *)fl->fl_owner;
>>>> +	clp = lo->lo_owner.so_client;
>>>> +
>>>> +	/* need to sync with courtesy client trying to reconnect */
>>>> +	spin_lock(&clp->cl_cs_lock);
>>>> +	if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags))
>>>> +		rc = true;
>>>> +	else {
>>>> +		if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags)) {
>>>> +			set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags);
>>>> +			rc =  true;
>>>> +		}
>>>> +	}
>>> I'd prefer:
>>>
>>> 	if (test_bit(NFSD4_CLIENT_COURTESY, &clp->cl_flags))
>>> 		set_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags);
>> we also need to set rc to true here.
>>
>>> 	if (test_bit(NFSD4_CLIENT_EXPIRED, &clp->cl_flags))
>>> 		rc = true;
>> With v16 we need to check for NFSD4_CLIENT_EXPIRED first then
>> NFSD4_CLIENT_COURTESY because both flags can be set. In the
>> next patch version, we will clear NFSD4_CLIENT_COURTESY when
>> setting NFSD4_CLIENT_EXPIRED so the order of check does not
>> matter.
>>
>>> Same result, but more compact and straightforward, I think.
>> Chuck wants to replace the bits used for courtesy client in
>> cl_flags with a  separate u8 field so it does not have to use
>> bit operation to set/test.
> Code audit suggested there are really only four unique
> combinations of the bit flags that are used.
>
> Plus, taking a spin_lock and using bitops seems like overkill.
>
> The rules for transitioning between the courtesy states are
> straightforward, but need to be done in a critical section.
> So I suggested storing the courtesy state in a lock-protected
> unsigned int instead of using bit flags.

I will try what Chuck suggested.

>
> If we hate it, we can go back to bit flags.

ok.

-Dai


>>>> +	spin_unlock(&clp->cl_cs_lock);
>>>> +	return rc;
>>>> +}
>>>> +
>>>>   static const struct lock_manager_operations nfsd_posix_mng_ops  = {
>>>>   	.lm_notify = nfsd4_lm_notify,
>>>>   	.lm_get_owner = nfsd4_lm_get_owner,
>>>>   	.lm_put_owner = nfsd4_lm_put_owner,
>>>> +	.lm_lock_expired = nfsd4_lm_lock_expired,
>>>>   };
>>>>     static inline void
>>>> -- 
>>>> 2.9.5
> --
> Chuck Lever
>
>
>