Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Chuck Lever <chuck.lever@oracle.com>
To:     Bruce Fields <bfields@fieldses.org>
CC:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH 2/2] Revert "nfsd4: a client's own opens needn't prevent
 delegations"
Thread-Topic: [PATCH 2/2] Revert "nfsd4: a client's own opens needn't prevent
 delegations"
Thread-Index: AQHXFDMNQbn31a+2PUeMNtliARrSqKp6PjWA
Date:   Mon, 8 Mar 2021 15:54:21 +0000
Message-ID: <7BC3CBA0-36AF-4F51-A5E4-ED0A1FABF16F@oracle.com>
References: <20210308155151.GC7284@fieldses.org>
 <20210308155229.GD7284@fieldses.org>
In-Reply-To: <20210308155229.GD7284@fieldses.org>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-ID: <FB8DDE1592F2BE45956C522022144030@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c9374a85-e356-457b-7655-08d8e24a70a2
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Mar 2021 15:54:21.7420
 (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: huIJbziElcEY6+Y/cQWIgxRwGkCa4OY0m7UTeo9HsmN9PO12Eis96OaZpez60Zv8GG+Q4vcqlOSTCIn6nYLLUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR10MB4049
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9917 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 adultscore=0
 malwarescore=0 bulkscore=0 suspectscore=0 mlxscore=0 mlxlogscore=999
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2009150000 definitions=main-2103080086
X-Proofpoint-Virus-Version: vendor=nai engine=6200 definitions=9917 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 suspectscore=0
 clxscore=1015 phishscore=0 adultscore=0 mlxlogscore=999 priorityscore=1501
 lowpriorityscore=0 bulkscore=0 mlxscore=0 impostorscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2009150000
 definitions=main-2103080086
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org


> On Mar 8, 2021, at 10:52 AM, J. Bruce Fields <bfields@fieldses.org> wrote=
:
>=20
> From: "J. Bruce Fields" <bfields@redhat.com>
>=20
> This reverts commit 94415b06eb8aed13481646026dc995f04a3a534a.
>=20
> That commit claimed to allow a client to get a read delegation when it
> was the only writer.  Actually it allowed a client to get a read
> delegation when *any* client has a write open!
>=20
> The main problem is that it's depending on nfs4_clnt_odstate structures
> that are actually only maintained for pnfs exports.
>=20
> This causes clients to miss writes performed by other clients, even when
> there have been intervening closes and opens, violating close-to-open
> cache consistency.
>=20
> We can do this a different way, but first we should just revert this.
>=20
> I've added pynfs 4.1 test DELEG19 to test for this, as I should have
> done originally!

Excellent, Bruce. I'll get these into for-rc right away so it can get
some testing with the 0-day infrastructure.


> Cc: stable@vger.kernel.org
> Reported-by: Timo Rothenpieler <timo@rothenpieler.org>
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> ---
> fs/locks.c          |  3 ---
> fs/nfsd/nfs4state.c | 54 ++++++++++++---------------------------------
> 2 files changed, 14 insertions(+), 43 deletions(-)
>=20
> diff --git a/fs/locks.c b/fs/locks.c
> index 99ca97e81b7a..6125d2de39b8 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1808,9 +1808,6 @@ check_conflicting_open(struct file *filp, const lon=
g arg, int flags)
>=20
> 	if (flags & FL_LAYOUT)
> 		return 0;
> -	if (flags & FL_DELEG)
> -		/* We leave these checks to the caller. */
> -		return 0;
>=20
> 	if (arg =3D=3D F_RDLCK)
> 		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 408e2c4db926..27983a50435e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4940,32 +4940,6 @@ static struct file_lock *nfs4_alloc_init_lease(str=
uct nfs4_delegation *dp,
> 	return fl;
> }
>=20
> -static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
> -						struct nfs4_file *fp)
> -{
> -	struct nfs4_clnt_odstate *co;
> -	struct file *f =3D fp->fi_deleg_file->nf_file;
> -	struct inode *ino =3D locks_inode(f);
> -	int writes =3D atomic_read(&ino->i_writecount);
> -
> -	if (fp->fi_fds[O_WRONLY])
> -		writes--;
> -	if (fp->fi_fds[O_RDWR])
> -		writes--;
> -	WARN_ON_ONCE(writes < 0);
> -	if (writes > 0)
> -		return -EAGAIN;
> -	spin_lock(&fp->fi_lock);
> -	list_for_each_entry(co, &fp->fi_clnt_odstate, co_perfile) {
> -		if (co->co_client !=3D clp) {
> -			spin_unlock(&fp->fi_lock);
> -			return -EAGAIN;
> -		}
> -	}
> -	spin_unlock(&fp->fi_lock);
> -	return 0;
> -}
> -
> static struct nfs4_delegation *
> nfs4_set_delegation(struct nfs4_client *clp, struct svc_fh *fh,
> 		    struct nfs4_file *fp, struct nfs4_clnt_odstate *odstate)
> @@ -4985,12 +4959,9 @@ nfs4_set_delegation(struct nfs4_client *clp, struc=
t svc_fh *fh,
>=20
> 	nf =3D find_readable_file(fp);
> 	if (!nf) {
> -		/*
> -		 * We probably could attempt another open and get a read
> -		 * delegation, but for now, don't bother until the
> -		 * client actually sends us one.
> -		 */
> -		return ERR_PTR(-EAGAIN);
> +		/* We should always have a readable file here */
> +		WARN_ON_ONCE(1);
> +		return ERR_PTR(-EBADF);
> 	}
> 	spin_lock(&state_lock);
> 	spin_lock(&fp->fi_lock);
> @@ -5020,19 +4991,11 @@ nfs4_set_delegation(struct nfs4_client *clp, stru=
ct svc_fh *fh,
> 	if (!fl)
> 		goto out_clnt_odstate;
>=20
> -	status =3D nfsd4_check_conflicting_opens(clp, fp);
> -	if (status) {
> -		locks_free_lock(fl);
> -		goto out_clnt_odstate;
> -	}
> 	status =3D vfs_setlease(fp->fi_deleg_file->nf_file, fl->fl_type, &fl, NU=
LL);
> 	if (fl)
> 		locks_free_lock(fl);
> 	if (status)
> 		goto out_clnt_odstate;
> -	status =3D nfsd4_check_conflicting_opens(clp, fp);
> -	if (status)
> -		goto out_clnt_odstate;
>=20
> 	spin_lock(&state_lock);
> 	spin_lock(&fp->fi_lock);
> @@ -5114,6 +5077,17 @@ nfs4_open_delegation(struct svc_fh *fh, struct nfs=
d4_open *open,
> 				goto out_no_deleg;
> 			if (!cb_up || !(oo->oo_flags & NFS4_OO_CONFIRMED))
> 				goto out_no_deleg;
> +			/*
> +			 * Also, if the file was opened for write or
> +			 * create, there's a good chance the client's
> +			 * about to write to it, resulting in an
> +			 * immediate recall (since we don't support
> +			 * write delegations):
> +			 */
> +			if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE)
> +				goto out_no_deleg;
> +			if (open->op_create =3D=3D NFS4_OPEN_CREATE)
> +				goto out_no_deleg;
> 			break;
> 		default:
> 			goto out_no_deleg;
> --=20
> 2.29.2
>=20

--
Chuck Lever