Skip site navigation (1)Skip section navigation (2)
Date:      Mon, 18 Jul 2011 21:04:35 +0000 (UTC)
From:      Rick Macklem <rmacklem@FreeBSD.org>
To:        src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-8@freebsd.org
Subject:   svn commit: r224203 - stable/8/sys/fs/nfsclient
Message-ID:  <201107182104.p6IL4ZQU039265@svn.freebsd.org>

next in thread | raw e-mail | index | archive | help
Author: rmacklem
Date: Mon Jul 18 21:04:35 2011
New Revision: 224203
URL: http://svn.freebsd.org/changeset/base/224203

Log:
  MFC: r223774
  The algorithm used by nfscl_getopen() could have resulted in
  multiple instances of the same lock_owner when a process both
  inherited an open file descriptor plus opened the same file itself.
  Since some NFSv4 servers cannot handle multiple instances of
  the same lock_owner string, this patch changes the algorithm
  used by nfscl_getopen() in the new NFSv4 client to keep that
  from happening. The new algorithm is simpler, since there is
  no longer any need to ascend the process's parentage tree because
  all NFSv4 Closes for a file are done at VOP_INACTIVE()/VOP_RECLAIM(),
  making the Opens indistinct w.r.t. use with Lock Ops.
  This problem was discovered at the recent NFSv4 interoperability
  Bakeathon.

Modified:
  stable/8/sys/fs/nfsclient/nfs_clstate.c
Directory Properties:
  stable/8/sys/   (props changed)
  stable/8/sys/amd64/include/xen/   (props changed)
  stable/8/sys/cddl/contrib/opensolaris/   (props changed)
  stable/8/sys/contrib/dev/acpica/   (props changed)
  stable/8/sys/contrib/pf/   (props changed)

Modified: stable/8/sys/fs/nfsclient/nfs_clstate.c
==============================================================================
--- stable/8/sys/fs/nfsclient/nfs_clstate.c	Mon Jul 18 20:57:43 2011	(r224202)
+++ stable/8/sys/fs/nfsclient/nfs_clstate.c	Mon Jul 18 21:04:35 2011	(r224203)
@@ -95,7 +95,7 @@ int nfscl_deleghighwater = NFSCLDELEGHIG
 
 static int nfscl_delegcnt = 0;
 static int nfscl_getopen(struct nfsclownerhead *, u_int8_t *, int, u_int8_t *,
-    NFSPROC_T *, u_int32_t, struct nfsclowner **, struct nfsclopen **);
+    u_int8_t *, u_int32_t, struct nfscllockowner **, struct nfsclopen **);
 static void nfscl_clrelease(struct nfsclclient *);
 static void nfscl_cleanclient(struct nfsclclient *);
 static void nfscl_expireclient(struct nfsclclient *, struct nfsmount *,
@@ -521,25 +521,20 @@ nfscl_getstateid(vnode_t vp, u_int8_t *n
 		 * for a matching OpenOwner and use that.
 		 */
 		nfscl_filllockowner(p->td_proc, own, F_POSIX);
-		error = nfscl_getopen(&clp->nfsc_owner, nfhp, fhlen, NULL, p,
-		    mode, NULL, &op);
-		if (error == 0) {
-			/* now look for a lockowner */
-			LIST_FOREACH(lp, &op->nfso_lock, nfsl_list) {
-				if (!NFSBCMP(lp->nfsl_owner, own,
-				    NFSV4CL_LOCKNAMELEN)) {
-					stateidp->seqid =
-					    lp->nfsl_stateid.seqid;
-					stateidp->other[0] =
-					    lp->nfsl_stateid.other[0];
-					stateidp->other[1] =
-					    lp->nfsl_stateid.other[1];
-					stateidp->other[2] =
-					    lp->nfsl_stateid.other[2];
-					NFSUNLOCKCLSTATE();
-					return (0);
-				}
-			}
+		lp = NULL;
+		error = nfscl_getopen(&clp->nfsc_owner, nfhp, fhlen, own, own,
+		    mode, &lp, &op);
+		if (error == 0 && lp != NULL) {
+			stateidp->seqid =
+			    lp->nfsl_stateid.seqid;
+			stateidp->other[0] =
+			    lp->nfsl_stateid.other[0];
+			stateidp->other[1] =
+			    lp->nfsl_stateid.other[1];
+			stateidp->other[2] =
+			    lp->nfsl_stateid.other[2];
+			NFSUNLOCKCLSTATE();
+			return (0);
 		}
 	}
 	if (op == NULL) {
@@ -578,55 +573,74 @@ nfscl_getstateid(vnode_t vp, u_int8_t *n
 }
 
 /*
- * Get an existing open. Search up the parentage tree for a match and
- * return with the first one found.
+ * Search for a matching file, mode and, optionally, lockowner.
  */
 static int
 nfscl_getopen(struct nfsclownerhead *ohp, u_int8_t *nfhp, int fhlen,
-    u_int8_t *rown, NFSPROC_T *p, u_int32_t mode, struct nfsclowner **owpp,
-    struct nfsclopen **opp)
+    u_int8_t *openown, u_int8_t *lockown, u_int32_t mode,
+    struct nfscllockowner **lpp, struct nfsclopen **opp)
 {
-	struct nfsclowner *owp = NULL;
-	struct nfsclopen *op;
-	NFSPROC_T *nproc;
-	u_int8_t own[NFSV4CL_LOCKNAMELEN], *ownp;
+	struct nfsclowner *owp;
+	struct nfsclopen *op, *rop, *rop2;
+	struct nfscllockowner *lp;
+	int keep_looping;
 
-	nproc = p;
-	op = NULL;
-	while (op == NULL && (nproc != NULL || rown != NULL)) {
-		if (nproc != NULL) {
-			nfscl_filllockowner(nproc->td_proc, own, F_POSIX);
-			ownp = own;
-		} else {
-			ownp = rown;
-		}
-		/* Search the client list */
-		LIST_FOREACH(owp, ohp, nfsow_list) {
-			if (!NFSBCMP(owp->nfsow_owner, ownp,
-			    NFSV4CL_LOCKNAMELEN))
-				break;
-		}
-		if (owp != NULL) {
-			/* and look for the correct open */
-			LIST_FOREACH(op, &owp->nfsow_open, nfso_list) {
-				if (op->nfso_fhlen == fhlen &&
-				    !NFSBCMP(op->nfso_fh, nfhp, fhlen)
-				    && (op->nfso_mode & mode) == mode) {
-					break;
+	if (lpp != NULL)
+		*lpp = NULL;
+	/*
+	 * rop will be set to the open to be returned. There are three
+	 * variants of this, all for an open of the correct file:
+	 * 1 - A match of lockown.
+	 * 2 - A match of the openown, when no lockown match exists.
+	 * 3 - A match for any open, if no openown or lockown match exists.
+	 * Looking for #2 over #3 probably isn't necessary, but since
+	 * RFC3530 is vague w.r.t. the relationship between openowners and
+	 * lockowners, I think this is the safer way to go.
+	 */
+	rop = NULL;
+	rop2 = NULL;
+	keep_looping = 1;
+	/* Search the client list */
+	owp = LIST_FIRST(ohp);
+	while (owp != NULL && keep_looping != 0) {
+		/* and look for the correct open */
+		op = LIST_FIRST(&owp->nfsow_open);
+		while (op != NULL && keep_looping != 0) {
+			if (op->nfso_fhlen == fhlen &&
+			    !NFSBCMP(op->nfso_fh, nfhp, fhlen)
+			    && (op->nfso_mode & mode) == mode) {
+				if (lpp != NULL) {
+					/* Now look for a matching lockowner. */
+					LIST_FOREACH(lp, &op->nfso_lock,
+					    nfsl_list) {
+						if (!NFSBCMP(lp->nfsl_owner,
+						    lockown,
+						    NFSV4CL_LOCKNAMELEN)) {
+							*lpp = lp;
+							rop = op;
+							keep_looping = 0;
+							break;
+						}
+					}
 				}
+				if (rop == NULL && !NFSBCMP(owp->nfsow_owner,
+				    openown, NFSV4CL_LOCKNAMELEN)) {
+					rop = op;
+					if (lpp == NULL)
+						keep_looping = 0;
+				}
+				if (rop2 == NULL)
+					rop2 = op;
 			}
+			op = LIST_NEXT(op, nfso_list);
 		}
-		if (rown != NULL)
-			break;
-		if (op == NULL)
-			nproc = nfscl_getparent(nproc);
+		owp = LIST_NEXT(owp, nfsow_list);
 	}
-	if (op == NULL) {
+	if (rop == NULL)
+		rop = rop2;
+	if (rop == NULL)
 		return (EBADF);
-	}
-	if (owpp)
-		*owpp = owp;
-	*opp = op;
+	*opp = rop;
 	return (0);
 }
 
@@ -891,16 +905,16 @@ nfscl_getbytelock(vnode_t vp, u_int64_t 
 	struct nfscldeleg *dp = NULL, *ldp = NULL;
 	struct nfscllockownerhead *lhp = NULL;
 	struct nfsnode *np;
-	u_int8_t own[NFSV4CL_LOCKNAMELEN], *ownp;
+	u_int8_t own[NFSV4CL_LOCKNAMELEN], *ownp, openown[NFSV4CL_LOCKNAMELEN];
+	u_int8_t *openownp;
 	int error = 0, ret, donelocally = 0;
 	u_int32_t mode;
 
-	if (type == F_WRLCK)
-		mode = NFSV4OPEN_ACCESSWRITE;
-	else
-		mode = NFSV4OPEN_ACCESSREAD;
+	/* For Lock Ops, the open mode doesn't matter, so use 0 to match any. */
+	mode = 0;
 	np = VTONFS(vp);
 	*lpp = NULL;
+	lp = NULL;
 	*newonep = 0;
 	*donelocallyp = 0;
 
@@ -940,9 +954,12 @@ nfscl_getbytelock(vnode_t vp, u_int64_t 
 	op = NULL;
 	if (recovery) {
 		ownp = rownp;
+		openownp = ropenownp;
 	} else {
 		nfscl_filllockowner(id, own, flags);
 		ownp = own;
+		nfscl_filllockowner(p->td_proc, openown, F_POSIX);
+		openownp = openown;
 	}
 	if (!recovery) {
 		NFSLOCKCLSTATE();
@@ -961,13 +978,13 @@ nfscl_getbytelock(vnode_t vp, u_int64_t 
 			dp = NULL;
 	}
 	if (dp != NULL) {
-		/* Now, find the associated open to get the correct openowner */
+		/* Now, find an open and maybe a lockowner. */
 		ret = nfscl_getopen(&dp->nfsdl_owner, np->n_fhp->nfh_fh,
-		    np->n_fhp->nfh_len, NULL, p, mode, NULL, &op);
+		    np->n_fhp->nfh_len, openownp, ownp, mode, NULL, &op);
 		if (ret)
 			ret = nfscl_getopen(&clp->nfsc_owner,
-			    np->n_fhp->nfh_fh, np->n_fhp->nfh_len, NULL, p,
-			    mode, NULL, &op);
+			    np->n_fhp->nfh_fh, np->n_fhp->nfh_len, openownp,
+			    ownp, mode, NULL, &op);
 		if (!ret) {
 			lhp = &dp->nfsdl_lock;
 			TAILQ_REMOVE(&clp->nfsc_deleg, dp, nfsdl_list);
@@ -980,16 +997,11 @@ nfscl_getbytelock(vnode_t vp, u_int64_t 
 	}
 	if (!donelocally) {
 		/*
-		 * Get the related Open.
+		 * Get the related Open and maybe lockowner.
 		 */
-		if (recovery)
-			error = nfscl_getopen(&clp->nfsc_owner,
-			    np->n_fhp->nfh_fh, np->n_fhp->nfh_len, ropenownp,
-			    NULL, mode, NULL, &op);
-		else
-			error = nfscl_getopen(&clp->nfsc_owner,
-			    np->n_fhp->nfh_fh, np->n_fhp->nfh_len, NULL, p,
-			    mode, NULL, &op);
+		error = nfscl_getopen(&clp->nfsc_owner,
+		    np->n_fhp->nfh_fh, np->n_fhp->nfh_len, openownp,
+		    ownp, mode, &lp, &op);
 		if (!error)
 			lhp = &op->nfso_lock;
 	}
@@ -1010,10 +1022,11 @@ nfscl_getbytelock(vnode_t vp, u_int64_t 
 	/*
 	 * Ok, see if a lockowner exists and create one, as required.
 	 */
-	LIST_FOREACH(lp, lhp, nfsl_list) {
-		if (!NFSBCMP(lp->nfsl_owner, ownp, NFSV4CL_LOCKNAMELEN))
-			break;
-	}
+	if (lp == NULL)
+		LIST_FOREACH(lp, lhp, nfsl_list) {
+			if (!NFSBCMP(lp->nfsl_owner, ownp, NFSV4CL_LOCKNAMELEN))
+				break;
+		}
 	if (lp == NULL) {
 		NFSBCOPY(ownp, nlp->nfsl_owner, NFSV4CL_LOCKNAMELEN);
 		if (recovery)



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201107182104.p6IL4ZQU039265>