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;
From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Bruce Fields <bfields@fieldses.org>,
        Jeff Layton <jlayton@redhat.com>
CC:     Dai Ngo <dai.ngo@oracle.com>, Al Viro <viro@zeniv.linux.org.uk>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH RFC v12 1/3] fs/lock: add new callback, lm_lock_conflict,
 to lock_manager_operations
Thread-Topic: [PATCH RFC v12 1/3] fs/lock: add new callback, lm_lock_conflict,
 to lock_manager_operations
Thread-Index: AQHYHjn6Ujwua4SbTkavmYzo8J2MF6yM2HoAgAAnq4A=
Date:   Thu, 10 Feb 2022 16:50:26 +0000
Message-ID: <2AEF8E7D-2F4E-4D88-8B71-48195C6E45ED@oracle.com>
References: <1644468729-30383-1-git-send-email-dai.ngo@oracle.com>
 <1644468729-30383-2-git-send-email-dai.ngo@oracle.com>
 <20220210142826.GD21434@fieldses.org>
In-Reply-To: <20220210142826.GD21434@fieldses.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?TboHXGxwPHebir3Z8oL2KbOnklQJjSOfSZWrCDSMz/cJGfUqDkl949qjIT9Q?=
 =?us-ascii?Q?nZWtjEAwV2g7hXDPbSgQiLl+l5lprgJGVJKCLVoI7+HPbhMjrvNW+p73FFoV?=
 =?us-ascii?Q?o9o/TQ3DdbLvfMRSq2qCqstQUzu8Atj1Rkfsit7dKFLWRjlinbk9WyVZj9oL?=
 =?us-ascii?Q?r3zf96TcPSslvUkiMGgc2GA2y2MGwH2yy+0zaOVKDHHiFyovpvGx4+rGSk2Z?=
 =?us-ascii?Q?UCImumHLtRkDfZV4ybmf+MS1C562F8ZzM2ltXjJNJ6ljQ7mdr3xSc/GIwxAh?=
 =?us-ascii?Q?f7YjAxun3tTF1mjWKu10d3RFNacKBwHhogb801KiPBK5En6pCOWoBNMY0CP4?=
 =?us-ascii?Q?xzZ3lD/byJjEg+r/sFiWiszCpoDbgnu4liIsp0Nmhek7tIc4eWv5e3+Nv0Qt?=
 =?us-ascii?Q?NKwOesXov1NBpRt5Ctu0NtmPwoJKmZ0Pg3Wf1pD8CxxRZdIg2jx/j2lWS29P?=
 =?us-ascii?Q?uqe+It2iEkLNoc+8sApENJZY6vU3XyDbQzgOJzp4gwO4dRyoSacmnPJ01ENH?=
 =?us-ascii?Q?0U8RbJWlSFJNIkzT+fIA/RQxXSouXPciN30mQf+fm4NsPcKoPmxGNA+HKsKq?=
 =?us-ascii?Q?1TbCbo6ARr+818y8saTK4hhixYbcB+smi8D++Zb0hgYy94zfwybIe3DQlcsU?=
 =?us-ascii?Q?uOmdewwNDF1szZI1NGs+sPg+7pKKexIl5GECr/aHoxvQxj2rJVuza3M0uctv?=
 =?us-ascii?Q?V+cYJVC/YOPHGN1fgcw76p9xDu2h3EVtXYf/frQAzlXrMLWCUXOPI5XswHXg?=
 =?us-ascii?Q?bGRqnMK0TIfu3mgaHT1rUdeiDW4XyUwMk091yoCpe7jEdrDf+cLY9JqdrEQI?=
 =?us-ascii?Q?epjfl5l1dYJ95McOyASjBETAT9wvSltic0z8/wqTkLRyfIBsZBuID4PuahC5?=
 =?us-ascii?Q?yAafi+wWGZPUQFseuzWe6prmLTscsLSA+Dxz9vZ9hbF24w1ksyXd5I+f5Wnr?=
 =?us-ascii?Q?y8m6X/5/zBIRJAJZNN3j65DHuheoMi8vkcMK/3pUL1fy6FkRVuIU2yv/adE+?=
 =?us-ascii?Q?kEFZ7T3pyNbH2GOaFiQfSC3vHsYU7sxyXOioaWQL9F138UESfybSohJpOja+?=
 =?us-ascii?Q?Yo+PYCGipWGR99RXJnRs2uTRCQEHIiUdJK0z36DwQ3KCdxttV18E++Szvpb8?=
 =?us-ascii?Q?zrFeki/XH90FKyfdNISSSwamzGM1rLc+dFJC3+R67mOmDC8xXzwjHdwAiVju?=
 =?us-ascii?Q?dkFH5OmJRLZs/Az0eNzie9/b79d+bPOa8LHupP7KvfjgFG8TkajSkq4QhWfj?=
 =?us-ascii?Q?HxyH/1dmPF/XuDlLZCKJSCq5X0l8gxedFGgRVonD54eF4NA61hTf/DK0zmJn?=
 =?us-ascii?Q?XDqz8oPV8j802QWf9R5SBRz+JQbhP/pgKe9NoAg510skWw096ZK+oJDwFmOq?=
 =?us-ascii?Q?l5E1FqMwt7osT6OM1I2QRPH3TYVG/H2vbTs95GS6LU/7iqMT8o5HNjYG0veJ?=
 =?us-ascii?Q?ivUGCUj6QKCPC3+BSUm9e1BpdJFfq1xra/1pAWPbhYgLbv1q3WdC+VkkL6BN?=
 =?us-ascii?Q?08TBPCkeiTaMl20lKbSGnrwaihJPPJ76Fi+xY5KvMuuDZqzIFi6T53RuFWDm?=
 =?us-ascii?Q?sHUU0y+2KU+lsWeIL9RPVj1aM06Yam6vHafhSg4tFgxXYQw6eGkBFO4WSFbr?=
 =?us-ascii?Q?AAlrrVPaeweAJNxEU9SCU8c=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <50EF3D782FB5DA46869E8E7FF9E71C5D@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH0PR10MB4858.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 719c6268-201b-4925-9d9f-08d9ecb56fff
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Feb 2022 16:50:26.1269
 (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: akbjlQR80pP3KM6tGJjD+zjXTdUApzCVf4IuTJvSP25ealqletBFDkY26Rt3GM1GdygXBBmPpSOPU6w/QxJ7FA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR10MB2881
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10254 signatures=673431
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 phishscore=0
 suspectscore=0 adultscore=0 malwarescore=0 bulkscore=0 spamscore=0
 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2201110000 definitions=main-2202100088
X-Proofpoint-ORIG-GUID: TOhD_jA7PH9TcgulIkB38_VlfFWSJenU
X-Proofpoint-GUID: TOhD_jA7PH9TcgulIkB38_VlfFWSJenU
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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 Feb 10, 2022, at 9:28 AM, J. Bruce Fields <bfields@fieldses.org> wrote=
:
>=20
> On Wed, Feb 09, 2022 at 08:52:07PM -0800, Dai Ngo wrote:
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index bbf812ce89a8..726d0005e32f 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1068,6 +1068,14 @@ 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 *);
>> +	/*
>> +	 * This callback function is called after a lock conflict is
>> +	 * detected. This allows the lock manager of the lock that
>> +	 * causes the conflict to see if the conflict can be resolved
>> +	 * somehow. If it can then this callback returns false; the
>> +	 * conflict was resolved, else returns true.
>> +	 */
>> +	bool (*lm_lock_conflict)(struct file_lock *cfl);
>> };
>=20
> I don't love that name.  The function isn't checking for a lock
> conflict--it'd have to know *what* the lock is conflicting with.  It's
> being told whether the lock is still valid.
>=20
> I'd prefer lm_lock_expired(), with the opposite return values.

Or even lm_lock_is_expired(). I agree that the sense of the
return values should be reversed.


The block comment does not belong in struct lock_manager_operations,
IMO.

Jeff's previous review comment was:

>> @@ -1059,6 +1062,9 @@ static int posix_lock_inode(struct inode *inode, s=
truct file_lock *request,
>> 		list_for_each_entry(fl, &ctx->flc_posix, fl_list) {
>> 			if (!posix_locks_conflict(request, fl))
>> 				continue;
>> +			if (fl->fl_lmops && fl->fl_lmops->lm_lock_conflict &&
>> +				!fl->fl_lmops->lm_lock_conflict(fl))
>> +				continue;
>=20
> The naming of this op is a little misleading. We already know that there
> is a lock confict in this case. The question is whether it's resolvable
> by expiring a tardy client. That said, I don't have a better name to
> suggest at the moment.
>=20
> A comment about what this function actually tells us would be nice here.


I agree that a comment that spells out the API contract would be
useful. But it doesn't belong in the middle of struct
lock_manager_operations, IMO.

I usually put such information in the block comment that precedes
the individual functions (nfsd4_fl_lock_conflict in this case).

Even so, the patch description has this information already.
Jeff, I think the patch description is adequate for this
purpose -- more information appears later in 3/3. What do you
think?


--
Chuck Lever