svn commit: r322722 - head/sys/fs/nfsclient
Konstantin Belousov
kib at FreeBSD.org
Sun Aug 20 10:08:47 UTC 2017
Author: kib
Date: Sun Aug 20 10:08:45 2017
New Revision: 322722
URL: https://svnweb.freebsd.org/changeset/base/322722
Log:
Do not drop NFS vnode lock when performing consistency checks.
Currently several paths in the NFS client upgrade the shared vnode
lock to exclusive, which might cause temporal dropping of the lock.
This action appears to be fatal for nullfs mounts over NFS. If the
operation is performed over nullfs vnode, then bypassed down to NFS
VOP, and the lock is dropped, other thread might reclaim the upper
nullfs vnode. Since on reclaim the nullfs vnode lock and NFS vnode
lock are split, the original lock state of the nullfs vnode is not
restored. As result, VFS operations receive not locked vnode after a
VOP call.
Stop upgrading the vnode lock when we check the consistency or flush
buffers as result of detected inconsistency. Instead, allocate a new
lockmgr lock for each NFS node, which is locked exclusive instead of
the vnode lock upgrade. In other words, the other parallel
modification of the vnode are excluded by either vnode lock conflict
or exclusivity of the new lock when the vnode lock is shared.
Also revert r316529 because now the vnode cannot be reclaimed during
ncl_vinvalbuf().
In collaboration with: pho
Reviewed by: rmacklem
Reported and tested by: pho
Sponsored by: The FreeBSD Foundation
MFC after: 2 weeks
Differential revision: https://reviews.freebsd.org/D12083
Modified:
head/sys/fs/nfsclient/nfs_clbio.c
head/sys/fs/nfsclient/nfs_clnode.c
head/sys/fs/nfsclient/nfs_clport.c
head/sys/fs/nfsclient/nfs_clsubs.c
head/sys/fs/nfsclient/nfs_clvnops.c
head/sys/fs/nfsclient/nfsnode.h
Modified: head/sys/fs/nfsclient/nfs_clbio.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clbio.c Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfs_clbio.c Sun Aug 20 10:08:45 2017 (r322722)
@@ -365,20 +365,13 @@ nfs_bioread_check_cons(struct vnode *vp, struct thread
int error = 0;
struct vattr vattr;
struct nfsnode *np = VTONFS(vp);
- int old_lock;
+ bool old_lock;
/*
- * Grab the exclusive lock before checking whether the cache is
- * consistent.
- * XXX - We can make this cheaper later (by acquiring cheaper locks).
- * But for now, this suffices.
+ * Ensure the exclusove access to the node before checking
+ * whether the cache is consistent.
*/
- old_lock = ncl_upgrade_vnlock(vp);
- if (vp->v_iflag & VI_DOOMED) {
- error = EBADF;
- goto out;
- }
-
+ old_lock = ncl_excl_start(vp);
mtx_lock(&np->n_mtx);
if (np->n_flag & NMODIFIED) {
mtx_unlock(&np->n_mtx);
@@ -386,9 +379,7 @@ nfs_bioread_check_cons(struct vnode *vp, struct thread
if (vp->v_type != VDIR)
panic("nfs: bioread, not dir");
ncl_invaldir(vp);
- error = ncl_vinvalbuf(vp, V_SAVE, td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
+ error = ncl_vinvalbuf(vp, V_SAVE | V_ALLOWCLEAN, td, 1);
if (error != 0)
goto out;
}
@@ -404,16 +395,14 @@ nfs_bioread_check_cons(struct vnode *vp, struct thread
mtx_unlock(&np->n_mtx);
error = VOP_GETATTR(vp, &vattr, cred);
if (error)
- return (error);
+ goto out;
mtx_lock(&np->n_mtx);
if ((np->n_flag & NSIZECHANGED)
|| (NFS_TIMESPEC_COMPARE(&np->n_mtime, &vattr.va_mtime))) {
mtx_unlock(&np->n_mtx);
if (vp->v_type == VDIR)
ncl_invaldir(vp);
- error = ncl_vinvalbuf(vp, V_SAVE, td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
+ error = ncl_vinvalbuf(vp, V_SAVE | V_ALLOWCLEAN, td, 1);
if (error != 0)
goto out;
mtx_lock(&np->n_mtx);
@@ -423,7 +412,7 @@ nfs_bioread_check_cons(struct vnode *vp, struct thread
mtx_unlock(&np->n_mtx);
}
out:
- ncl_downgrade_vnlock(vp, old_lock);
+ ncl_excl_finish(vp, old_lock);
return (error);
}
@@ -608,8 +597,6 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int iof
while (error == NFSERR_BAD_COOKIE) {
ncl_invaldir(vp);
error = ncl_vinvalbuf(vp, 0, td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
/*
* Yuck! The directory has been modified on the
@@ -933,8 +920,6 @@ ncl_write(struct vop_write_args *ap)
KDTRACE_NFS_ATTRCACHE_FLUSH_DONE(vp);
error = ncl_vinvalbuf(vp, V_SAVE | ((ioflag &
IO_VMIO) != 0 ? V_VMIO : 0), td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
if (error != 0)
return (error);
} else
@@ -1016,9 +1001,6 @@ ncl_write(struct vop_write_args *ap)
KDTRACE_NFS_ATTRCACHE_FLUSH_DONE(vp);
error = ncl_vinvalbuf(vp, V_SAVE | ((ioflag &
IO_VMIO) != 0 ? V_VMIO : 0), td, 1);
- if (error == 0 &&
- (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
if (error != 0)
return (error);
wouldcommit = biosize;
@@ -1336,7 +1318,7 @@ ncl_vinvalbuf(struct vnode *vp, int flags, struct thre
struct nfsnode *np = VTONFS(vp);
struct nfsmount *nmp = VFSTONFS(vp->v_mount);
int error = 0, slpflag, slptimeo;
- int old_lock = 0;
+ bool old_lock;
ASSERT_VOP_LOCKED(vp, "ncl_vinvalbuf");
@@ -1352,16 +1334,9 @@ ncl_vinvalbuf(struct vnode *vp, int flags, struct thre
slptimeo = 0;
}
- old_lock = ncl_upgrade_vnlock(vp);
- if (vp->v_iflag & VI_DOOMED) {
- /*
- * Since vgonel() uses the generic vinvalbuf() to flush
- * dirty buffers and it does not call this function, it
- * is safe to just return OK when VI_DOOMED is set.
- */
- ncl_downgrade_vnlock(vp, old_lock);
- return (0);
- }
+ old_lock = ncl_excl_start(vp);
+ if (old_lock)
+ flags |= V_ALLOWCLEAN;
/*
* Now, flush as required.
@@ -1400,7 +1375,7 @@ ncl_vinvalbuf(struct vnode *vp, int flags, struct thre
np->n_flag &= ~NMODIFIED;
mtx_unlock(&np->n_mtx);
out:
- ncl_downgrade_vnlock(vp, old_lock);
+ ncl_excl_finish(vp, old_lock);
return error;
}
Modified: head/sys/fs/nfsclient/nfs_clnode.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clnode.c Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfs_clnode.c Sun Aug 20 10:08:45 2017 (r322722)
@@ -141,6 +141,9 @@ ncl_nget(struct mount *mntp, u_int8_t *fhp, int fhsize
* happened to return an error no special casing is needed).
*/
mtx_init(&np->n_mtx, "NEWNFSnode lock", NULL, MTX_DEF | MTX_DUPOK);
+ lockinit(&np->n_excl, PVFS, "nfsupg", VLKTIMEOUT, LK_NOSHARE |
+ LK_CANRECURSE);
+
/*
* NFS supports recursive and shared locking.
*/
@@ -167,6 +170,7 @@ ncl_nget(struct mount *mntp, u_int8_t *fhp, int fhsize
*npp = NULL;
FREE((caddr_t)np->n_fhp, M_NFSFH);
mtx_destroy(&np->n_mtx);
+ lockdestroy(&np->n_excl);
uma_zfree(newnfsnode_zone, np);
return (error);
}
@@ -332,6 +336,7 @@ ncl_reclaim(struct vop_reclaim_args *ap)
if (np->n_v4 != NULL)
FREE((caddr_t)np->n_v4, M_NFSV4NODE);
mtx_destroy(&np->n_mtx);
+ lockdestroy(&np->n_excl);
uma_zfree(newnfsnode_zone, vp->v_data);
vp->v_data = NULL;
return (0);
Modified: head/sys/fs/nfsclient/nfs_clport.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clport.c Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfs_clport.c Sun Aug 20 10:08:45 2017 (r322722)
@@ -230,6 +230,8 @@ nfscl_nget(struct mount *mntp, struct vnode *dvp, stru
* happened to return an error no special casing is needed).
*/
mtx_init(&np->n_mtx, "NEWNFSnode lock", NULL, MTX_DEF | MTX_DUPOK);
+ lockinit(&np->n_excl, PVFS, "nfsupg", VLKTIMEOUT, LK_NOSHARE |
+ LK_CANRECURSE);
/*
* Are we getting the root? If so, make sure the vnode flags
@@ -271,6 +273,7 @@ nfscl_nget(struct mount *mntp, struct vnode *dvp, stru
if (error != 0) {
*npp = NULL;
mtx_destroy(&np->n_mtx);
+ lockdestroy(&np->n_excl);
FREE((caddr_t)nfhp, M_NFSFH);
if (np->n_v4 != NULL)
FREE((caddr_t)np->n_v4, M_NFSV4NODE);
Modified: head/sys/fs/nfsclient/nfs_clsubs.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clsubs.c Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfs_clsubs.c Sun Aug 20 10:08:45 2017 (r322722)
@@ -135,30 +135,33 @@ ncl_dircookie_unlock(struct nfsnode *np)
mtx_unlock(&np->n_mtx);
}
-int
-ncl_upgrade_vnlock(struct vnode *vp)
+bool
+ncl_excl_start(struct vnode *vp)
{
- int old_lock;
+ struct nfsnode *np;
+ int vn_lk;
- ASSERT_VOP_LOCKED(vp, "ncl_upgrade_vnlock");
- old_lock = NFSVOPISLOCKED(vp);
- if (old_lock != LK_EXCLUSIVE) {
- KASSERT(old_lock == LK_SHARED,
- ("ncl_upgrade_vnlock: wrong old_lock %d", old_lock));
- /* Upgrade to exclusive lock, this might block */
- NFSVOPLOCK(vp, LK_UPGRADE | LK_RETRY);
- }
- return (old_lock);
+ ASSERT_VOP_LOCKED(vp, "ncl_excl_start");
+ vn_lk = NFSVOPISLOCKED(vp);
+ if (vn_lk == LK_EXCLUSIVE)
+ return (false);
+ KASSERT(vn_lk == LK_SHARED,
+ ("ncl_excl_start: wrong vnode lock %d", vn_lk));
+ /* Ensure exclusive access, this might block */
+ np = VTONFS(vp);
+ lockmgr(&np->n_excl, LK_EXCLUSIVE, NULL);
+ return (true);
}
void
-ncl_downgrade_vnlock(struct vnode *vp, int old_lock)
+ncl_excl_finish(struct vnode *vp, bool old_lock)
{
- if (old_lock != LK_EXCLUSIVE) {
- KASSERT(old_lock == LK_SHARED, ("wrong old_lock %d", old_lock));
- /* Downgrade from exclusive lock. */
- NFSVOPLOCK(vp, LK_DOWNGRADE | LK_RETRY);
- }
+ struct nfsnode *np;
+
+ if (!old_lock)
+ return;
+ np = VTONFS(vp);
+ lockmgr(&np->n_excl, LK_RELEASE, NULL);
}
#ifdef NFS_ACDEBUG
Modified: head/sys/fs/nfsclient/nfs_clvnops.c
==============================================================================
--- head/sys/fs/nfsclient/nfs_clvnops.c Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfs_clvnops.c Sun Aug 20 10:08:45 2017 (r322722)
@@ -520,8 +520,6 @@ nfs_open(struct vop_open_args *ap)
if (np->n_flag & NMODIFIED) {
mtx_unlock(&np->n_mtx);
error = ncl_vinvalbuf(vp, V_SAVE, ap->a_td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
if (error == EINTR || error == EIO) {
if (NFS_ISV4(vp))
(void) nfsrpc_close(vp, 0, ap->a_td);
@@ -558,8 +556,6 @@ nfs_open(struct vop_open_args *ap)
np->n_direofoffset = 0;
mtx_unlock(&np->n_mtx);
error = ncl_vinvalbuf(vp, V_SAVE, ap->a_td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
if (error == EINTR || error == EIO) {
if (NFS_ISV4(vp))
(void) nfsrpc_close(vp, 0, ap->a_td);
@@ -580,8 +576,6 @@ nfs_open(struct vop_open_args *ap)
if (np->n_directio_opens == 0) {
mtx_unlock(&np->n_mtx);
error = ncl_vinvalbuf(vp, V_SAVE, ap->a_td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
if (error) {
if (NFS_ISV4(vp))
(void) nfsrpc_close(vp, 0, ap->a_td);
@@ -722,8 +716,6 @@ nfs_close(struct vop_close_args *ap)
}
} else {
error = ncl_vinvalbuf(vp, V_SAVE, ap->a_td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
}
mtx_lock(&np->n_mtx);
}
@@ -942,9 +934,7 @@ nfs_setattr(struct vop_setattr_args *ap)
mtx_unlock(&np->n_mtx);
error = ncl_vinvalbuf(vp, vap->va_size == 0 ?
0 : V_SAVE, td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
- if (error != 0) {
+ if (error != 0) {
vnode_pager_setsize(vp, tsize);
return (error);
}
@@ -971,8 +961,6 @@ nfs_setattr(struct vop_setattr_args *ap)
(np->n_flag & NMODIFIED) && vp->v_type == VREG) {
mtx_unlock(&np->n_mtx);
error = ncl_vinvalbuf(vp, V_SAVE, td, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- return (EBADF);
if (error == EINTR || error == EIO)
return (error);
} else
@@ -1676,9 +1664,7 @@ nfs_remove(struct vop_remove_args *ap)
* unnecessary delayed writes later.
*/
error = ncl_vinvalbuf(vp, 0, cnp->cn_thread, 1);
- if (error == 0 && (vp->v_iflag & VI_DOOMED) != 0)
- error = EBADF;
- else if (error != EINTR && error != EIO)
+ if (error != EINTR && error != EIO)
/* Do the rpc */
error = nfs_removerpc(dvp, vp, cnp->cn_nameptr,
cnp->cn_namelen, cnp->cn_cred, cnp->cn_thread);
@@ -3089,10 +3075,6 @@ nfs_advlock(struct vop_advlock_args *ap)
if ((np->n_flag & NMODIFIED) || ret ||
np->n_change != va.va_filerev) {
(void) ncl_vinvalbuf(vp, V_SAVE, td, 1);
- if ((vp->v_iflag & VI_DOOMED) != 0) {
- NFSVOPUNLOCK(vp, 0);
- return (EBADF);
- }
np->n_attrstamp = 0;
KDTRACE_NFS_ATTRCACHE_FLUSH_DONE(vp);
ret = VOP_GETATTR(vp, &va, cred);
Modified: head/sys/fs/nfsclient/nfsnode.h
==============================================================================
--- head/sys/fs/nfsclient/nfsnode.h Sun Aug 20 10:07:45 2017 (r322721)
+++ head/sys/fs/nfsclient/nfsnode.h Sun Aug 20 10:08:45 2017 (r322722)
@@ -92,6 +92,8 @@ struct nfs_accesscache {
*/
struct nfsnode {
struct mtx n_mtx; /* Protects all of these members */
+ struct lock n_excl; /* Exclusive helper for shared
+ vnode lock */
u_quad_t n_size; /* Current size of file */
u_quad_t n_brev; /* Modify rev when cached */
u_quad_t n_lrev; /* Modify rev for lease */
@@ -184,8 +186,8 @@ int ncl_removeit(struct sillyrename *, struct vnode *)
int ncl_nget(struct mount *, u_int8_t *, int, struct nfsnode **, int);
nfsuint64 *ncl_getcookie(struct nfsnode *, off_t, int);
void ncl_invaldir(struct vnode *);
-int ncl_upgrade_vnlock(struct vnode *);
-void ncl_downgrade_vnlock(struct vnode *, int);
+bool ncl_excl_start(struct vnode *);
+void ncl_excl_finish(struct vnode *, bool old_lock);
void ncl_dircookie_lock(struct nfsnode *);
void ncl_dircookie_unlock(struct nfsnode *);
More information about the svn-src-head
mailing list