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:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 3/4] nfsd: make nfsd4_run_cb a bool return function
Thread-Topic: [PATCH 3/4] nfsd: make nfsd4_run_cb a bool return function
Thread-Index: AQHY0cZ51eRBkuHk8kWRMF5Zpbx2Nq3x/cEAgAAJLoCAAAHTgA==
Date:   Mon, 26 Sep 2022 18:31:13 +0000
Message-ID: <5D93DF5C-B214-456A-9742-CCC90C8F6802@oracle.com>
References: <20220926163847.47558-1-jlayton@kernel.org>
 <20220926163847.47558-4-jlayton@kernel.org>
 <F3B08B69-F05B-4F83-B407-343884ABC263@oracle.com>
 <a751b036a0a46a42abe8809c340b05c093e3d148.camel@kernel.org>
In-Reply-To: <a751b036a0a46a42abe8809c340b05c093e3d148.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?AtbHFpXBZGT6DBR8CNcEwSTYlnxu8s7VqOqQ3E0ach+mRYnlHiMQkn3M9tuK?=
 =?us-ascii?Q?gHc6nSJxAygmMr1+zsiCWlYnSwoZ4vUHC5e4DYzxQKru85rXqPUoMSGNinpT?=
 =?us-ascii?Q?qVyyJObGHHfFa/L+62qEbL+OTbdwyhIlYxDH7NJZcBGdP1zVRfnNKF41B25d?=
 =?us-ascii?Q?Gr4eTaHdmaza1+1Q7jJFdGneo7unllxqjVvWIEBxf0MM4LrB81bITzumZEJh?=
 =?us-ascii?Q?ALYNGqZF9+ZL+QVLLKL8uYa1AQAN9jHnAtQLj+13tsjjsFnXuZRHYgZdAPmx?=
 =?us-ascii?Q?Uh8r30LMhg/IzDbsAHjk2z5hiwaocX9FW2OtPH6KxRJBOULQF89Phmz+VW+q?=
 =?us-ascii?Q?iLoG/n6wWd+0u+nEAuiZElMoCoIpTrqgH8HMXfWJntnVpZu3aBvX610cYGwo?=
 =?us-ascii?Q?W7egPMh2kTcRDmkmBbxzAvQysh5sKwWllCbfPFKXYGjlJT/2mqMySo7gBfPh?=
 =?us-ascii?Q?fGZEQ0DmC/zXPCeXMt7Od3M9lrc6AjVyOeegotqJkU83zeJT1yDmlsKAgXcY?=
 =?us-ascii?Q?5yu8VxYcJrvPKoh9kN3OoxcaR6zd39YR3l72UCDe+43/0PPr6/jVF8RRZSc2?=
 =?us-ascii?Q?fzd+nJZFRvpaWs9Zm/3B2PJyAn0EChJNcXVcq8QLg30EhqOwp8461T96iC6Y?=
 =?us-ascii?Q?JEYADnnUcOkRjPwMpnZsWZnT6shIbmYFFeg72mTGR0OJJ2l4M+cqJnqCfYLz?=
 =?us-ascii?Q?VSJsxwf1YVxcuJauJE3pwou+egicUQDphoQ3YLhD96jm4yiwXmhu6EAh5QTY?=
 =?us-ascii?Q?YzENn2KijjgB9XAHt1O9AVLhTYfRqE8V6mPibQMtQ2lHv2uzDNgeBJxrMEsv?=
 =?us-ascii?Q?zRdytT2lsqod4+DQjg5haWRdaXS4pakJsC8TFpakmiZ0gHgnRFV7suPHhT5b?=
 =?us-ascii?Q?HLXNGtwkTA4DH0d1tK6AHVHZuoM6xAJEMoOQuhSpcnBtHnTsBU1AVq1paQGQ?=
 =?us-ascii?Q?qVzKv/oclmvm/xR1Gkbi0Md860S/6SVSm8ooqJvDx/URljkz95UzZxkEd4Jx?=
 =?us-ascii?Q?AWTIn5jsyOJRH0yPRmaM6SS+4k0kRzLdS/0uemtVJP82BaQBeRo+dP9F3+Rl?=
 =?us-ascii?Q?lCW4MRCFrQ+/wtADHvVgI6+m2jGnpyxe/IbCOMomXK2Ak2w83gyyqd+cgjCA?=
 =?us-ascii?Q?fNmu2a8qfpuzxOZfk4suL+h6baeJ3fsZIwBamzI/mF1Xfcutg81HWKuzCANj?=
 =?us-ascii?Q?NJK3V4FVEPPsFmActnXpVMZjoNg4LkTM6p33Z2V5nK6tDtaIbjfSYyUKuszB?=
 =?us-ascii?Q?auMeeSd8pcrUFPXoTO0hu3cu5qfoXckZ6dp6jXsKWvOyH7+/xqGACLR3/chD?=
 =?us-ascii?Q?JXstCCXuSrZ9fxAkHxYmQ8vJJDcqhHqeHp1ugoDERxayQRKMLxmx0I8pfh/M?=
 =?us-ascii?Q?K/V8lkcnudlOevvbL1BvK9vBKxFt/G8BDpPtfydW2DDdhhW2MdDKT8u3yncq?=
 =?us-ascii?Q?6I9wFhYjwQsi3/6fbNK/wWtW/eP8S2q0DKbLkAq6zTlUC1ZbTBAoDuaH9qGi?=
 =?us-ascii?Q?VemiPvWOvQ7J86LQVde5pv37PGgzA6AnCVtwrIQdwT6K9j0guMDcd0IwB6nr?=
 =?us-ascii?Q?+FgbGe1lYpQC9P9o8OjGRJFGwy+80W3S+FwJPQatGToodt3f4e4qp5om7AKI?=
 =?us-ascii?Q?Ag=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <EA3C64B6CF8EE44C94367D67D347F469@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: b06c8f1d-0b11-45bf-08b2-08da9fed4ab7
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Sep 2022 18:31:13.5452
 (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: P/lC8FUmk9Vtj3bon61BAA/fGbxQk2qFvDZRE0fRdPmU3yZgTnBIPvwXvWzBlQKv1PJKY+UHFQXP5gMe7m3EXw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR10MB6017
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.895,Hydra:6.0.528,FMLib:17.11.122.1
 definitions=2022-09-26_09,2022-09-22_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 mlxlogscore=999 bulkscore=0 mlxscore=0
 suspectscore=0 malwarescore=0 adultscore=0 spamscore=0 phishscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2209130000
 definitions=main-2209260115
X-Proofpoint-GUID: HaOB1_yNmjUFB_4yKawCj1aXXSCWTv8o
X-Proofpoint-ORIG-GUID: HaOB1_yNmjUFB_4yKawCj1aXXSCWTv8o
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 Sep 26, 2022, at 2:24 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> On Mon, 2022-09-26 at 17:51 +0000, Chuck Lever III wrote:
>>=20
>>> On Sep 26, 2022, at 12:38 PM, Jeff Layton <jlayton@kernel.org> wrote:
>>>=20
>>> ...and mirror the semantics of queue_work. Also, queueing a delegation
>>> recall should always succeed when queueing, so WARN if one ever doesn't=
.
>>=20
>> The description is not especially clear. It seems like the point
>> of this patch is really the part after the "Also, ..." Otherwise,
>> I'm not getting why this change is really needed?
>>=20
>> Maybe a tracepoint would be less alarming to users/administrators
>> than would a WARN with a full stack trace? The kdoc comment you
>> added (thank you!) suggests there are times when it is OK for the
>> nfsd4_queue_cb() to fail.
>>=20
>>=20
>=20
> Yeah, that's pretty much the case with the "also". There may be some
> other cases where we ought to catch this sort of thing too. We'd likely
> never see a tracepoint for this. We'd just notice that there was a
> refcount leak.
>=20
> queue_work can return false without queueing anything if the work is
> already queued. In this case, we will have taken an extra reference to
> the stid that will never get put. I think this should never happen
> because of the flc_lock, but it'd be good to catch it if it does.

I've pushed Dai's v3 SSC fix and the first two patches in this series
to the nfsd for-next tree. Can you send me 3/4 and 4/4 again but=20

 - Include the above text in the description of 3/4, and

 - rebase 4/4 on top of the latest nfsd for-next

Thanks!


> That said, I don't feel that strongly about the patch, so if you think
> it's not worthwhile, I'm fine with holding off on it.
>=20
>>> Signed-off-by: Jeff Layton <jlayton@kernel.org>
>>> ---
>>> fs/nfsd/nfs4callback.c | 14 ++++++++++++--
>>> fs/nfsd/nfs4state.c    |  5 ++---
>>> fs/nfsd/state.h        |  2 +-
>>> 3 files changed, 15 insertions(+), 6 deletions(-)
>>>=20
>>> diff --git a/fs/nfsd/nfs4callback.c b/fs/nfsd/nfs4callback.c
>>> index 4ce328209f61..ba904614ebf5 100644
>>> --- a/fs/nfsd/nfs4callback.c
>>> +++ b/fs/nfsd/nfs4callback.c
>>> @@ -1371,11 +1371,21 @@ void nfsd4_init_cb(struct nfsd4_callback *cb, s=
truct nfs4_client *clp,
>>> 	cb->cb_holds_slot =3D false;
>>> }
>>>=20
>>> -void nfsd4_run_cb(struct nfsd4_callback *cb)
>>> +/**
>>> + * nfsd4_run_cb - queue up a callback job to run
>>> + * @cb: callback to queue
>>> + *
>>> + * Kick off a callback to do its thing. Returns false if it was alread=
y
>>> + * queued or running, true otherwise.
>>> + */
>>> +bool nfsd4_run_cb(struct nfsd4_callback *cb)
>>> {
>>> +	bool queued;
>>> 	struct nfs4_client *clp =3D cb->cb_clp;
>>=20
>> Reverse christmas tree, please.
>>=20
>>=20
>>>=20
>>> 	nfsd41_cb_inflight_begin(clp);
>>> -	if (!nfsd4_queue_cb(cb))
>>> +	queued =3D nfsd4_queue_cb(cb);
>>> +	if (!queued)
>>> 		nfsd41_cb_inflight_end(clp);
>>> +	return queued;
>>> }
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 211f1af1cfb3..90533f43fea9 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -4861,14 +4861,13 @@ static void nfsd_break_one_deleg(struct nfs4_de=
legation *dp)
>>> 	 * we know it's safe to take a reference.
>>> 	 */
>>> 	refcount_inc(&dp->dl_stid.sc_count);
>>> -	nfsd4_run_cb(&dp->dl_recall);
>>> +	WARN_ON_ONCE(!nfsd4_run_cb(&dp->dl_recall));
>>> }
>>>=20
>>> /* Called from break_lease() with flc_lock held. */
>>> static bool
>>> nfsd_break_deleg_cb(struct file_lock *fl)
>>> {
>>> -	bool ret =3D false;
>>> 	struct nfs4_delegation *dp =3D (struct nfs4_delegation *)fl->fl_owner;
>>> 	struct nfs4_file *fp =3D dp->dl_stid.sc_file;
>>> 	struct nfs4_client *clp =3D dp->dl_stid.sc_client;
>>> @@ -4894,7 +4893,7 @@ nfsd_break_deleg_cb(struct file_lock *fl)
>>> 	fp->fi_had_conflict =3D true;
>>> 	nfsd_break_one_deleg(dp);
>>> 	spin_unlock(&fp->fi_lock);
>>> -	return ret;
>>> +	return false;
>>> }
>>>=20
>>> /**
>>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>>> index b3477087a9fc..e2daef3cc003 100644
>>> --- a/fs/nfsd/state.h
>>> +++ b/fs/nfsd/state.h
>>> @@ -692,7 +692,7 @@ extern void nfsd4_probe_callback_sync(struct nfs4_c=
lient *clp);
>>> extern void nfsd4_change_callback(struct nfs4_client *clp, struct nfs4_=
cb_conn *);
>>> extern void nfsd4_init_cb(struct nfsd4_callback *cb, struct nfs4_client=
 *clp,
>>> 		const struct nfsd4_callback_ops *ops, enum nfsd4_cb_op op);
>>> -extern void nfsd4_run_cb(struct nfsd4_callback *cb);
>>> +extern bool nfsd4_run_cb(struct nfsd4_callback *cb);
>>> extern int nfsd4_create_callback_queue(void);
>>> extern void nfsd4_destroy_callback_queue(void);
>>> extern void nfsd4_shutdown_callback(struct nfs4_client *);
>>> --=20
>>> 2.37.3
>>>=20
>>=20
>> --
>> Chuck Lever
>>=20
>>=20
>>=20
>=20
> --=20
> Jeff Layton <jlayton@kernel.org>

--
Chuck Lever