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>,
        Olga Kornievskaia <kolga@netapp.com>
Subject: Re: [PATCH 3/3] nfsd: eliminate the NFSD_FILE_BREAK_* flags
Thread-Topic: [PATCH 3/3] nfsd: eliminate the NFSD_FILE_BREAK_* flags
Thread-Index: AQHYo2rgY8a6WWViM0iW0L8RSDO/l62VmE8A
Date:   Fri, 29 Jul 2022 17:21:01 +0000
Message-ID: <5B5182C2-2B5D-4863-A6A4-8F3A6098A9AC@oracle.com>
References: <20220729164715.75702-1-jlayton@kernel.org>
 <20220729164715.75702-3-jlayton@kernel.org>
In-Reply-To: <20220729164715.75702-3-jlayton@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?n4Y9vwep3KbNpSlrKz9Mz8m2gbBJSJsUX3vXIgnugcrj38G4X5/AiaKrB3xn?=
 =?us-ascii?Q?CtU2G3LDRG85eW37bLkTmSB6/rGI1uweSAJYGkX6/xGcuWw1HdgLn0Fvvk/Y?=
 =?us-ascii?Q?h9i8TFafmzL/FO6z3kBBMUq2go/CSa22Plnr8fJEIlGGQ2GHSBrJFS6Lpgm4?=
 =?us-ascii?Q?HkPwYjdhWAVlClKF1Uzd/ZE5m5pjTyeEWGK43r6ifhQh8bzGv7k8QtrAI1f4?=
 =?us-ascii?Q?LCmM3gEytTgBaHjD4FeBe9FwftFvLD6NaDxp8VKkyvutQ/esw2BrCVGHznbK?=
 =?us-ascii?Q?Sfpz+ieVlSO7D3obpBgQGylh6nVolqQsdEX0EwVzntGUuwVfkuLBQ77+XnDR?=
 =?us-ascii?Q?ac8Qe67ga9HhPZHRv9S74qCG39ql0laRc6TZA7ER5YMPyy6/gZv9K5wjOtCJ?=
 =?us-ascii?Q?pODY4BrG6jx/Q/pvADIhvmuEA8WxQsknqHIsyZe0SJysNIGG2ur9fWfRlseL?=
 =?us-ascii?Q?sJayuYXL3cjEZXzyQj0tuJaOKHcohDGTIYrIrOfOq/enaBfgK9JdtajO1zPz?=
 =?us-ascii?Q?E7gBHtpW6zEFLvA5cSmQnQ72jHKPnQlUVEgzvZLn0QCYQMpsH3f/pfd9qsk4?=
 =?us-ascii?Q?ss88SSDhEd7km6CeWvdeKHf84vS+sXyQfWNb5w/iT6Bh3m7folQI+3O8lqND?=
 =?us-ascii?Q?TY3sV5fh7G/4YkpPvAHaX3yxRlO0869RlNhn5W7doAKhQXhfM+Y1PSWom6dP?=
 =?us-ascii?Q?6NFlmtkXssHYideqioQ28G7HXMq3SSBtjsaimA7Ly4eK+PD1ZBeICSSOBTb+?=
 =?us-ascii?Q?Fj9TND+ozrDr9Izw4dk0r8weExgkpXGyhzZjWEndB3RamUc8pQsERABQb9pd?=
 =?us-ascii?Q?ELuZfbL4PPyA6jjv7Rz0uoY7/Nmwvly+PGU1aTJT0KBQ9qR9FQYa0zL/EI1v?=
 =?us-ascii?Q?b1L9qZhRQhliq5MJzUfQOxYOt6MA34ztRMU489FHk9FRHVnAutfrBHJg5KkW?=
 =?us-ascii?Q?T9nIs0gAsfNRwXv3HsB//OeOub5PNmTCGA5czf9U16ISCYTqKPQY5mn3uJQv?=
 =?us-ascii?Q?GBymHvylSzdjAE2ItjGYWnu/MxMSFo7q0bVTSPAZ7uqPFjic1VwwO9oARr+H?=
 =?us-ascii?Q?uqZjG8IT3tDiNCr7/8PZExLWZcUwrRALMBBQyoP/SmJGqDHJl9eFqN7KK8bp?=
 =?us-ascii?Q?L1OIAcPNBA75mRV/zser8Js7LKsixMzgJH26XiujuUva4/DGk5qP1Emh2jrB?=
 =?us-ascii?Q?BfAaRWesC6EVWTR90kczi+InKXoMljn2yuYlWNxI9aEu4yJseyhiUuXbXOH/?=
 =?us-ascii?Q?npV5rr7PtejzFVLx0IGtPOiA9L2BgfF02pi1OKopG9FKtRuK4Ny7XEs5Q7i9?=
 =?us-ascii?Q?i8GvUojzfoFiA4vYULMX+qifd2AQwrH09+WqDBGLiPH+zGc2/DZLS5SSqcoN?=
 =?us-ascii?Q?B77vvGg3ntOp/G0cQxgiyweyOsHJGXctyyTh5IEmBc/Bdchrox40Uc+Ku/cB?=
 =?us-ascii?Q?J6TLnGkICD0ngDpZufYYG58u0l5BczIXkILAa6Uj2p2AEL6T5jkWbQiiIzgA?=
 =?us-ascii?Q?XZeNgTAbliR4ozagsjxV0G8+7IWVfIy3nhDxY7ZA23+w3EMpVyOapFH6Axbn?=
 =?us-ascii?Q?WbxT0mce/lYy8mbqns+6hE/GhrbgEq7T7a72XIOh9AqFoy57gh/DhdLp5fJz?=
 =?us-ascii?Q?CQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <4B3F58EC8C50D0439D4852610D56085B@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: a2c43f78-f585-4094-cfd2-08da7186b5a8
X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Jul 2022 17:21:01.3017
 (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: 2D02T4xhxVuTpUz5dFCaxoze0TEsok5i98eZFVJHhHE2X3AdmzkXQGIXAQwwsG8Mx25J+SA0WaWN+AIwzpKakg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR10MB1313
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-07-29_18,2022-07-28_02,2022-06-22_01
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 bulkscore=0 malwarescore=0
 adultscore=0 mlxscore=0 mlxlogscore=999 suspectscore=0 spamscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2206140000
 definitions=main-2207290075
X-Proofpoint-ORIG-GUID: m5HcEIlHtBCrRwbCSo30bhOZv14V6iNj
X-Proofpoint-GUID: m5HcEIlHtBCrRwbCSo30bhOZv14V6iNj
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 Jul 29, 2022, at 12:47 PM, Jeff Layton <jlayton@kernel.org> wrote:
>=20
> We had a report from the spring Bake-a-thon of data corruption in some
> nfstest_interop tests. Looking at the traces showed the NFS server
> allowing a v3 WRITE to proceed while a read delegation was still
> outstanding.
>=20
> Currently, we only set NFSD_FILE_BREAK_* flags if
> NFSD_MAY_NOT_BREAK_LEASE was set when we call nfsd_file_alloc.
> NFSD_MAY_NOT_BREAK_LEASE was intended to be set when finding files for
> COMMIT ops, where we need a writeable filehandle but don't need to
> break read leases.
>=20
> It doesn't make any sense to consult that flag when allocating a file
> since the file may be used on subsequent calls where we do want to break
> the lease (and the usage of it here seems to be reverse from what it
> should be anyway).
>=20
> Also, after calling nfsd_open_break_lease, we don't want to clear the
> BREAK_* bits. A lease could end up being set on it later (more than
> once) and we need to be able to break those leases as well.
>=20
> This means that the NFSD_FILE_BREAK_* flags now just mirror
> NFSD_MAY_{READ,WRITE} flags, so there's no need for them at all. Just
> drop those flags and unconditionally call nfsd_open_break_lease every
> time.
>=20
> Link: https://bugzilla.redhat.com/show_bug.cgi?id=3D2107360
> Fixes: 65294c1f2c5e (nfsd: add a new struct file caching facility to nfsd=
)
> Reported-by: Olga Kornieskaia <kolga@netapp.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>

I'm going to go out on a limb and predict this will conflict
heavily with the filecache overhaul patches I have queued for
next. :-)

Do you believe this is something that urgently needs to be
backported to stable kernels, or can it be rebased on top of
the filecache overhaul work?


> ---
> fs/nfsd/filecache.c | 26 +++-----------------------
> fs/nfsd/filecache.h |  4 +---
> fs/nfsd/trace.h     |  2 --
> 3 files changed, 4 insertions(+), 28 deletions(-)
>=20
> diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> index 4758c2a3fcf8..7e566ddca388 100644
> --- a/fs/nfsd/filecache.c
> +++ b/fs/nfsd/filecache.c
> @@ -283,7 +283,7 @@ nfsd_file_mark_find_or_create(struct nfsd_file *nf, s=
truct inode *inode)
> }
>=20
> static struct nfsd_file *
> -nfsd_file_alloc(struct nfsd_file_lookup_key *key, unsigned int may)
> +nfsd_file_alloc(struct nfsd_file_lookup_key *key)
> {
> 	struct nfsd_file *nf;
>=20
> @@ -301,12 +301,6 @@ nfsd_file_alloc(struct nfsd_file_lookup_key *key, un=
signed int may)
> 		/* nf_ref is pre-incremented for hash table */
> 		refcount_set(&nf->nf_ref, 2);
> 		nf->nf_may =3D key->need;
> -		if (may & NFSD_MAY_NOT_BREAK_LEASE) {
> -			if (may & NFSD_MAY_WRITE)
> -				__set_bit(NFSD_FILE_BREAK_WRITE, &nf->nf_flags);
> -			if (may & NFSD_MAY_READ)
> -				__set_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags);
> -		}
> 		nf->nf_mark =3D NULL;
> 	}
> 	return nf;
> @@ -1090,7 +1084,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struct=
 svc_fh *fhp,
> 	if (nf)
> 		goto wait_for_construction;
>=20
> -	new =3D nfsd_file_alloc(&key, may_flags);
> +	new =3D nfsd_file_alloc(&key);
> 	if (!new) {
> 		status =3D nfserr_jukebox;
> 		goto out_status;
> @@ -1130,21 +1124,7 @@ nfsd_file_do_acquire(struct svc_rqst *rqstp, struc=
t svc_fh *fhp,
> 	nfsd_file_lru_remove(nf);
> 	this_cpu_inc(nfsd_file_cache_hits);
>=20
> -	if (!(may_flags & NFSD_MAY_NOT_BREAK_LEASE)) {
> -		bool write =3D (may_flags & NFSD_MAY_WRITE);
> -
> -		if (test_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags) ||
> -		    (test_bit(NFSD_FILE_BREAK_WRITE, &nf->nf_flags) && write)) {
> -			status =3D nfserrno(nfsd_open_break_lease(
> -					file_inode(nf->nf_file), may_flags));
> -			if (status =3D=3D nfs_ok) {
> -				clear_bit(NFSD_FILE_BREAK_READ, &nf->nf_flags);
> -				if (write)
> -					clear_bit(NFSD_FILE_BREAK_WRITE,
> -						  &nf->nf_flags);
> -			}
> -		}
> -	}
> +	status =3D nfserrno(nfsd_open_break_lease(file_inode(nf->nf_file), may_=
flags));
> out:
> 	if (status =3D=3D nfs_ok) {
> 		if (open)
> diff --git a/fs/nfsd/filecache.h b/fs/nfsd/filecache.h
> index d534b76cb65b..8e8c0c47d67d 100644
> --- a/fs/nfsd/filecache.h
> +++ b/fs/nfsd/filecache.h
> @@ -37,9 +37,7 @@ struct nfsd_file {
> 	struct net		*nf_net;
> #define NFSD_FILE_HASHED	(0)
> #define NFSD_FILE_PENDING	(1)
> -#define NFSD_FILE_BREAK_READ	(2)
> -#define NFSD_FILE_BREAK_WRITE	(3)
> -#define NFSD_FILE_REFERENCED	(4)
> +#define NFSD_FILE_REFERENCED	(2)
> 	unsigned long		nf_flags;
> 	struct inode		*nf_inode;	/* don't deref */
> 	refcount_t		nf_ref;
> diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> index e9c5d0f56977..2bd867a96eba 100644
> --- a/fs/nfsd/trace.h
> +++ b/fs/nfsd/trace.h
> @@ -758,8 +758,6 @@ DEFINE_CLID_EVENT(confirmed_r);
> 	__print_flags(val, "|",						\
> 		{ 1 << NFSD_FILE_HASHED,	"HASHED" },		\
> 		{ 1 << NFSD_FILE_PENDING,	"PENDING" },		\
> -		{ 1 << NFSD_FILE_BREAK_READ,	"BREAK_READ" },		\
> -		{ 1 << NFSD_FILE_BREAK_WRITE,	"BREAK_WRITE" },	\
> 		{ 1 << NFSD_FILE_REFERENCED,	"REFERENCED"})
>=20
> DECLARE_EVENT_CLASS(nfsd_file_class,
> --=20
> 2.37.1
>=20

--
Chuck Lever