Date: Thu, 9 Jul 2009 19:00:30 +0000 (UTC) From: Rick Macklem <rmacklem@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r195510 - in head/sys/fs: nfs nfsclient Message-ID: <200907091900.n69J0UaD094239@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: rmacklem Date: Thu Jul 9 19:00:29 2009 New Revision: 195510 URL: http://svn.freebsd.org/changeset/base/195510 Log: Since the nfscl_getclose() function both decremented open counts and, optionally, created a separate list of NFSv4 opens to be closed, it was possible for the associated OpenOwner to be free'd before the Open was closed. The problem was that the Open was taken off the OpenOwner list before the Close RPC was done and OpenOwners can be free'd once the list is empty. This patch separates out the case of doing the Close RPC into a separate function called nfscl_doclose() and simplifies nfsrpc_doclose() so that it closes a single open instead of a list of them. This avoids removing the Open from the OpenOwner list before doing the Close RPC. Approved by: re (kensmith), kib (mentor) Modified: head/sys/fs/nfs/nfs_var.h head/sys/fs/nfsclient/nfs_clrpcops.c head/sys/fs/nfsclient/nfs_clstate.c Modified: head/sys/fs/nfs/nfs_var.h ============================================================================== --- head/sys/fs/nfs/nfs_var.h Thu Jul 9 18:54:38 2009 (r195509) +++ head/sys/fs/nfs/nfs_var.h Thu Jul 9 19:00:29 2009 (r195510) @@ -457,7 +457,9 @@ void nfscl_initiate_recovery(struct nfsc int nfscl_hasexpired(struct nfsclclient *, u_int32_t, NFSPROC_T *); void nfscl_dumpstate(struct nfsmount *, int, int, int, int); void nfscl_dupopen(vnode_t, int); -int nfscl_getclose(vnode_t, struct nfsclclient **, struct nfsclopenhead *); +int nfscl_getclose(vnode_t, struct nfsclclient **); +int nfscl_doclose(vnode_t, struct nfsclclient **, NFSPROC_T *); +void nfsrpc_doclose(struct nfsmount *, struct nfsclopen *, NFSPROC_T *); int nfscl_deleg(mount_t, struct nfsclclient *, u_int8_t *, int, struct ucred *, NFSPROC_T *, struct nfscldeleg **); void nfscl_lockinit(struct nfsv4lock *); Modified: head/sys/fs/nfsclient/nfs_clrpcops.c ============================================================================== --- head/sys/fs/nfsclient/nfs_clrpcops.c Thu Jul 9 18:54:38 2009 (r195509) +++ head/sys/fs/nfsclient/nfs_clrpcops.c Thu Jul 9 19:00:29 2009 (r195510) @@ -81,8 +81,6 @@ static int nfsrpc_createv4(vnode_t , cha static int nfsrpc_locku(struct nfsrv_descript *, struct nfsmount *, struct nfscllockowner *, u_int64_t, u_int64_t, u_int32_t, struct ucred *, NFSPROC_T *, int); -static void nfsrpc_doclose(struct nfsmount *, struct nfsclopenhead *, - NFSPROC_T *); #ifdef NFS4_ACL_EXTATTR_NAME static int nfsrpc_setaclrpc(vnode_t, struct ucred *, NFSPROC_T *, struct acl *, nfsv4stateid_t *, void *); @@ -553,119 +551,117 @@ APPLESTATIC int nfsrpc_close(vnode_t vp, int doclose, NFSPROC_T *p) { struct nfsclclient *clp; - struct nfsclopenhead oh; int error; if (vnode_vtype(vp) != VREG) return (0); if (doclose) - error = nfscl_getclose(vp, &clp, &oh); + error = nfscl_doclose(vp, &clp, p); else - error = nfscl_getclose(vp, &clp, NULL); + error = nfscl_getclose(vp, &clp); if (error) return (error); - if (doclose && !LIST_EMPTY(&oh)) - nfsrpc_doclose(VFSTONFS(vnode_mount(vp)), &oh, p); nfscl_clientrelease(clp); return (0); } /* - * Close/free all the opens in the list. + * Close the open. */ -static void -nfsrpc_doclose(struct nfsmount *nmp, struct nfsclopenhead *ohp, NFSPROC_T *p) +APPLESTATIC void +nfsrpc_doclose(struct nfsmount *nmp, struct nfsclopen *op, NFSPROC_T *p) { struct nfsrv_descript nfsd, *nd = &nfsd; - struct nfsclopen *op, *nop; struct nfscllockowner *lp; struct nfscllock *lop, *nlop; struct ucred *tcred; u_int64_t off = 0, len = 0; u_int32_t type = NFSV4LOCKT_READ; - int error; + int error, do_unlock, trycnt; tcred = newnfs_getcred(); - op = LIST_FIRST(ohp); - while (op != NULL) { - nop = LIST_NEXT(op, nfso_list); - newnfs_copycred(&op->nfso_cred, tcred); - /* - * (Theoretically this could be done in the same - * compound as the close, but having multiple - * sequenced Ops in the same compound might be - * too scary for some servers.) - */ - if (op->nfso_posixlock) { - off = 0; - len = NFS64BITSSET; - type = NFSV4LOCKT_READ; - } - LIST_FOREACH(lp, &op->nfso_lock, nfsl_list) { - lop = LIST_FIRST(&lp->nfsl_lock); - while (lop != NULL) { - nlop = LIST_NEXT(lop, nfslo_list); + newnfs_copycred(&op->nfso_cred, tcred); + /* + * (Theoretically this could be done in the same + * compound as the close, but having multiple + * sequenced Ops in the same compound might be + * too scary for some servers.) + */ + if (op->nfso_posixlock) { + off = 0; + len = NFS64BITSSET; + type = NFSV4LOCKT_READ; + } + + /* + * Since this function is only called from VOP_INACTIVE(), no + * other thread will be manipulating this Open. As such, the + * lock lists are not being changed by other threads, so it should + * be safe to do this without locking. + */ + LIST_FOREACH(lp, &op->nfso_lock, nfsl_list) { + do_unlock = 1; + LIST_FOREACH_SAFE(lop, &lp->nfsl_lock, nfslo_list, nlop) { if (op->nfso_posixlock == 0) { - off = lop->nfslo_first; - len = lop->nfslo_end - lop->nfslo_first; - if (lop->nfslo_type == F_WRLCK) - type = NFSV4LOCKT_WRITE; - else - type = NFSV4LOCKT_READ; + off = lop->nfslo_first; + len = lop->nfslo_end - lop->nfslo_first; + if (lop->nfslo_type == F_WRLCK) + type = NFSV4LOCKT_WRITE; + else + type = NFSV4LOCKT_READ; } - if (lop == LIST_FIRST(&lp->nfsl_lock) || - op->nfso_posixlock == 0) { - NFSLOCKCLSTATE(); - nfscl_lockexcl(&lp->nfsl_rwlock, - NFSCLSTATEMUTEXPTR); - NFSUNLOCKCLSTATE(); - do { - error = nfsrpc_locku(nd, nmp, lp, off, len, - type, tcred, p, 0); - if ((nd->nd_repstat == NFSERR_GRACE || - nd->nd_repstat == NFSERR_DELAY) && - error == 0) - (void) nfs_catnap(PZERO, "nfs_close"); - } while ((nd->nd_repstat == NFSERR_GRACE || - nd->nd_repstat == NFSERR_DELAY) && error == 0); - NFSLOCKCLSTATE(); - nfscl_lockunlock(&lp->nfsl_rwlock); - NFSUNLOCKCLSTATE(); + if (do_unlock) { + trycnt = 0; + do { + error = nfsrpc_locku(nd, nmp, lp, off, + len, type, tcred, p, 0); + if ((nd->nd_repstat == NFSERR_GRACE || + nd->nd_repstat == NFSERR_DELAY) && + error == 0) + (void) nfs_catnap(PZERO, + "nfs_close"); + } while ((nd->nd_repstat == NFSERR_GRACE || + nd->nd_repstat == NFSERR_DELAY) && + error == 0 && trycnt++ < 5); + if (op->nfso_posixlock) + do_unlock = 0; } nfscl_freelock(lop, 0); - lop = nlop; - } } - NFSLOCKCLSTATE(); - nfscl_lockexcl(&op->nfso_own->nfsow_rwlock, NFSCLSTATEMUTEXPTR); - NFSUNLOCKCLSTATE(); - do { - error = nfscl_tryclose(op, tcred, nmp, p); - if (error == NFSERR_GRACE) - (void) nfs_catnap(PZERO, "nfs_close"); - } while (error == NFSERR_GRACE); - NFSLOCKCLSTATE(); - nfscl_lockunlock(&op->nfso_own->nfsow_rwlock); - NFSUNLOCKCLSTATE(); + } - /* - * Move the lockowner to nfsc_defunctlockowner, - * so the Renew thread will do the ReleaseLockOwner - * Op on it later. There might still be other - * opens using the same lockowner name. - */ - lp = LIST_FIRST(&op->nfso_lock); - if (lp != NULL) { - while (LIST_NEXT(lp, nfsl_list) != NULL) + /* + * There could be other Opens for different files on the same + * OpenOwner, so locking is required. + */ + NFSLOCKCLSTATE(); + nfscl_lockexcl(&op->nfso_own->nfsow_rwlock, NFSCLSTATEMUTEXPTR); + NFSUNLOCKCLSTATE(); + do { + error = nfscl_tryclose(op, tcred, nmp, p); + if (error == NFSERR_GRACE) + (void) nfs_catnap(PZERO, "nfs_close"); + } while (error == NFSERR_GRACE); + NFSLOCKCLSTATE(); + nfscl_lockunlock(&op->nfso_own->nfsow_rwlock); + + /* + * Move the lockowner to nfsc_defunctlockowner, + * so the Renew thread will do the ReleaseLockOwner + * Op on it later. There might still be other + * opens using the same lockowner name. + */ + lp = LIST_FIRST(&op->nfso_lock); + if (lp != NULL) { + while (LIST_NEXT(lp, nfsl_list) != NULL) lp = LIST_NEXT(lp, nfsl_list); - LIST_PREPEND(&nmp->nm_clp->nfsc_defunctlockowner, - &op->nfso_lock, lp, nfsl_list); - LIST_INIT(&op->nfso_lock); - } - nfscl_freeopen(op, 0); - op = nop; + LIST_PREPEND(&nmp->nm_clp->nfsc_defunctlockowner, + &op->nfso_lock, lp, nfsl_list); + LIST_INIT(&op->nfso_lock); } + nfscl_freeopen(op, 0); + NFSUNLOCKCLSTATE(); NFSFREECRED(tcred); } Modified: head/sys/fs/nfsclient/nfs_clstate.c ============================================================================== --- head/sys/fs/nfsclient/nfs_clstate.c Thu Jul 9 18:54:38 2009 (r195509) +++ head/sys/fs/nfsclient/nfs_clstate.c Thu Jul 9 19:00:29 2009 (r195510) @@ -450,7 +450,7 @@ nfscl_getstateid(vnode_t vp, u_int8_t *n { struct nfsclclient *clp; struct nfsclowner *owp; - struct nfsclopen *op; + struct nfsclopen *op = NULL; struct nfscllockowner *lp; struct nfscldeleg *dp; struct nfsnode *np; @@ -512,40 +512,40 @@ nfscl_getstateid(vnode_t vp, u_int8_t *n nfscl_filllockowner(p, own); error = nfscl_getopen(&clp->nfsc_owner, nfhp, fhlen, NULL, p, mode, NULL, &op); - if (error) { - NFSUNLOCKCLSTATE(); - return (error); - } - - /* 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); - } + 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); + } + } } - } else { - /* - * If p == NULL, it is a read ahead or write behind, - * so just look for any OpenOwner that will work. - */ + } + if (op == NULL) { + /* If not found, just look for any OpenOwner that will work. */ done = 0; owp = LIST_FIRST(&clp->nfsc_owner); while (!done && owp != NULL) { - LIST_FOREACH(op, &owp->nfsow_open, nfso_list) { - if (op->nfso_fhlen == fhlen && - !NFSBCMP(op->nfso_fh, nfhp, fhlen) && - (mode & op->nfso_mode) == mode) { - done = 1; - break; + LIST_FOREACH(op, &owp->nfsow_open, nfso_list) { + if (op->nfso_fhlen == fhlen && + !NFSBCMP(op->nfso_fh, nfhp, fhlen) && + (mode & op->nfso_mode) == mode) { + done = 1; + break; + } } - } - if (!done) - owp = LIST_NEXT(owp, nfsow_list); + if (!done) + owp = LIST_NEXT(owp, nfsow_list); } if (!done) { NFSUNLOCKCLSTATE(); @@ -2752,33 +2752,30 @@ nfscl_dupopen(vnode_t vp, int dupopens) /* * During close, find an open that needs to be dereferenced and * dereference it. If there are no more opens for this file, - * return the list of opens, so they can be closed on the - * server. As such, opens aren't closed on the server until - * all the opens for the file are closed off. + * log a message to that effect. + * Opens aren't actually Close'd until VOP_INACTIVE() is performed + * on the file's vnode. * This is the safe way, since it is difficult to identify - * which open the close is for. + * which open the close is for and I/O can be performed after the + * close(2) system call when a file is mmap'd. * If it returns 0 for success, there will be a referenced - * clp returned via clpp and a list of opens to close/free - * on ohp. + * clp returned via clpp. */ APPLESTATIC int -nfscl_getclose(vnode_t vp, struct nfsclclient **clpp, - struct nfsclopenhead *ohp) +nfscl_getclose(vnode_t vp, struct nfsclclient **clpp) { struct nfsclclient *clp; - struct nfsclowner *owp, *nowp; - struct nfsclopen *op, *nop; + struct nfsclowner *owp; + struct nfsclopen *op; struct nfscldeleg *dp; struct nfsfh *nfhp; - int error, notdecr, candelete; + int error, notdecr; error = nfscl_getcl(vp, NULL, NULL, &clp); if (error) return (error); *clpp = clp; - if (ohp != NULL) - LIST_INIT(ohp); nfhp = VTONFS(vp)->n_fhp; notdecr = 1; NFSLOCKCLSTATE(); @@ -2788,7 +2785,7 @@ nfscl_getclose(vnode_t vp, struct nfsclc * the server are DENY_NONE, I don't see a problem with hanging * onto them. (It is much easier to use one of the extant Opens * that I already have on the server when a Delegation is recalled - * than to do fresh Opens.) Someday, I might need to rethink this, but.. + * than to do fresh Opens.) Someday, I might need to rethink this, but. */ dp = nfscl_finddeleg(clp, nfhp->nfh_fh, nfhp->nfh_len); if (dp != NULL) { @@ -2813,9 +2810,7 @@ nfscl_getclose(vnode_t vp, struct nfsclc /* Now process the opens against the server. */ LIST_FOREACH(owp, &clp->nfsc_owner, nfsow_list) { - op = LIST_FIRST(&owp->nfsow_open); - while (op != NULL) { - nop = LIST_NEXT(op, nfso_list); + LIST_FOREACH(op, &owp->nfsow_open, nfso_list) { if (op->nfso_fhlen == nfhp->nfh_len && !NFSBCMP(op->nfso_fh, nfhp->nfh_fh, nfhp->nfh_len)) { @@ -2825,74 +2820,76 @@ nfscl_getclose(vnode_t vp, struct nfsclc op->nfso_opencnt--; } /* - * There are more opens, so just return after - * putting any opens already found back in the - * state list. + * There are more opens, so just return. */ if (op->nfso_opencnt > 0) { - if (ohp != NULL) { - /* Reattach open until later */ - op = LIST_FIRST(ohp); - while (op != NULL) { - nop = LIST_NEXT(op, nfso_list); - LIST_REMOVE(op, nfso_list); - LIST_INSERT_HEAD( - &op->nfso_own->nfsow_open, - op, nfso_list); - op = nop; - } - LIST_INIT(ohp); - } NFSUNLOCKCLSTATE(); return (0); } - - /* - * Move this entry to the list of opens to be - * returned. (If we find other open(s) still in - * use, it will be put back in the state list - * in the code just above.) - */ - if (ohp != NULL) { - LIST_REMOVE(op, nfso_list); - LIST_INSERT_HEAD(ohp, op, nfso_list); - } } - op = nop; } } + NFSUNLOCKCLSTATE(); + if (notdecr) + printf("nfscl: never fnd open\n"); + return (0); +} - if (dp != NULL && ohp != NULL) { - /* - * If we are flushing all writes against the server for this - * file upon close, we do not need to keep the local opens - * (against the delegation) if they all have an opencnt == 0, - * since there are now no opens on the file and no dirty blocks. - * If the writes aren't being flushed upon close, - * a test for "no dirty blocks to write back" would have to - * be added to this code. - */ - candelete = 1; - LIST_FOREACH(owp, &dp->nfsdl_owner, nfsow_list) { +APPLESTATIC int +nfscl_doclose(vnode_t vp, struct nfsclclient **clpp, NFSPROC_T *p) +{ + struct nfsclclient *clp; + struct nfsclowner *owp, *nowp; + struct nfsclopen *op; + struct nfscldeleg *dp; + struct nfsfh *nfhp; + int error; + + error = nfscl_getcl(vp, NULL, NULL, &clp); + if (error) + return (error); + *clpp = clp; + + nfhp = VTONFS(vp)->n_fhp; + NFSLOCKCLSTATE(); + /* + * First get rid of the local Open structures, which should be no + * longer in use. + */ + dp = nfscl_finddeleg(clp, nfhp->nfh_fh, nfhp->nfh_len); + if (dp != NULL) { + LIST_FOREACH_SAFE(owp, &dp->nfsdl_owner, nfsow_list, nowp) { op = LIST_FIRST(&owp->nfsow_open); - if (op != NULL && op->nfso_opencnt > 0) { - candelete = 0; - break; + if (op != NULL) { + KASSERT((op->nfso_opencnt == 0), + ("nfscl: bad open cnt on deleg")); + nfscl_freeopen(op, 1); } + nfscl_freeopenowner(owp, 1); } - if (candelete) { - LIST_FOREACH_SAFE(owp, &dp->nfsdl_owner, nfsow_list, - nowp) { - op = LIST_FIRST(&owp->nfsow_open); - if (op != NULL) - nfscl_freeopen(op, 1); - nfscl_freeopenowner(owp, 1); + } + + /* Now process the opens against the server. */ +lookformore: + LIST_FOREACH(owp, &clp->nfsc_owner, nfsow_list) { + op = LIST_FIRST(&owp->nfsow_open); + while (op != NULL) { + if (op->nfso_fhlen == nfhp->nfh_len && + !NFSBCMP(op->nfso_fh, nfhp->nfh_fh, + nfhp->nfh_len)) { + /* Found an open, close it. */ + KASSERT((op->nfso_opencnt == 0), + ("nfscl: bad open cnt on server")); + NFSUNLOCKCLSTATE(); + nfsrpc_doclose(VFSTONFS(vnode_mount(vp)), op, + p); + NFSLOCKCLSTATE(); + goto lookformore; } + op = LIST_NEXT(op, nfso_list); } } NFSUNLOCKCLSTATE(); - if (notdecr && ohp == NULL) - printf("nfscl: never fnd open\n"); return (0); }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200907091900.n69J0UaD094239>