Message-ID: <ba2acdf8-f9b9-8348-86a2-7772235af5a6@oracle.com>
Date:   Mon, 6 Dec 2021 15:07:02 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.3.2
Subject: Re: [PATCH RFC v6 1/2] fs/lock: add new callback, lm_expire_lock, to
 lock_manager_operations
Content-Language: en-US
To:     Trond Myklebust <trondmy@hammerspace.com>,
        "bfields@fieldses.org" <bfields@fieldses.org>
Cc:     "jlayton@redhat.com" <jlayton@redhat.com>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>
References: <20211206175942.47326-1-dai.ngo@oracle.com>
 <20211206175942.47326-2-dai.ngo@oracle.com>
 <133AE467-0990-469D-A8A9-497C1C1BD09A@oracle.com>
 <254f1d07c02a5b39d8b7743af4ceb9b5f69e4e07.camel@hammerspace.com>
 <20211206200540.GD20244@fieldses.org>
 <6aea870f-d51e-ed42-6f96-6b5b78edfcc3@oracle.com>
 <33e964f8aef8a94f8b4903c1b9b6c037e37cb325.camel@hammerspace.com>
From:   dai.ngo@oracle.com
In-Reply-To: <33e964f8aef8a94f8b4903c1b9b6c037e37cb325.camel@hammerspace.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Sk95RGRLdWtrQkZoSHAxcHU4OTAwLzAwbjNTdFY5Skl2a1RaUUc0MlB6bGZL?=
 =?utf-8?B?UFU5c2pZc1d4MEY2WTdGU2liaE9heWsrNmFnV0xmYURlYkhrRTI5dytRQ0di?=
 =?utf-8?B?cDZ2YU1ac3NFMHBWS05sekxBK3hjbzhwaFVlUnpXbTc2U081cEVQVitLUXZx?=
 =?utf-8?B?UGNWZncyZ2UxZzl2RnFtNW5Yd0pBblB4QktLVFpJYklUdUFXWUk5YUdiYXMr?=
 =?utf-8?B?UFNKbkRpcmJxdHNlbVNhakJXZVlqUG1hcWtvVWJkREZJcW1ONVBSdzFTeHky?=
 =?utf-8?B?bnQrUm5yUnBLcXVWRStKcDdhWEd6ZGs2NmtyeUpZMmJvMFk0VjkyNHROeXBS?=
 =?utf-8?B?Ulc2TjdraE5yS2s5clVMdmRnMVk4b0F3amc1VUV6T2FtcWdEbU51aXlsYWNL?=
 =?utf-8?B?aTFvNG5ZWmFzeEF0MDl5Qk92MWZ2cmY3RHNjVW4ySERqd0pxUkEzb3NTRTRk?=
 =?utf-8?B?cmxWTldaeWFiMEM0S2VqK1ZzcThSMVVvWjRYWk5ONXZxb0RiYTkrUDBxNWts?=
 =?utf-8?B?TTVOc0tXOFdoKzJGOUZvNzlDb0QwVUErUG95dVpOaEUzeXhLMUlLc2t0eE5B?=
 =?utf-8?B?UjBSY2NyOGN4WUtmUGVsNDNyQ3VGSnIxVWhvMG4xSmMyZ0t0c1N6S1VvSThZ?=
 =?utf-8?B?anFzWVZTeUwzN1BuZ0hlS1kyYXd6czBnWmxFcDNHNHdMejRNVU9xVVcwSmRH?=
 =?utf-8?B?cFZhcDRvaFhLV2w0czF3OUtjWUhoclJyTEVwS0RsQldWVk5PVDlhODhnL2hM?=
 =?utf-8?B?NlFJNm80YTJYd1E1a2p1QU95QWdJSjcrSXNZdFNXa0dZenk3UEI5TUVreXZP?=
 =?utf-8?B?Rk1MUjlyenl2ajFqSW9MWFhPdE9xNjJnUUh3R1B3Tks0aUFHQ3B0bGoxYlhG?=
 =?utf-8?B?aVM2VVlkQ2d2SEd3UHZsTDZMYnc0YUFnWHdBa0s2bjVWOWhzQkVsdHFTVzRD?=
 =?utf-8?B?aFJOa2tpUkQ0Y1l3eWl2OGZUSmJPZUpTdTVmY1RLSGxYcmw0b0pWdDh0Z2E1?=
 =?utf-8?B?QUYrSU9qWi9UbTFJUk9xZ05LTyt6WjRqWVdvUXNKRGRQNDl4L05mWjczWStE?=
 =?utf-8?B?b1dubUdvZ2pDT0VjN3pRWnB3TUZhT0Jod0JwLzd5TzBMa2NqaUtMa1ZlN290?=
 =?utf-8?B?M3NOdkxaYVhGK3NvSnpTWGVKamptZjYxQy9ORWZkamFvR1V3d21JOXJxRzkx?=
 =?utf-8?B?QW85cStLR2JqcWFYQW1JWE5SeElIYWVWUmc5cmtXVEthOVBqMVpoMnptYU5J?=
 =?utf-8?B?c2ZzQXBqejlDK3UzUDhUYVlhQ1hLUklrdnZlMi9XSUtha2p6djVhQUo2bDhX?=
 =?utf-8?B?YTUzYmw4UkRRcHc0U0VnSDEyVjdqYVZCUEFwMm5qcGc5TWtwUWQyVDA5Y01h?=
 =?utf-8?B?TDhlWThkOVd2YWk3NkorN3lkS201SGd4OWJCdWhGeXI0MDk0QTFaK0xCdmZW?=
 =?utf-8?B?UU5qQ1VXa0czSWZXTkE5SC9oSjlHc3V0RDgzMGprUG1tem1ldDdaaGRpWWtO?=
 =?utf-8?B?WnhTUGxjTmI1WWZJY0Y2QUw3OW9kSzAvQlhEVE83OFFpZ1VaUHpCWTBnRVlK?=
 =?utf-8?B?SDM0aEFXWldNL1B0K3dnekVtcVI1dW1IMzBEVEhCN0hESDZZRTRCdzN1bjdU?=
 =?utf-8?B?MGh5cWMwdzBXUi9nVU9TQ2xKTkVWVFJmaEN5dStIdzk1VXc4dW5UV2dQb0Zt?=
 =?utf-8?B?ZkdoZ3N1N1gzS1pwSmpYYlRuQ3pGTWw0djh0dFlhczJ6eVNVU3BTdlBTMGZT?=
 =?utf-8?B?ck91bHJnVGROTmJ1TFFTNm9GUzlLQVNlUDBCUEJ4V093QkNFcno1SW9KVElK?=
 =?utf-8?B?U3FydHpFYUlRSUx4M21NVDU3Y29KQXJNU2tTODljZnAyd21OSWxOZWNrclNC?=
 =?utf-8?B?QVd0YjlNN1BubFN4UTQyUGx6M3cxaW1QK2ltdjJ4TFdId3RhaTV3emFJT0t3?=
 =?utf-8?B?L3FySEoyQmdFYW41Tkh3a2NvM2NWN0Z4TXhMOGhGTThNT21NWDY0Yi9HcVdR?=
 =?utf-8?B?U0J5RCtjRE5Vakdndm5XRlB1QW9YdkkzOFYzdythRzFOS3VYb2NFcjlOWFJS?=
 =?utf-8?B?WXArbmpLVUJaMDU2MHFUMU1keGRwYWtZOGlPeWRNVHVTa0kvWGJYaFRuOXpo?=
 =?utf-8?B?M3FTckwvUnZEZ0hYYzdJSy9IVzJlWGFuU2JTZkZ2MGpkVlhPb3ZGanU4OG9o?=
 =?utf-8?Q?BOCTmlYk6rS6yrQdB6Y+aZA=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ef7109e0-5a04-4d2c-ada7-08d9b90d1eed
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4257.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Dec 2021 23:07:05.5491
 (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: mjq5dnEV/JqiFkmZe3mhZnrVwR2erpDTfhtRyGylq9olC16+HCH5HyHFntoyyy03KDfc+LmVu0hDu3cL8jiUSw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB5892
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10190 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 mlxscore=0 spamscore=0
 phishscore=0 bulkscore=0 suspectscore=0 malwarescore=0 adultscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112060139
X-Proofpoint-ORIG-GUID: HGczKmE_kgYUoGVRhxlUFK81CtfBM-hm
X-Proofpoint-GUID: HGczKmE_kgYUoGVRhxlUFK81CtfBM-hm
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org

On 12/6/21 2:05 PM, Trond Myklebust wrote:
> On Mon, 2021-12-06 at 12:36 -0800, dai.ngo@oracle.com wrote:
>> On 12/6/21 12:05 PM, bfields@fieldses.org wrote:
>>> On Mon, Dec 06, 2021 at 07:52:29PM +0000, Trond Myklebust wrote:
>>>> On Mon, 2021-12-06 at 18:39 +0000, Chuck Lever III wrote:
>>>>>> On Dec 6, 2021, at 12:59 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 true if lock conflict can be
>>>>>> resolved
>>>>>>                else return false.
>>>>>> 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>
>>>>> Al, Jeff, as co-maintainers of record for fs/locks.c, can you
>>>>> give
>>>>> an Ack or Reviewed-by? I'd like to take this patch through the
>>>>> nfsd
>>>>> tree for v5.17. Thanks for your time!
>>>>>
>>>>>
>>>>>> ---
>>>>>> fs/locks.c         | 28 +++++++++++++++++++++++++---
>>>>>> include/linux/fs.h |  1 +
>>>>>> 2 files changed, 26 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>> index 3d6fb4ae847b..0fef0a6322c7 100644
>>>>>> --- a/fs/locks.c
>>>>>> +++ b/fs/locks.c
>>>>>> @@ -954,6 +954,7 @@ posix_test_lock(struct file *filp, struct
>>>>>> file_lock *fl)
>>>>>>           struct file_lock *cfl;
>>>>>>           struct file_lock_context *ctx;
>>>>>>           struct inode *inode = locks_inode(filp);
>>>>>> +       bool ret;
>>>>>>
>>>>>>           ctx = smp_load_acquire(&inode->i_flctx);
>>>>>>           if (!ctx || list_empty_careful(&ctx->flc_posix)) {
>>>>>> @@ -962,11 +963,20 @@ 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 (cfl->fl_lmops && cfl->fl_lmops-
>>>>>>> lm_expire_lock
>>>>>> &&
>>>>>> +                               cfl->fl_lmops-
>>>>>>> lm_expire_lock(cfl,
>>>>>> 1)) {
>>>>>> +                       spin_unlock(&ctx->flc_lock);
>>>>>> +                       ret = cfl->fl_lmops-
>>>>>>> lm_expire_lock(cfl,
>>>>>> 0);
>>>>>> +                       spin_lock(&ctx->flc_lock);
>>>>>> +                       if (ret)
>>>>>> +                               goto retry;
>>>>>>                   }
>>>>>> +               locks_copy_conflock(fl, cfl);
>>>> How do you know 'cfl' still points to a valid object after you've
>>>> dropped the spin lock that was protecting the list?
>>> Ugh, good point, I should have noticed that when I suggested this
>>> approach....
>>>
>>> Maybe the first call could instead return return some reference-
>>> counted
>>> object that a second call could wait on.
>>>
>>> Better, maybe it could add itself to a list of such things and then
>>> we
>>> could do this in one pass.
>> I think we adjust this logic a little bit to cover race condition:
>>
>> The 1st call to lm_expire_lock returns the client needs to be
>> expired.
>>
>> Before we make the 2nd call, we save the 'lm_expire_lock' into a
>> local
>> variable then drop the spinlock, and use the local variable to make
>> the
>> 2nd call so that we do not reference 'cfl'. The argument of the
>> second
>> is the opaque return value from the 1st call.
>>
>> nfsd4_fl_expire_lock also needs some adjustment to support the above.
>>
> It's not just the fact that you're using 'cfl' in the actual call to
> lm_expire_lock(), but you're also using it after retaking the spinlock.

I plan to do this:

         checked_cfl = NULL;
         spin_lock(&ctx->flc_lock);
retry:
         list_for_each_entry(cfl, &ctx->flc_posix, fl_list) {
                 if (!posix_locks_conflict(fl, cfl))
                         continue;
                 if (check_cfl != cfg && cfl->fl_lmops && cfl->fl_lmops->lm_expire_lock) {
                         void *res_data;

                         res_data = cfl->fl_lmops->lm_expire_lock(cfl, 1);
                         if (res_data) {
                                 func = cfl->fl_lmops->lm_expire_lock;
                                 spin_unlock(&ctx->flc_lock);
                                 func(res_data, 0);
                                 spin_lock(&ctx->flc_lock);
                                 checked_cfl = cfl;
                                 goto retry;
                         }
                 locks_copy_conflock(fl, cfl);
                 goto out;
         }
         fl->fl_type = F_UNLCK;

Do you still see problems with this?

-Dai