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;
Content-Type: text/plain;
        charset=us-ascii
Mime-Version: 1.0 (Mac OS X Mail 12.4 \(3445.104.14\))
Subject: Re: [PATCH] nfsd4: a client's own opens needn't prevent delegations
From:   Chuck Lever <chuck.lever@oracle.com>
In-Reply-To: <20200707132805.GA25846@fieldses.org>
Date:   Tue, 7 Jul 2020 10:19:47 -0400
Cc:     Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Content-Transfer-Encoding: quoted-printable
Message-Id: <706D120D-490E-4C59-B2C7-7F5092D2A70E@oracle.com>
References: <20200707132805.GA25846@fieldses.org>
To:     Bruce Fields <bfields@fieldses.org>
Sender: linux-nfs-owner@vger.kernel.org
Precedence: bulk


> On Jul 7, 2020, at 9:28 AM, bfields@fieldses.org wrote:
>=20
> From: "J. Bruce Fields" <bfields@redhat.com>
>=20
> We recently fixed lease breaking so that a client's actions won't =
break
> its own delegations.
>=20
> But we still have an unnecessary self-conflict when granting
> delegations: a client's own write opens will prevent us from handing =
out
> a read delegation even when no other client has the file open for =
write.
>=20
> Fix that by turning off the checks for conflicting opens under
> vfs_setlease, and instead performing those checks in the nfsd code.
>=20
> We don't depend much on locks here: instead we acquire the delegation,
> then check for conflicts, and drop the delegation again if we find =
any.
>=20
> The check beforehand is an optimization of sorts, just to avoid
> acquiring the delegation unnecessarily.  There's a race where the =
first
> check could cause us to deny the delegation when we could have granted
> it.  But, that's OK, delegation grants are optional (and probably not
> even a good idea in that case).
>=20
> Signed-off-by: J. Bruce Fields <bfields@redhat.com>

Applied to my private tree for testing. Thanks!


> ---
> fs/locks.c          |  3 +++
> fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++------------
> 2 files changed, 43 insertions(+), 14 deletions(-)
>=20
> diff --git a/fs/locks.c b/fs/locks.c
> index 7df0f9fa66f4..d5de9039dbd7 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -1807,6 +1807,9 @@ check_conflicting_open(struct file *filp, const =
long 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 bb3d2c32664a..ab5c8857ae5a 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -4922,6 +4922,32 @@ static struct file_lock =
*nfs4_alloc_init_lease(struct 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)
> @@ -4941,9 +4967,12 @@ nfs4_set_delegation(struct nfs4_client *clp, =
struct svc_fh *fh,
>=20
> 	nf =3D find_readable_file(fp);
> 	if (!nf) {
> -		/* We should always have a readable file here */
> -		WARN_ON_ONCE(1);
> -		return ERR_PTR(-EBADF);
> +		/*
> +		 * 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);
> 	}
> 	spin_lock(&state_lock);
> 	spin_lock(&fp->fi_lock);
> @@ -4973,11 +5002,19 @@ nfs4_set_delegation(struct nfs4_client *clp, =
struct 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, NULL);
> 	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);
> @@ -5059,17 +5096,6 @@ nfs4_open_delegation(struct svc_fh *fh, struct =
nfsd4_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.26.2
>=20

--
Chuck Lever