diff mbox series

nfsd4: a client's own opens needn't prevent delegations

Message ID 20200707132805.GA25846@fieldses.org (mailing list archive)
State New, archived
Headers show
Series nfsd4: a client's own opens needn't prevent delegations | expand

Commit Message

J. Bruce Fields July 7, 2020, 1:28 p.m. UTC 
From: "J. Bruce Fields" <bfields@redhat.com>

We recently fixed lease breaking so that a client's actions won't break
its own delegations.

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.

Fix that by turning off the checks for conflicting opens under
vfs_setlease, and instead performing those checks in the nfsd code.

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.

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).

Signed-off-by: J. Bruce Fields <bfields@redhat.com>
---
 fs/locks.c          |  3 +++
 fs/nfsd/nfs4state.c | 54 +++++++++++++++++++++++++++++++++------------
 2 files changed, 43 insertions(+), 14 deletions(-)

Comments

J. Bruce Fields July 7, 2020, 2:11 p.m. UTC | #1 
On Tue, Jul 07, 2020 at 09:28:05AM -0400, bfields wrote:
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We recently fixed lease breaking so that a client's actions won't break
> its own delegations.
> 
> 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.
> 
> Fix that by turning off the checks for conflicting opens under
> vfs_setlease, and instead performing those checks in the nfsd code.

I'm testing this with pynfs.

--b.

commit f9f2b68d2f17
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Tue Jul 7 10:03:24 2020 -0400

    DELEG22: grant delegs on write opens too
    
    Even when the client has write opens, the server can still grant
    delegations.
    
    (Didn't add the "all" flag since this behavior is optional.  Then again,
    so is most delegation-granting behavior.  Some day we should think about
    how to report optional behavior so that we can still for example
    identify regressions in a given implementation even when the behavior
    we're regressing to isn't strictly incorrect.)
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/nfs4.0/servertests/st_delegation.py b/nfs4.0/servertests/st_delegation.py
index e7ebcc5a2c67..875dbc94ded1 100644
--- a/nfs4.0/servertests/st_delegation.py
+++ b/nfs4.0/servertests/st_delegation.py
@@ -745,3 +745,32 @@ def testServerSelfConflict(t, env):
     newcount = c.cb_server.opcounts[OP_CB_RECALL]
     if newcount > count:
         t.fail("Unnecessary delegation recall")
+
+def testServerSelfConflict2(t,env):
+    """DELEGATION test
+
+    Test that we can still get a delegation even when we have the
+    file open for write from the same client.
+
+    FLAGS: delegations
+    CODE: DELEG22
+    """
+    c = env.c1
+    c.init_connection(b'pynfs%i_%s' % (os.getpid(), t.word()), cb_ident=0)
+    time.sleep(0.5)
+    res = c.create_file(t.word(), c.homedir+[t.word()],
+                        access = OPEN4_SHARE_ACCESS_BOTH,
+                        deny = OPEN4_SHARE_DENY_NONE)
+    check(res)
+    fh, stateid = c.confirm(t.word(), res)
+    deleg_info = res.resarray[-2].switch.switch.delegation
+    if deleg_info.delegation_type != OPEN_DELEGATE_NONE:
+        return
+    res = c.open_file(t.word(), c.homedir+[t.word()],
+                        access=OPEN4_SHARE_ACCESS_BOTH,
+                        deny = OPEN4_SHARE_DENY_NONE)
+    check(res)
+    fh, stateid = c.confirm(t.word(), res)
+    deleg_info = res.resarray[-2].switch.switch.delegation
+    if deleg_info.delegation_type == OPEN_DELEGATE_NONE:
+        t.fail("Could not get delegation")
Chuck Lever July 7, 2020, 2:19 p.m. UTC | #2 
> On Jul 7, 2020, at 9:28 AM, bfields@fieldses.org wrote:
> 
> From: "J. Bruce Fields" <bfields@redhat.com>
> 
> We recently fixed lease breaking so that a client's actions won't break
> its own delegations.
> 
> 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.
> 
> Fix that by turning off the checks for conflicting opens under
> vfs_setlease, and instead performing those checks in the nfsd code.
> 
> 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.
> 
> 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).
> 
> 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(-)
> 
> 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)
> 
> 	if (flags & FL_LAYOUT)
> 		return 0;
> +	if (flags & FL_DELEG)
> +		/* We leave these checks to the caller. */
> +		return 0;
> 
> 	if (arg == 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;
> }
> 
> +static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
> +						struct nfs4_file *fp)
> +{
> +	struct nfs4_clnt_odstate *co;
> +	struct file *f = fp->fi_deleg_file->nf_file;
> +	struct inode *ino = locks_inode(f);
> +	int writes = 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 != 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,
> 
> 	nf = 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;
> 
> +	status = nfsd4_check_conflicting_opens(clp, fp);
> +	if (status) {
> +		locks_free_lock(fl);
> +		goto out_clnt_odstate;
> +	}
> 	status = 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 = nfsd4_check_conflicting_opens(clp, fp);
> +	if (status)
> +		goto out_clnt_odstate;
> 
> 	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 == NFS4_OPEN_CREATE)
> -				goto out_no_deleg;
> 			break;
> 		default:
> 			goto out_no_deleg;
> -- 
> 2.26.2
> 

--
Chuck Lever
diff mbox series

Patch

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)
 
 	if (flags & FL_LAYOUT)
 		return 0;
+	if (flags & FL_DELEG)
+		/* We leave these checks to the caller. */
+		return 0;
 
 	if (arg == 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;
 }
 
+static int nfsd4_check_conflicting_opens(struct nfs4_client *clp,
+						struct nfs4_file *fp)
+{
+	struct nfs4_clnt_odstate *co;
+	struct file *f = fp->fi_deleg_file->nf_file;
+	struct inode *ino = locks_inode(f);
+	int writes = 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 != 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,
 
 	nf = 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;
 
+	status = nfsd4_check_conflicting_opens(clp, fp);
+	if (status) {
+		locks_free_lock(fl);
+		goto out_clnt_odstate;
+	}
 	status = 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 = nfsd4_check_conflicting_opens(clp, fp);
+	if (status)
+		goto out_clnt_odstate;
 
 	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 == NFS4_OPEN_CREATE)
-				goto out_no_deleg;
 			break;
 		default:
 			goto out_no_deleg;