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:     Jeff Layton <jlayton@kernel.org>
CC:     Dai Ngo <dai.ngo@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] NFSD: add support for sending CB_RECALL_ANY
Thread-Topic: [PATCH v2 1/2] NFSD: add support for sending CB_RECALL_ANY
Thread-Index: AQHY5kFu1afwEzLcEkq5CCpU1lPbEa4deH2AgABOEIA=
Date:   Mon, 24 Oct 2022 16:56:17 +0000
Message-ID: <0DF26ACC-0D67-46D7-A213-40B00AECB138@oracle.com>
References: <1666462150-11736-1-git-send-email-dai.ngo@oracle.com>
 <1666462150-11736-2-git-send-email-dai.ngo@oracle.com>
 <d43a3dac01f8c4211ec7634a0d78dae70468f39b.camel@kernel.org>
In-Reply-To: <d43a3dac01f8c4211ec7634a0d78dae70468f39b.camel@kernel.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?BWN0Z0ai0JuprOY1loMEUfNXhsvClpI74wDDog7e4zkAagn14XWVLOy0GI19?=
 =?us-ascii?Q?u0BBHXkdWWhsOzFIRigrAXhEV5M6UH9hJ1+Py82ZuPJnqQqSj9L/SSC62GdK?=
 =?us-ascii?Q?AXjUoUt0WvamJcxmkj5Un8uWD/IIY5QHCIqZ794WyBt7Jtb/gZLmKwdf8GB1?=
 =?us-ascii?Q?kPKTdQLYb+ScSe4wZVqHjhgkWQ+lifBKsnUHce6Vs/xtfry5S7BdhCrScIU+?=
 =?us-ascii?Q?wd8hO2wJgO/aGpeA0aN+HTo17CP2srUibt0cilRNWzmc+HIIVwyqC7uRB9e+?=
 =?us-ascii?Q?yQVeilF5e67xosRdlOuleooPoapl8JWMyPcYCv0JqwS2DUTdm/2eo7JWIXUA?=
 =?us-ascii?Q?NEoftqCM9mqjWDhRfmKsAmI3Q4OwvVPMIKFz/g3cpPHjdOzCs1gNvVrTZo6l?=
 =?us-ascii?Q?wUcc9qtI/VevpZy5jDVXfu1BRR3VApOBEUTT4L+4G/NUPoYiq02HzuoJRgGM?=
 =?us-ascii?Q?XbhcXP84YT9bSSA6ey+rYnmJfVDxCgA6h9a6m4DaSVvBK9zF7FzZbsb6f0up?=
 =?us-ascii?Q?t/5PMvRhZcHskJlsDsbBFZslm9MUfjcmCtg29S0i08KCoLbBCxgUgVxTcV1Y?=
 =?us-ascii?Q?Gsj+kOVw/5IfTxpKk0kwhxSMYzBf/WjB42Dg2GmwrQvkga8VZUwBoxnGeMeC?=
 =?us-ascii?Q?6QJiXLBgDV403aHlZRKIeiMGYveVd+ATLmlVOXiLXutcKJsP7AQgQSW3ef3k?=
 =?us-ascii?Q?lZ8H14XVEbH0UrUhspjbO2ypyPXmOIwVbKP9BBrcLbn1F0bLh1E2irANLvjt?=
 =?us-ascii?Q?02vzm3BeR2wF9ePpseyR+HO6JHiCZ/89TKSahUNhsMMK0UwW/zvheNXSSBcD?=
 =?us-ascii?Q?+nZetL6EL+7+fEu0T+YCB3Lv/kbmvrhL9Scwh13JR3az+FtlTbopIScohbvM?=
 =?us-ascii?Q?ldN/4Wp4pZHY50BO82scH1sQj4D9w9ZzPyq5mL5xysbTbt3Dnrg8yFVmEGUW?=
 =?us-ascii?Q?8Fg4aHTwaHfCWWIhtYFHc6VrT9NloUmjgM5a/l2gQn8HyC59/GxzF/Zkb6e0?=
 =?us-ascii?Q?uuQMSiC9QuYaHAKKjD3Rpa3bQrQn0vsEm7UtOsqJ6UkIz7lLMm8qLC3dujbh?=
 =?us-ascii?Q?W5rrqm4gQIRqt37rNO3GS7pHlSVBrxCD0acgspoYAN414IK3zQmwe5C0HT65?=
 =?us-ascii?Q?TSNYxVn4OOVn5pfioJp0bvZdulQ1f5SjB6yZ4XKgl37u99wOP/8BJYXpuaii?=
 =?us-ascii?Q?2H3tMK0wx+YSPnvUaAQaR9iHlgZCXGlSrgicXG/fZcznlaSY5Q/7X+mIYp04?=
 =?us-ascii?Q?xOyaRrF2u5IKqEFjUVcuTa310mdz/0hjjs8Ajp5IWT1rtsSFvXmqbDlzpQYv?=
 =?us-ascii?Q?y2wnjVlFtJcIiec0sJsBbW6s/yQRnx/HhvDPOv0oPIx7w2B2brZVNZatP4S3?=
 =?us-ascii?Q?q3p+MNepIKZl2r9PYr3tUxUagyXl5iB92UJ9/vL8kgxn8QEudTADT86cNmY7?=
 =?us-ascii?Q?kFQbZQwSFUV7I3+Nx7mid7ggqrS70Y0ysl1GhY6wo+8Whnxw03FZO5mK5Uye?=
 =?us-ascii?Q?HiCxpol3ixAWT/meacjfO75PClOYZ9fVA0zaRQCfYOFP7pdMCn3vPWjm75Zj?=
 =?us-ascii?Q?Xc7K1iAuWgcFRj8k8SOsfeb9bD4Pu9lvX4BvDRpQpf05ECS685AsOoaI/iSo?=
 =?us-ascii?Q?rw=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <AECA3F7BCFAD564CB694320F23A703EC@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: 0a743306-ff3c-448d-3e91-08dab5e0ab56
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Oct 2022 16:56:17.7891
 (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: UA6La/Oi5toUhZbfsVVitHrCw2ILd+4fWCYxs4Py6AwyOc/aTJgI4a56/IHcxaLcF3TWQV83VcDnR0oGdXNWug==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB6822
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.545,FMLib:17.11.122.1
 definitions=2022-10-24_05,2022-10-21_01,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 suspectscore=0
 mlxlogscore=999 malwarescore=0 spamscore=0 bulkscore=0 mlxscore=0
 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2209130000 definitions=main-2210240103
X-Proofpoint-GUID: V2JLmKgJYlF41VFNNs4_cHS6IqYL9EhV
X-Proofpoint-ORIG-GUID: V2JLmKgJYlF41VFNNs4_cHS6IqYL9EhV
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_H2,SPF_HELO_NONE,SPF_NONE 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 Oct 24, 2022, at 8:16 AM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Sat, 2022-10-22 at 11:09 -0700, Dai Ngo wrote:
>> There is only one nfsd4_callback, cl_recall_any, added for each
>> nfs4_client. Access to it must be serialized. For now it's done
>> by the cl_recall_any_busy flag since it's used only by the
>> delegation shrinker. If there is another consumer of CB_RECALL_ANY
>> then a spinlock must be used.
>>=20
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4callback.c | 64 ++++++++++++++++++++++++++++++++++++++++++++=
++++++
>> fs/nfsd/nfs4state.c    | 27 +++++++++++++++++++++
>> fs/nfsd/state.h        |  8 +++++++
>> fs/nfsd/xdr4cb.h       |  6 +++++
>> 4 files changed, 105 insertions(+)
>>=20
>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>> index f0e69edf5f0f..03587e1397f4 100644
>> --- a/fs/nfsd/nfs4callback.c
>> +++ b/fs/nfsd/nfs4callback.c
>> @@ -329,6 +329,29 @@ static void encode_cb_recall4args(struct xdr_stream=
 *xdr,
>> }
>>=20
>> /*
>> + * CB_RECALLANY4args
>> + *
>> + *	struct CB_RECALLANY4args {
>> + *		uint32_t	craa_objects_to_keep;
>> + *		bitmap4		craa_type_mask;
>> + *	};
>> + */
>> +static void
>> +encode_cb_recallany4args(struct xdr_stream *xdr,
>> +			struct nfs4_cb_compound_hdr *hdr, uint32_t bmval)
>> +{
>> +	__be32 *p;
>> +
>> +	encode_nfs_cb_opnum4(xdr, OP_CB_RECALL_ANY);
>> +	p =3D xdr_reserve_space(xdr, 4);
>> +	*p++ =3D xdr_zero;	/* craa_objects_to_keep */
>> +	p =3D xdr_reserve_space(xdr, 8);
>> +	*p++ =3D cpu_to_be32(1);
>> +	*p++ =3D cpu_to_be32(bmval);
>> +	hdr->nops++;
>> +}
>> +
>> +/*
>>  * CB_SEQUENCE4args
>>  *
>>  *	struct CB_SEQUENCE4args {
>> @@ -482,6 +505,24 @@ static void nfs4_xdr_enc_cb_recall(struct rpc_rqst =
*req, struct xdr_stream *xdr,
>> 	encode_cb_nops(&hdr);
>> }
>>=20
>> +/*
>> + * 20.6. Operation 8: CB_RECALL_ANY - Keep Any N Recallable Objects
>> + */
>> +static void
>> +nfs4_xdr_enc_cb_recall_any(struct rpc_rqst *req,
>> +		struct xdr_stream *xdr, const void *data)
>> +{
>> +	const struct nfsd4_callback *cb =3D data;
>> +	struct nfs4_cb_compound_hdr hdr =3D {
>> +		.ident =3D cb->cb_clp->cl_cb_ident,
>> +		.minorversion =3D cb->cb_clp->cl_minorversion,
>> +	};
>> +
>> +	encode_cb_compound4args(xdr, &hdr);
>> +	encode_cb_sequence4args(xdr, cb, &hdr);
>> +	encode_cb_recallany4args(xdr, &hdr, cb->cb_clp->cl_recall_any_bm);
>> +	encode_cb_nops(&hdr);
>> +}
>>=20
>> /*
>>  * NFSv4.0 and NFSv4.1 XDR decode functions
>> @@ -520,6 +561,28 @@ static int nfs4_xdr_dec_cb_recall(struct rpc_rqst *=
rqstp,
>> 	return decode_cb_op_status(xdr, OP_CB_RECALL, &cb->cb_status);
>> }
>>=20
>> +/*
>> + * 20.6. Operation 8: CB_RECALL_ANY - Keep Any N Recallable Objects
>> + */
>> +static int
>> +nfs4_xdr_dec_cb_recall_any(struct rpc_rqst *rqstp,
>> +				  struct xdr_stream *xdr,
>> +				  void *data)
>> +{
>> +	struct nfsd4_callback *cb =3D data;
>> +	struct nfs4_cb_compound_hdr hdr;
>> +	int status;
>> +
>> +	status =3D decode_cb_compound4res(xdr, &hdr);
>> +	if (unlikely(status))
>> +		return status;
>> +	status =3D decode_cb_sequence4res(xdr, cb);
>> +	if (unlikely(status || cb->cb_seq_status))
>> +		return status;
>> +	status =3D  decode_cb_op_status(xdr, OP_CB_RECALL_ANY, &cb->cb_status)=
;
>> +	return status;
>> +}
>> +
>> #ifdef CONFIG_NFSD_PNFS
>> /*
>>  * CB_LAYOUTRECALL4args
>> @@ -783,6 +846,7 @@ static const struct rpc_procinfo nfs4_cb_procedures[=
] =3D {
>> #endif
>> 	PROC(CB_NOTIFY_LOCK,	COMPOUND,	cb_notify_lock,	cb_notify_lock),
>> 	PROC(CB_OFFLOAD,	COMPOUND,	cb_offload,	cb_offload),
>> +	PROC(CB_RECALL_ANY,	COMPOUND,	cb_recall_any,	cb_recall_any),
>> };
>>=20
>> static unsigned int nfs4_cb_counts[ARRAY_SIZE(nfs4_cb_procedures)];
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 4e718500a00c..c60c937dece6 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -2854,6 +2854,31 @@ static const struct tree_descr client_files[] =3D=
 {
>> 	[3] =3D {""},
>> };
>>=20
>> +static int
>> +nfsd4_cb_recall_any_done(struct nfsd4_callback *cb,
>> +			struct rpc_task *task)
>> +{
>> +	switch (task->tk_status) {
>> +	case -NFS4ERR_DELAY:
>> +		rpc_delay(task, 2 * HZ);
>> +		return 0;
>> +	default:
>> +		return 1;
>> +	}
>> +}
>> +
>> +static void
>> +nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
>> +{
>> +	cb->cb_clp->cl_recall_any_busy =3D false;
>> +	atomic_dec(&cb->cb_clp->cl_rpc_users);
>> +}
>=20
>=20
> This series probably ought to be one big patch. The problem is that
> you're adding a bunch of code to do CB_RECALL_ANY, but there is no way
> to call it without patch #2.

True enough, but there are other potential uses of CB_RECALL_ANY
(and I think Dai is about to add one or two to this series).

I haven't had a chance to look through these yet. I don't mind it
staying split for review, for now. When we get closer to merge,
we can consider squashing if there's still a problem.


> That makes it hard to judge whether there could be races and locking
> issues around the handling of cb_recall_any_busy, in particular. From
> patch #2, it looks like cb_recall_any_busy is protected by the
> nn->client_lock, but I don't think ->release is called with that held.

Then maybe the patches aren't split properly... if there are things
in the second patch that fix issues in the first, then those can be
moved as appropriate.

Adding the new XDR pieces in a separate patch makes sense to me, but
the other parts might be moved to 2/2, for instance.


> Also, cl_rpc_users is a refcount (though we don't necessarily free the
> object when it goes to zero). I think you need to call
> put_client_renew_locked here instead of just decrementing the counter.
>=20
>> +
>> +static const struct nfsd4_callback_ops nfsd4_cb_recall_any_ops =3D {
>> +	.done		=3D nfsd4_cb_recall_any_done,
>> +	.release	=3D nfsd4_cb_recall_any_release,
>> +};
>> +
>> static struct nfs4_client *create_client(struct xdr_netobj name,
>> 		struct svc_rqst *rqstp, nfs4_verifier *verf)
>> {
>> @@ -2891,6 +2916,8 @@ static struct nfs4_client *create_client(struct xd=
r_netobj name,
>> 		free_client(clp);
>> 		return NULL;
>> 	}
>> +	nfsd4_init_cb(&clp->cl_recall_any, clp, &nfsd4_cb_recall_any_ops,
>> +			NFSPROC4_CLNT_CB_RECALL_ANY);
>> 	return clp;
>> }
>>=20
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index e2daef3cc003..49ca06169642 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -411,6 +411,10 @@ struct nfs4_client {
>>=20
>> 	unsigned int		cl_state;
>> 	atomic_t		cl_delegs_in_recall;
>> +
>> +	bool			cl_recall_any_busy;
>> +	uint32_t		cl_recall_any_bm;
>> +	struct nfsd4_callback	cl_recall_any;
>> };
>>=20
>> /* struct nfs4_client_reset
>> @@ -639,8 +643,12 @@ enum nfsd4_cb_op {
>> 	NFSPROC4_CLNT_CB_OFFLOAD,
>> 	NFSPROC4_CLNT_CB_SEQUENCE,
>> 	NFSPROC4_CLNT_CB_NOTIFY_LOCK,
>> +	NFSPROC4_CLNT_CB_RECALL_ANY,
>> };
>>=20
>> +#define RCA4_TYPE_MASK_RDATA_DLG	0
>> +#define RCA4_TYPE_MASK_WDATA_DLG	1
>> +
>> /* Returns true iff a is later than b: */
>> static inline bool nfsd4_stateid_generation_after(stateid_t *a, stateid_=
t *b)
>> {
>> diff --git a/fs/nfsd/xdr4cb.h b/fs/nfsd/xdr4cb.h
>> index 547cf07cf4e0..0d39af1b00a0 100644
>> --- a/fs/nfsd/xdr4cb.h
>> +++ b/fs/nfsd/xdr4cb.h
>> @@ -48,3 +48,9 @@
>> #define NFS4_dec_cb_offload_sz		(cb_compound_dec_hdr_sz  +      \
>> 					cb_sequence_dec_sz +            \
>> 					op_dec_sz)
>> +#define NFS4_enc_cb_recall_any_sz	(cb_compound_enc_hdr_sz +       \
>> +					cb_sequence_enc_sz +            \
>> +					1 + 1 + 1)
>> +#define NFS4_dec_cb_recall_any_sz	(cb_compound_dec_hdr_sz  +      \
>> +					cb_sequence_dec_sz +            \
>> +					op_dec_sz)
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever