From:   Chuck Lever III <chuck.lever@oracle.com>
To:     Dai Ngo <dai.ngo@oracle.com>,
        Calum Mackay <calum.mackay@oracle.com>
CC:     Bruce Fields <bfields@fieldses.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH RFC v6 2/2] nfsd: Initial implementation of NFSv4
 Courteous Server
Thread-Topic: [PATCH RFC v6 2/2] nfsd: Initial implementation of NFSv4
 Courteous Server
Thread-Index: AQHX6ssTAZRABZ1HqkCYuoltcOnBFawl4NyAgALhbICAAAEdAA==
Date:   Wed, 8 Dec 2021 15:58:50 +0000
Message-ID: <6E81B5EC-9449-4337-9FEB-66DEE08809D1@oracle.com>
References: <20211206175942.47326-1-dai.ngo@oracle.com>
 <20211206175942.47326-3-dai.ngo@oracle.com>
 <4025C4F6-258F-4A2E-8715-05FD77052275@oracle.com>
 <f6d65309-6679-602a-19b8-414ce29c691a@oracle.com>
In-Reply-To: <f6d65309-6679-602a-19b8-414ce29c691a@oracle.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?oHL0qENpqw8Sb9nV6epd9QaNU52SlPVDV6q5tkx67F6QRS5t4Umr4HhUAKNv?=
 =?us-ascii?Q?jYufIFkawSsIjZ5vCQxx38wOgQuk0Am0h0pjFGNyev4CXQwNgvmc5P/bDEft?=
 =?us-ascii?Q?uY+A1GURJXh1ke7nWF9yDFZV2fby2EISfNSNfztbahQfKBq8ND35kB2VXfQi?=
 =?us-ascii?Q?XCq5+d0DgfoWZedBFBAjrDGYNj+DiO21ksHV/3ec+dftGfbPOHyVwv69TPhp?=
 =?us-ascii?Q?H0drs99xRTQWsiclObKfnA68DZ+34pB/mQxdjv8jSE3e7QT5MSsBtdtzvdLB?=
 =?us-ascii?Q?jgNFDgn0HdKOSqRdTjTTILFqtFORWypz1mTq5hvQa8SoSjfEolhwSj8S9GQ+?=
 =?us-ascii?Q?3+Ekl/gQD85hR+Buj1ukyopktBLZSbuZ6ocRGLL5bBvBkHAvmVY8U2QkI9dM?=
 =?us-ascii?Q?hvt6EUOzSRjc049aS7qRX/IEy1xZ1Sy1pEmvxmqKwKMxO/412spBbBNrmZii?=
 =?us-ascii?Q?67Kw00hNSBJDGwwocnsKai5J/WEvxSxbTji9v91uXJj/ZovPHQNGrZJMat1Q?=
 =?us-ascii?Q?hp/+PydnhAZ9obTm/JeljDtV8G0Kc16uQMb1gPqcbVwK7Vc/YBY4dXqozh1r?=
 =?us-ascii?Q?3VslgUbidrkrVxcMK5RpSXsllVguoLSRWoKLO68BZK0ti5+AC+kYqtRP9rbU?=
 =?us-ascii?Q?0w4PlxM2I2M8TYkCCb8OhAICcXuvV5q24krXF1+GgiZiFL7kKhVYAylPAO3z?=
 =?us-ascii?Q?3GBKBomf6FtInVk7lsdaNTsNykKDcmwkWj0Sjr7iWkd3VTA52vcvcf5acE7L?=
 =?us-ascii?Q?NTiLC3M2ELovOTyvJMMMqFdesfGmqb4BS/WFpbztr9rc9qTlvPpF4HlGeK4l?=
 =?us-ascii?Q?VOBEM20ewHoKhzi/hbj+0myy4cHQ0p9Uiptf+lAa+qAAIILmp1GNJYm/RcWw?=
 =?us-ascii?Q?4jdPEBD5QwsyBYm57HHk165ev5hFQSwLwW+o+/38vLs4DJUNk1dsZkFAEA04?=
 =?us-ascii?Q?Rq4lZ03zbwxDHhPwd1MWPfYKw38kVaF/XKIIZkmyV0xkL9R8Mh5wRszpT+R+?=
 =?us-ascii?Q?OyP3ypOfbbTdoMhthIWbzdhUcydbI8sH8tjvJwNvEn94Yyu5Y1/03NNr0AJu?=
 =?us-ascii?Q?gcR0tQvi2+g6eUTTxMJH0sgtn8KLInMCAHuZ870RFRKRmJx9Z1KyaQ14iNjn?=
 =?us-ascii?Q?j6KP8x0EIxbtPuUfqSjUv4K4wdO06ShB6uYbLLpIEvzcnjge0xBlVCCFI8zd?=
 =?us-ascii?Q?vMPMWvcSoQsfjXCUyZXNKcL45MJf+EBgjPVnF/CWQDEpUzfMTSxclj1NiJkj?=
 =?us-ascii?Q?+W/g6N60Bk3aFfw4os+jCAyeEFoiqDFBxoJ5Rv/0vmJfKuAeivtOU3WK+WD7?=
 =?us-ascii?Q?2/8GKGH/+1/Byvjrcby6qJcldnMT65MJYfdkWPI70LioV8xsR05iBQcxTXFP?=
 =?us-ascii?Q?O1hAax8haYXyfkNcm4HuBe28zJThLNh1kW3oXCD77nP1AbHQvVZ8rij1EZcm?=
 =?us-ascii?Q?3rPwO+J1enWG0PSHkX4JOnL9opUudWrVox2haQEu6Q4zYW0ZXrh8R+PT0aNY?=
 =?us-ascii?Q?621oPSzRi3LJwLodtBo61ZeVcO0+ujS3tEW5sK065pjpMZx5kKWI1P4J1tAf?=
 =?us-ascii?Q?qT5N31iLQrlgcje8vtChwyh8VZQXGa8orhPxrnYZqFN4HIsSJ7cA/7OIAq6g?=
 =?us-ascii?Q?7C+X+1G++MXeQIuMi/i6Z5c=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <B1DD52A656E47D4F907E1671A247B496@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: b8624ea4-8181-444c-8ae0-08d9ba63a0dc
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Dec 2021 15:58:51.0085
 (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: FdDrVQeTfH+pem+pjT4HtUtWQym1fkB7wfq7NMh3rar02u1g/+SdqA/kx6GJvlzO4XY3DrRvpq0s+kIDf4y2KA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR10MB4166
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10192 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 phishscore=0
 malwarescore=0 spamscore=0 mlxlogscore=999 adultscore=0 bulkscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2112080096
X-Proofpoint-GUID: BMsJzY2OW4wkiPolk75l7zwV2q3FcvEH
X-Proofpoint-ORIG-GUID: BMsJzY2OW4wkiPolk75l7zwV2q3FcvEH
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Dec 8, 2021, at 10:54 AM, Dai Ngo <dai.ngo@oracle.com> wrote:
>=20
> On 12/6/21 11:55 AM, Chuck Lever III wrote:
>=20
>>=20
>>> +
>>> +/*
>>> + * Function to check if the nfserr_share_denied error for 'fp' resulte=
d
>>> + * from conflict with courtesy clients then release their state to res=
olve
>>> + * the conflict.
>>> + *
>>> + * Function returns:
>>> + *	 0 -  no conflict with courtesy clients
>>> + *	>0 -  conflict with courtesy clients resolved, try access/deny chec=
k again
>>> + *	-1 -  conflict with courtesy clients being resolved in background
>>> + *            return nfserr_jukebox to NFS client
>>> + */
>>> +static int
>>> +nfs4_destroy_clnts_with_sresv_conflict(struct svc_rqst *rqstp,
>>> +			struct nfs4_file *fp, struct nfs4_ol_stateid *stp,
>>> +			u32 access, bool share_access)
>>> +{
>>> +	int cnt =3D 0;
>>> +	int async_cnt =3D 0;
>>> +	bool no_retry =3D false;
>>> +	struct nfs4_client *cl;
>>> +	struct list_head *pos, *next, reaplist;
>>> +	struct nfsd_net *nn =3D net_generic(SVC_NET(rqstp), nfsd_net_id);
>>> +
>>> +	INIT_LIST_HEAD(&reaplist);
>>> +	spin_lock(&nn->client_lock);
>>> +	list_for_each_safe(pos, next, &nn->client_lru) {
>>> +		cl =3D list_entry(pos, struct nfs4_client, cl_lru);
>>> +		/*
>>> +		 * check all nfs4_ol_stateid of this client
>>> +		 * for conflicts with 'access'mode.
>>> +		 */
>>> +		if (nfs4_check_deny_bmap(cl, fp, stp, access, share_access)) {
>>> +			if (!test_bit(NFSD4_COURTESY_CLIENT, &cl->cl_flags)) {
>>> +				/* conflict with non-courtesy client */
>>> +				no_retry =3D true;
>>> +				cnt =3D 0;
>>> +				goto out;
>>> +			}
>>> +			/*
>>> +			 * if too many to resolve synchronously
>>> +			 * then do the rest in background
>>> +			 */
>>> +			if (cnt > 100) {
>>> +				set_bit(NFSD4_DESTROY_COURTESY_CLIENT, &cl->cl_flags);
>>> +				async_cnt++;
>>> +				continue;
>>> +			}
>>> +			if (mark_client_expired_locked(cl))
>>> +				continue;
>>> +			cnt++;
>>> +			list_add(&cl->cl_lru, &reaplist);
>>> +		}
>>> +	}
>> Bruce suggested simply returning NFS4ERR_DELAY for all cases.
>> That would simplify this quite a bit for what is a rare edge
>> case.
>=20
> If we always do this asynchronously by returning NFS4ERR_DELAY
> for all cases then the following pynfs tests need to be modified
> to handle the error:
>=20
> RENEW3   st_renew.testExpired                                     : FAILU=
RE
> LKU10    st_locku.testTimedoutUnlock                              : FAILU=
RE
> CLOSE9   st_close.testTimedoutClose2                              : FAILU=
RE
>=20
> and any new tests that opens file have to be prepared to handle
> NFS4ERR_DELAY due to the lack of destroy_clientid in 4.0.
>=20
> Do we still want to take this approach?

I'm still interested, but Bruce should chime in.

Maybe Calum could have a look under the covers of pynfs and see how difficu=
lt the change might be.


--
Chuck Lever