delegation patches for 3.14

PATCH In review J. Bruce Fields 2 participants 5 messages Jan 26, 2014 – Jan 27, 2014

Subsystems (2)

NFS client NFS server

lore

Share & tools

lore

J. Bruce Fields

author

2014-01-26 20:59 UTC #0 lore raw

These patches do a little minor cleanup and close a small race left after the vfs delegation patches.

--b.

J. Bruce Fields

author

2014-01-26 20:59 UTC #1 lore raw

PATCH 1/3 nfsd4: minor nfs4_setlease cleanup

From: "J. Bruce Fields" <[email protected]>

As far as I can tell, this list is used only under the state lock, so we may as well do this in the simpler order.

Acked-by Jeff Layton <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4state.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

fs/nfsd/nfs4state.c +6 -6

index 5795d5f..ed3085b 100644
@@ -3043,18 +3043,18 @@ static int nfs4_setlease(struct nfs4_delegation *dp)
 	if (!fl)
 		return -ENOMEM;
 	fl->fl_file = find_readable_file(fp);
-	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 	status = vfs_setlease(fl->fl_file, fl->fl_type, &fl);
-	if (status) {
-		list_del_init(&dp->dl_perclnt);
-		locks_free_lock(fl);
-		return status;
-	}
+	if (status)
+		goto out_free;
+	list_add(&dp->dl_perclnt, &dp->dl_stid.sc_client->cl_delegations);
 	fp->fi_lease = fl;
 	fp->fi_deleg_file = get_file(fl->fl_file);
 	atomic_set(&fp->fi_delegees, 1);
 	list_add(&dp->dl_perfile, &fp->fi_delegations);
 	return 0;
+out_free:
+	locks_free_lock(fl);
+	return status;
 }
 
 static int nfs4_set_delegation(struct nfs4_delegation *dp, struct nfs4_file *fp)

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-01-26 20:59 UTC #2 lore raw

PATCH 2/3 nfsd4: delay setting current_fh in open

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

This is basically a no-op, to simplify a following patch.

Acked-by Jeff Layton <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

fs/nfsd/nfs4proc.c +20 ~~-17~~

index dadff09..844813a 100644
@@ -230,17 +230,16 @@ static void nfsd4_set_open_owner_reply_cache(struct nfsd4_compound_state *cstate
 }
 
 static __be32
-do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open)
+do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, struct nfsd4_open *open, struct svc_fh **resfh)
 {
 	struct svc_fh *current_fh = &cstate->current_fh;
-	struct svc_fh *resfh;
 	int accmode;
 	__be32 status;
 
-	resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
-	if (!resfh)
+	*resfh = kmalloc(sizeof(struct svc_fh), GFP_KERNEL);
+	if (!*resfh)
 		return nfserr_jukebox;
-	fh_init(resfh, NFS4_FHSIZE);
+	fh_init(*resfh, NFS4_FHSIZE);
 	open->op_truncate = 0;
 
 	if (open->op_create) {

@@ -265,12 +264,12 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		 */
 		status = do_nfsd_create(rqstp, current_fh, open->op_fname.data,
 					open->op_fname.len, &open->op_iattr,
-					resfh, open->op_createmode,
+					*resfh, open->op_createmode,
 					(u32 *)open->op_verf.data,
 					&open->op_truncate, &open->op_created);
 
 		if (!status && open->op_label.len)
-			nfsd4_security_inode_setsecctx(resfh, &open->op_label, open->op_bmval);
+			nfsd4_security_inode_setsecctx(*resfh, &open->op_label, open->op_bmval);
 
 		/*
 		 * Following rfc 3530 14.2.16, use the returned bitmask

@@ -282,29 +281,26 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 							FATTR4_WORD1_TIME_MODIFY);
 	} else {
 		status = nfsd_lookup(rqstp, current_fh,
-				     open->op_fname.data, open->op_fname.len, resfh);
+				     open->op_fname.data, open->op_fname.len, *resfh);
 		fh_unlock(current_fh);
 	}
 	if (status)
 		goto out;
-	status = nfsd_check_obj_isreg(resfh);
+	status = nfsd_check_obj_isreg(*resfh);
 	if (status)
 		goto out;
 
 	if (is_create_with_attrs(open) && open->op_acl != NULL)
-		do_set_nfs4_acl(rqstp, resfh, open->op_acl, open->op_bmval);
+		do_set_nfs4_acl(rqstp, *resfh, open->op_acl, open->op_bmval);
 
-	nfsd4_set_open_owner_reply_cache(cstate, open, resfh);
+	nfsd4_set_open_owner_reply_cache(cstate, open, *resfh);
 	accmode = NFSD_MAY_NOP;
 	if (open->op_created ||
 			open->op_claim_type == NFS4_OPEN_CLAIM_DELEGATE_CUR)
 		accmode |= NFSD_MAY_OWNER_OVERRIDE;
-	status = do_open_permission(rqstp, resfh, open, accmode);
+	status = do_open_permission(rqstp, *resfh, open, accmode);
 	set_change_info(&open->op_cinfo, current_fh);
-	fh_dup2(current_fh, resfh);
 out:
-	fh_put(resfh);
-	kfree(resfh);
 	return status;
 }

@@ -357,6 +353,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	   struct nfsd4_open *open)
 {
 	__be32 status;
+	struct svc_fh *resfh = NULL;
 	struct nfsd4_compoundres *resp;
 	struct net *net = SVC_NET(rqstp);
 	struct nfsd_net *nn = net_generic(net, nfsd_net_id);

@@ -423,7 +420,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	switch (open->op_claim_type) {
 		case NFS4_OPEN_CLAIM_DELEGATE_CUR:
 		case NFS4_OPEN_CLAIM_NULL:
-			status = do_open_lookup(rqstp, cstate, open);
+			status = do_open_lookup(rqstp, cstate, open, &resfh);
 			if (status)
 				goto out;
 			break;

@@ -439,6 +436,7 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			status = do_open_fhandle(rqstp, cstate, open);
 			if (status)
 				goto out;
+			resfh = &cstate->current_fh;
 			break;
 		case NFS4_OPEN_CLAIM_DELEG_PREV_FH:
              	case NFS4_OPEN_CLAIM_DELEGATE_PREV:

@@ -458,9 +456,14 @@ nfsd4_open(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * successful, it (1) truncates the file if open->op_truncate was
 	 * set, (2) sets open->op_stateid, (3) sets open->op_delegation.
 	 */
-	status = nfsd4_process_open2(rqstp, &cstate->current_fh, open);
+	status = nfsd4_process_open2(rqstp, resfh, open);
 	WARN_ON(status && open->op_created);
 out:
+	if (resfh && resfh != &cstate->current_fh) {
+		fh_dup2(&cstate->current_fh, resfh);
+		fh_put(resfh);
+		kfree(resfh);
+	}
 	nfsd4_cleanup_open_state(open, status);
 	if (open->op_openowner && !nfsd4_has_session(cstate))
 		cstate->replay_owner = &open->op_openowner->oo_owner;

Signature · 1 line

1.7.9.5

J. Bruce Fields

author

2014-01-26 20:59 UTC #3 lore raw

PATCH 3/3 nfsd4: fix delegation-unlink/rename race

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

If a file is unlinked or renamed between the time when we do the local open and the time when we get the delegation, then we will return to the client indicating that it holds a delegation even though the file no longer exists under the name it was open under.

But a client performing an open-by-name, when it is returned a delegation, must be able to assume that the file is still linked at the name it was opened under.

So, hold the parent i_mutex for longer to prevent concurrent renames or unlinks.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/nfsd/nfs4proc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

fs/nfsd/nfs4proc.c +7 -3

index 844813a..ef76ba6 100644
@@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0)
 			open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS |
 							FATTR4_WORD1_TIME_MODIFY);
-	} else {
+	} else
+		/*
+		 * Note this may exit with the parent still locked.
+		 * We will hold the lock until nfsd4_open's final
+		 * lookup, to prevent renames or unlinks until we've had
+		 * a chance to an acquire a delegation if appropriate.
+		 */
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname.data, open->op_fname.len, *resfh);
-		fh_unlock(current_fh);
-	}
 	if (status)
 		goto out;
 	status = nfsd_check_obj_isreg(*resfh);

Signature · 1 line

1.7.9.5

J. Bruce Fields

2014-01-27 19:24 UTC #4 lore raw

Re: [PATCH 3/3] nfsd4: fix delegation-unlink/rename race

Quoted 32 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>
If a file is unlinked or renamed between the time when we do the local open and the time when we get the delegation, then we will return to the client indicating that it holds a delegation even though the file no longer exists under the name it was open under.
But a client performing an open-by-name, when it is returned a delegation, must be able to assume that the file is still linked at the name it was opened under.
So, hold the parent i_mutex for longer to prevent concurrent renames or unlinks.
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/nfsd/nfs4proc.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)
fs/nfsd/nfs4proc.c +4 -1
index 844813a..ef76ba6 100644
@@ -279,11 +279,15 @@ do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru
 		if (open->op_createmode == NFS4_CREATE_EXCLUSIVE && status == 0)
 			open->op_bmval[1] = (FATTR4_WORD1_TIME_ACCESS |
 							FATTR4_WORD1_TIME_MODIFY);
-	} else {
+	} else
+		/*
+		 * Note this may exit with the parent still locked.
+		 * We will hold the lock until nfsd4_open's final

fs/nfsd/nfs4proc.c do_open_lookup(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate, stru · line 293

Quoted 10 lines

fs/nfsd/nfs4proc.c +3 -2

+		 * lookup, to prevent renames or unlinks until we've had
+		 * a chance to an acquire a delegation if appropriate.
+		 */
 		status = nfsd_lookup(rqstp, current_fh,
 				     open->op_fname.data, open->op_fname.len, *resfh);
-		fh_unlock(current_fh);
-	}
 	if (status)
 		goto out;
 	status = nfsd_check_obj_isreg(*resfh);

One last-minute fix: we can now end up taking two i_mutexes. The locking's still correct but we need the following annotation on the parent directory.

(I haven't actually seen any lockdep warning trigger here.)

--b.

fs/nfsd/vfs.c +6 -1

index e85b463..a41302a 100644
@@ -207,7 +207,12 @@ nfsd_lookup_dentry(struct svc_rqst *rqstp, struct svc_fh *fhp,
 				goto out_nfserr;
 		}
 	} else {
-		fh_lock(fhp);
+		/*
+		 * In the nfsd4_open() case, this may be held across
+		 * subsequent open and delegation acquisition which may
+		 * need to take the child's i_mutex:
+		 */
+		fh_lock_nested(fhp, I_MUTEX_PARENT);
 		dentry = lookup_one_len(name, dparent, len);
 		host_err = PTR_ERR(dentry);
 		if (IS_ERR(dentry))