Message-ID: <5fa49a09-50c9-efb4-fa72-35c0e8d889b1@oracle.com>
Date:   Fri, 17 Dec 2021 12:50:55 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.4.0
Subject: Re: [PATCH RFC v8 1/2] fs/lock: add new callback, lm_expire_lock, to
 lock_manager_operations
Content-Language: en-US
To:     Bruce Fields <bfields@fieldses.org>,
        Chuck Lever III <chuck.lever@oracle.com>
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Jeff Layton <jlayton@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
References: <20211213172423.49021-1-dai.ngo@oracle.com>
 <20211213172423.49021-2-dai.ngo@oracle.com>
 <0C2E5E30-86A3-489E-9366-DC4FF109DD93@oracle.com>
 <20211217203517.GJ28098@fieldses.org>
From:   dai.ngo@oracle.com
In-Reply-To: <20211217203517.GJ28098@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?Z2o3VXdsenpxekxZckphQUNHMEdkbW92WWlSSktRV01YTHd6WDkzTVhDbU1E?=
 =?utf-8?B?N0tkNnh5OGVDR01ndGEybHRGZ2VhcW9Ja0Y5VUpwOWVGSDBIU1plNi9tdXQy?=
 =?utf-8?B?Y2hhUXZXM3p3QU5NdXZSKzNCR2JZNWxDU0lkUkZaN2tuYU1rTVdzbXNtOHYx?=
 =?utf-8?B?UkdnQThCbTlmTU11bHpaSTdJcGRzUVJqTHZiWHNIU3FvVXlrVTZzRjVycHFv?=
 =?utf-8?B?eENDL2txSHFZZHVCNHZaSHVzL3Vzb0JoT1AzUEtKODRWQWhlOTN2c3phUm9O?=
 =?utf-8?B?VnVXdjZwM2dWWHpNMHZyNnhibEE0dER0Q0duR3VmNVZTd09KTWlDUE1GMURT?=
 =?utf-8?B?Q1pzc1AzK3RXMldqVG9BL0tHWkJIc052YjhPaVRBQ3JqU1hoZWNWTWRvd1hW?=
 =?utf-8?B?MjNYamdPN2lJcGU0WlRNRHBXQmg0elFJeFZteWlldkExR1VwNE5zWXNJWkFi?=
 =?utf-8?B?R3pJSEJRL3lqTk5nMjY1cXEzSTRGWGxGczduUjBVSWdPV0F3Y2Nldk1VbmZs?=
 =?utf-8?B?NWxOYnNGNzdwVnVxbk9sNTN0V3VETkZvUm50aC81TUZscWJHanZSdHVuU3RV?=
 =?utf-8?B?RkRJUDhkR1R5ZVNVWWdwVFVPL0ZhMUFpb0p4RWVJYlhWdFBlbDhnRDlqNzhN?=
 =?utf-8?B?Mzg2WEhWU1lIcDBjOUlmSE81QkQ5aS9LMWltWlB3UE4rNXl3RFM5RlVXbG96?=
 =?utf-8?B?eW9VUWlJZFhvNVBZaEJHbHc5TktwNXJGVDBTQzFzUTByTVhsQVN0TkdpT1Rq?=
 =?utf-8?B?bS91bW1IZXZoV2ZOblVWeEdHSWZoWit3cmprVVJORWREUUlpVEwrRGx1TXEv?=
 =?utf-8?B?ZFRwcUE5R0xLci9xb0IzY20rMjQ1UHlDWlM4Y0J3bzJEaVBHSUhNcHlBbGpZ?=
 =?utf-8?B?ZzlmTElFOFJxVFVxdEorYzFiWlh2TnhLRUYwZG5yK3RYNWVtUXVtbTMzUnRz?=
 =?utf-8?B?cTBPaUlJSHRDdTBXSWRYSWRXZ2RDenRUaG1XalFtNzNPSHVwVGlENSt4VzFI?=
 =?utf-8?B?TVpNUDc1L09RWjdLUTRWTjk2R2M1aitlSG1JWDQwaTNvSFEvL2RqSnA4YzNN?=
 =?utf-8?B?NmdsR2lWYU5FUlBiU1kyaktvVmFNZWdxNGNNa2d3UVFCQnNxelI3RjNOb2hz?=
 =?utf-8?B?TWZiV1NhK0poNkVlY3NsZkZ0UHdPa1hCT2F6MzFmempScFc3cXpkdDRsaGk3?=
 =?utf-8?B?cTBZa0swYmltNEJUZnJhMTg1dytUVlM4eXFoSVdPLzU0ZE1KalQzK25KYkhN?=
 =?utf-8?B?b2tMaG5xV2JMMXNJcnk2WU5wSUl3Tk5vRW5ZRHlkOGdHaFdsVDRwd1JQQXhT?=
 =?utf-8?B?ZS83c3lrRmY0TUxLZGxxeXdkMUtzbUJIZXkraVZ3eDhPWjdlSjArMmowT3lm?=
 =?utf-8?B?dU1QTTAwaFpaY3RXS1Fyak9FaGVFYWpPZU5WQmV4b3dXVitaRHJlSmhFTzc4?=
 =?utf-8?B?U2F4bDdPT0tWS3RJcEkySlUzMXh0RWs1L0FJUTlYQnplZnJKd0Z2K2F4cXdp?=
 =?utf-8?B?OHY0Mm5OWHlobklqUGE5T21FV0ZJY1ErRHEvSER1VzBnTnFCZ0V5VitpZ2Ix?=
 =?utf-8?B?YUtUcWtIWkMrQ25oTzhaNWZQV3BXb1JLQWNWUkRsa3FFNjBzcEg2bjUrU3Uz?=
 =?utf-8?B?Unc4SWNQT2ZIb3U0VGZRMmRJVDRpeHVWekZFZEpXUEhJbndKOTVibzMwSjZ5?=
 =?utf-8?B?QWlFY2F4RjJ1clg3dTVZQUtLTlE3WFJrUUJyRXJQVGxZVGpWYUtyQzZYNWxq?=
 =?utf-8?B?UTIzZ01kNDBUeUJKb1FhdXhQTWhDUG9YR3VWa0h2NDhhdkEwQ21IdWFwVTRx?=
 =?utf-8?B?T1lnaGdsNFNRT1hsVEhuMnpMYU9vcXJnTG1tbEgrLzBYTmFFRC9oQm9VZ0th?=
 =?utf-8?B?SzhUZkhORkxpOFBEampMczVsOCtaYUFDWDBJdHBSNVgzcFJhSXl2VHkxbUhm?=
 =?utf-8?B?azlTTHVOenN1MkdlYnZxWFY2S3k4ODNLR0J1M0FQNy84MUptTnFTUmx3ODRS?=
 =?utf-8?B?N3dMT0htMnl1Q2xVOFUrdGZwQkdJYXNnTGs1QWdYRXcyZWw4VFNTbURrSFJZ?=
 =?utf-8?B?RzMvWmJYWlJpS3Z0a1RmUE1XbVZYL2kzWnBrblB2SnFqTGc1NnNsRnRGZ0l5?=
 =?utf-8?B?S2V5bEx3eDEzbjVsczJDbExib2tNSDRQWUZnczRNUmJYQ3VEMGxCTVdvOXF6?=
 =?utf-8?Q?FXDYgRLfC6pM7kteIYxajlY=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d7b2161a-3a09-4d35-9394-08d9c19eedc1
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Dec 2021 20:50:58.8921
 (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: 5tE1iiOa1KG2gZItMUykPHCM0ryX87HoM130D3/XDeL3nBRxuVMX0dFqzyc9rW+1UouoZHI5exUCUKdmH83/7Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4464
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10201 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxscore=0 bulkscore=0 spamscore=0
 mlxlogscore=999 suspectscore=0 adultscore=0 malwarescore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112170116
X-Proofpoint-GUID: UA2Z8yGBPHXaU1CnP5O4uG0lcqhZjfuM
X-Proofpoint-ORIG-GUID: UA2Z8yGBPHXaU1CnP5O4uG0lcqhZjfuM
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


On 12/17/21 12:35 PM, Bruce Fields wrote:
> On Tue, Dec 14, 2021 at 11:41:41PM +0000, Chuck Lever III wrote:
>>
>>> On Dec 13, 2021, at 12:24 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>
>>> Add new callback, lm_expire_lock, to lock_manager_operations to allow
>>> the lock manager to take appropriate action to resolve the lock conflict
>>> if possible. The callback takes 2 arguments, file_lock of the blocker
>>> and a testonly flag:
>>>
>>> testonly = 1  check and return lock manager's private data if lock conflict
>>>               can be resolved else return NULL.
>>> testonly = 0  resolve the conflict if possible, return true if conflict
>>>               was resolved esle return false.
>>>
>>> Lock manager, such as NFSv4 courteous server, uses this callback to
>>> resolve conflict by destroying lock owner, or the NFSv4 courtesy client
>>> (client that has expired but allowed to maintains its states) that owns
>>> the lock.
>>>
>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>> ---
>>> fs/locks.c         | 40 +++++++++++++++++++++++++++++++++++++---
>>> include/linux/fs.h |  1 +
>>> 2 files changed, 38 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/locks.c b/fs/locks.c
>>> index 3d6fb4ae847b..5f3ea40ce2aa 100644
>>> --- a/fs/locks.c
>>> +++ b/fs/locks.c
>>> @@ -952,8 +952,11 @@ void
>>> posix_test_lock(struct file *filp, struct file_lock *fl)
>>> {
>>> 	struct file_lock *cfl;
>>> +	struct file_lock *checked_cfl = NULL;
>>> 	struct file_lock_context *ctx;
>>> 	struct inode *inode = locks_inode(filp);
>>> +	void *res_data;
>>> +	void *(*func)(void *priv, bool testonly);
>>>
>>> 	ctx = smp_load_acquire(&inode->i_flctx);
>>> 	if (!ctx || list_empty_careful(&ctx->flc_posix)) {
>>> @@ -962,11 +965,24 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>> 	}
>>>
>>> 	spin_lock(&ctx->flc_lock);
>>> +retry:
>>> 	list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
>>> -		if (posix_locks_conflict(fl, cfl)) {
>>> -			locks_copy_conflock(fl, cfl);
>>> -			goto out;
>>> +		if (!posix_locks_conflict(fl, cfl))
>>> +			continue;
>>> +		if (checked_cfl != cfl && cfl->fl_lmops &&
>>> +				cfl->fl_lmops->lm_expire_lock) {
>>> +			res_data = cfl->fl_lmops->lm_expire_lock(cfl, true);
>>> +			if (res_data) {
>>> +				func = cfl->fl_lmops->lm_expire_lock;
>>> +				spin_unlock(&ctx->flc_lock);
>>> +				func(res_data, false);
>>> +				spin_lock(&ctx->flc_lock);
>>> +				checked_cfl = cfl;
>>> +				goto retry;
>>> +			}
>>> 		}
>> Dai and I discussed this offline. Depending on a pointer to represent
>> exactly the same struct file_lock across a dropped spinlock is racy.
> Yes.  There's also no need for that (checked_cfl != cfl) check, though.
> By the time func() returns, that lock should be gone from the list
> anyway.

func() eventually calls expire_client. But we do not know if expire_client
succeeds. One simple way to know if the conflict client was successfully
expired is to check the list again. If the client was successfully expired
then its locks were removed from the list. Otherwise we get the same 'cfl'
from the list again on the next get.

-Dai

>
> It's a little inefficient to have to restart the list every time--but
> that theoretical n^2 behavior won't matter much compared to the time
> spent waiting for clients to expire.  And this approach has the benefit
> of being simple.
>
> --b.
>
>> Dai plans to investigate other mechanisms to perform this check
>> reliably.
>>
>>
>>> +		locks_copy_conflock(fl, cfl);
>>> +		goto out;
>>> 	}
>>> 	fl->fl_type = F_UNLCK;
>>> out:
>>> @@ -1136,10 +1152,13 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>>> 	struct file_lock *new_fl2 = NULL;
>>> 	struct file_lock *left = NULL;
>>> 	struct file_lock *right = NULL;
>>> +	struct file_lock *checked_fl = NULL;
>>> 	struct file_lock_context *ctx;
>>> 	int error;
>>> 	bool added = false;
>>> 	LIST_HEAD(dispose);
>>> +	void *res_data;
>>> +	void *(*func)(void *priv, bool testonly);
>>>
>>> 	ctx = locks_get_lock_context(inode, request->fl_type);
>>> 	if (!ctx)
>>> @@ -1166,9 +1185,24 @@ static int posix_lock_inode(struct inode *inode, struct file_lock *request,
>>> 	 * blocker's list of waiters and the global blocked_hash.
>>> 	 */
>>> 	if (request->fl_type != F_UNLCK) {
>>> +retry:
>>> 		list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
>>> 			if (!posix_locks_conflict(request, fl))
>>> 				continue;
>>> +			if (checked_fl != fl && fl->fl_lmops &&
>>> +					fl->fl_lmops->lm_expire_lock) {
>>> +				res_data = fl->fl_lmops->lm_expire_lock(fl, true);
>>> +				if (res_data) {
>>> +					func = fl->fl_lmops->lm_expire_lock;
>>> +					spin_unlock(&ctx->flc_lock);
>>> +					percpu_up_read(&file_rwsem);
>>> +					func(res_data, false);
>>> +					percpu_down_read(&file_rwsem);
>>> +					spin_lock(&ctx->flc_lock);
>>> +					checked_fl = fl;
>>> +					goto retry;
>>> +				}
>>> +			}
>>> 			if (conflock)
>>> 				locks_copy_conflock(conflock, fl);
>>> 			error = -EAGAIN;
>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>> index e7a633353fd2..8cb910c3a394 100644
>>> --- a/include/linux/fs.h
>>> +++ b/include/linux/fs.h
>>> @@ -1071,6 +1071,7 @@ struct lock_manager_operations {
>>> 	int (*lm_change)(struct file_lock *, int, struct list_head *);
>>> 	void (*lm_setup)(struct file_lock *, void **);
>>> 	bool (*lm_breaker_owns_lease)(struct file_lock *);
>>> +	void *(*lm_expire_lock)(void *priv, bool testonly);
>>> };
>>>
>>> struct lock_manager {
>>> -- 
>>> 2.9.5
>>>
>> --
>> Chuck Lever
>>
>>