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>
CC:     Dai Ngo <dai.ngo@oracle.com>, Jeff Layton <jlayton@redhat.com>,
        Al Viro <viro@zeniv.linux.org.uk>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [PATCH RFC v19 06/11] NFSD: Update find_clp_in_name_tree() to
 handle courtesy client
Thread-Topic: [PATCH RFC v19 06/11] NFSD: Update find_clp_in_name_tree() to
 handle courtesy client
Thread-Index: AQHYRRiz4DLghbl5p0mDl+lBwBV++6zbLgiAgAAKP4A=
Date:   Fri, 1 Apr 2022 15:57:50 +0000
Message-ID: <52CA1DBC-A0E2-4C1C-96DF-3E6114CDDFFD@oracle.com>
References: <1648742529-28551-1-git-send-email-dai.ngo@oracle.com>
 <1648742529-28551-7-git-send-email-dai.ngo@oracle.com>
 <20220401152109.GB18534@fieldses.org>
In-Reply-To: <20220401152109.GB18534@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?E7n+gt1Ps5xlS5surYmKtLejM1LrjlIpcAMAqe0AxCJ1jhru9Rq4TdRfyKRP?=
 =?us-ascii?Q?wViYMa8fjXmwtS8kAA49HTmO7awHQKTrTl7MhAHRmiPd47d4nV9c+gaYWI0x?=
 =?us-ascii?Q?+KtEpoM0LuxhvD13zCd3Z9+t9tU7hCaLleDCQItTn9qF/7rSleByZTidfKEQ?=
 =?us-ascii?Q?Lkbd7KEEz/qXXv5WvOtCtI5wwbbZgNMctbdgE/lWYKNGo1nz0XWJUy54b116?=
 =?us-ascii?Q?1tLqwsO1YDMfCUPRCcJnpocKucZGZDoMz5SFrUpe8d7Kt7HyaapWarFhgox3?=
 =?us-ascii?Q?lIh5Fv4dCZ9EaXFjEIolFVuDEKedNg4lbmN++hIGFmfR4fmKXA5i9L6piu+M?=
 =?us-ascii?Q?jQhSOWUjE54j0VRxX/l2cxbl6u2dT6g8h57+X0OpBdRIon2+ALCxG0IvUgnk?=
 =?us-ascii?Q?PaeDCWLN8GHT4F/mJyuB9Rx77Tkf6eRMiXYY78bRdwFH6T6SREnO/0V0KQbe?=
 =?us-ascii?Q?xdnXMTmGgpxaqEDsdVr7y+Pyne8ZMc4gU8a9N9X48nNUvivIsiSNGKenH5O7?=
 =?us-ascii?Q?XF2fYAmeEsQsqZLxVOTfKQbRwb6WrHUznJOmsDtvx4W5myz3iqy9nJ5eceJN?=
 =?us-ascii?Q?FaDeJyXOT3xSdd536uCPvKIwL+Mtnvi2Mx3B81lHodYfaX6QVa0wpqWkVrOD?=
 =?us-ascii?Q?M0zzpS7ckgo6LPpoc1x7Z8wGGs2ZFJJ/H870/hd3b96bX+6n2ZZa7hAk2pK7?=
 =?us-ascii?Q?evBlTRCzCf6BH7/lD4c8PDw8697v5SXgGBCHs5BI4C2WqTHeGtPABzDl1WSh?=
 =?us-ascii?Q?JR5yhQQMFTgof1inFNVtBkIyTK7qT3BavKb6hFvjagC4Yy60BcFlR68fGOnL?=
 =?us-ascii?Q?11aH8jCC72UHJqfcaL5U3ZmnpekJHZ/bI2geMPkHjy5cI673urREvfXT+gzX?=
 =?us-ascii?Q?clT+mOw7Tjq4RkilyRC2U7tW6y9VTVl4BAk6E5oyxMqySswjR3l10TsLHDgh?=
 =?us-ascii?Q?XgOskx6T558VCsuiAMkJL6WUAVQQJ3719qRNYfuzJLPSWH4o+4LSYzAC/AAI?=
 =?us-ascii?Q?TYIEMvTUgUyhRxKLHwqCu2cISmIeIF/R1BmOf7PfezTiNQYIIkAenMo/+LCw?=
 =?us-ascii?Q?ds2uJnDoaPD4TAY0bfjglJeDyRNUoBBe/XqqVBo0Imkqvlkd/1tDs6F0HX3I?=
 =?us-ascii?Q?uc57/bQ/ESn4RnlylKL0Z2OskptzjC3rLU/JKBa7veEoaUnTNRjbksLipMcy?=
 =?us-ascii?Q?jJZRSl6TDFAkORH/y8+PeFYfZ9nrrqfbtt7y2HDY7CqRGUOXFnyH8lUfpzGg?=
 =?us-ascii?Q?xyHvousua0Sk8/JvKDhrmbtc6/Nav4MGeNonPaiynKGvcAT5hFPRR8hceegE?=
 =?us-ascii?Q?vHUURL10mXLG1SNgQ/kievpSZdEF006ETaD4Wg9TuCQmVld+f7m6iu/ODvRn?=
 =?us-ascii?Q?biGwztnbeITqOoJGU2c++AHKt+IzgOAq6qgOAQTbYLbSsz2QoP9Rvg/kK/fj?=
 =?us-ascii?Q?HkYrdpt8qOJcVYF40C0ZORmXOdLVhP/VzXYnaYmh8/GymJzVaFHXY52WctKw?=
 =?us-ascii?Q?y46wy1m9rr1Lsn5HOEJlq08267pv30VpfXwAJweR/7xBc5UdhmHRxoMlhriV?=
 =?us-ascii?Q?MsKgWr3vwKfibAvufsdm09NNpl33bhhxLtxjAyTqY/LroRHKovj362gXuggk?=
 =?us-ascii?Q?KRxqIybNCzAUzg7WSxe35bznA/xRp8zS7k280YHSE4yRZ57SO06CRyBa1ERR?=
 =?us-ascii?Q?3wKZfmrAVturF+abjg0duDOPx3H/27JwsPXuL+f3/FhsgsmAP5cv4UEVqAOu?=
 =?us-ascii?Q?/4xG92lvI5xJDoGonX/pSNL8CVRDtpc=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <0EC5A8171AF27C41A3E4F1F0FD3DD00A@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: BN0PR10MB5128.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e85c1436-cdcf-42b9-6406-08da13f85fb8
X-MS-Exchange-CrossTenant-originalarrivaltime: 01 Apr 2022 15:57:50.4146
 (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: ptnUx6/2JErG3nfJfMHyFpd8HVannFlLVWA4TzSPZH/YLRQkDfO8x0RRF8uni1i2HGEQrpJZnf6LsAUnXFLWkA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB3876
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.425,18.0.850
 definitions=2022-04-01_05:2022-03-30,2022-04-01 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 suspectscore=0
 spamscore=0 mlxscore=0 phishscore=0 malwarescore=0 bulkscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2202240000 definitions=main-2204010077
X-Proofpoint-GUID: fouTvWnicoYEo7w-th0eXd_29ySMuPKf
X-Proofpoint-ORIG-GUID: fouTvWnicoYEo7w-th0eXd_29ySMuPKf
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 Apr 1, 2022, at 11:21 AM, J. Bruce Fields <bfields@fieldses.org> wrote=
:
>=20
> On Thu, Mar 31, 2022 at 09:02:04AM -0700, Dai Ngo wrote:
>> Update find_clp_in_name_tree to check and expire courtesy client.
>>=20
>> Update find_confirmed_client_by_name to discard the courtesy
>> client by setting CLIENT_EXPIRED.
>>=20
>> Update nfsd4_setclientid to expire client with CLIENT_EXPIRED
>> state to prevent multiple confirmed clients with the same name
>> on the conf_name_tree.
>>=20
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4state.c | 27 ++++++++++++++++++++++++---
>> fs/nfsd/state.h     | 22 ++++++++++++++++++++++
>> 2 files changed, 46 insertions(+), 3 deletions(-)
>>=20
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index fe8969ba94b3..eadce5d19473 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2893,8 +2893,11 @@ find_clp_in_name_tree(struct xdr_netobj *name, st=
ruct rb_root *root)
>> 			node =3D node->rb_left;
>> 		else if (cmp < 0)
>> 			node =3D node->rb_right;
>> -		else
>> +		else {
>> +			if (nfsd4_courtesy_clnt_expired(clp))
>> +				return NULL;
>> 			return clp;
>> +		}
>> 	}
>> 	return NULL;
>> }
>> @@ -2973,8 +2976,15 @@ static bool clp_used_exchangeid(struct nfs4_clien=
t *clp)
>> static struct nfs4_client *
>> find_confirmed_client_by_name(struct xdr_netobj *name, struct nfsd_net *=
nn)
>> {
>> +	struct nfs4_client *clp;
>> +
>> 	lockdep_assert_held(&nn->client_lock);
>> -	return find_clp_in_name_tree(name, &nn->conf_name_tree);
>> +	clp =3D find_clp_in_name_tree(name, &nn->conf_name_tree);
>> +	if (clp && clp->cl_cs_client_state =3D=3D NFSD4_CLIENT_RECONNECTED) {
>> +		nfsd4_discard_courtesy_clnt(clp);
>> +		clp =3D NULL;
>> +	}
>> +	return clp;
>> }
>>=20
> ....
>> +static inline void
>> +nfsd4_discard_courtesy_clnt(struct nfs4_client *clp)
>> +{
>> +	spin_lock(&clp->cl_cs_lock);
>> +	clp->cl_cs_client_state =3D NFSD4_CLIENT_EXPIRED;
>> +	spin_unlock(&clp->cl_cs_lock);
>> +}
>=20
> This is a red flag to me.... What guarantees that the condition we just
> checked (cl_cs_client_state =3D=3D NFSD4_CLIENT_RECONNECTED) is still tru=
e
> here?  Why couldn't another thread have raced in and called
> reactivate_courtesy_client?
>=20
> Should we be holding cl_cs_lock across both the cl_cs_client_state and
> the assignment?

Holding cl_cs_lock while checking cl_cs_client_state and then
updating it sounds OK to me.


> Or should reactivate_courtesy_client be taking the
> client_lock?
>=20
> I'm still not clear on the need for the CLIENT_RECONNECTED state.
>=20
> I think analysis would be a bit simpler if the only states were ACTIVE,
> COURTESY, and EXPIRED, the only transitions possible were
>=20
> 	ACTIVE->COURTESY->{EXPIRED or ACTIVE}
>=20
> and the same lock ensured that those were the only possible transitions.

Yes, that would be easier, but I don't think it's realistic. There
are lock ordering issues between nn->client_lock and the locks on
the individual files and state that make it onerous.


> (And to be honest I'd still prefer the original approach where we expire
> clients from the posix locking code and then retry.  It handles an
> additional case (the one where reboot happens after a long network
> partition), and I don't think it requires adding these new client
> states....)

The locking of the earlier approach was unworkable.

But, I'm happy to consider that again if you can come up with a way
of handling it properly and simply.


--
Chuck Lever