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>