nfsclient: add nfs node locking around uses of n_direofoffset
During code inspection I noticed that the n_direofoffset field of the NFS node was being manipulated without any lock being held to make it SMP safe. This patch adds locking of the NFS node's mutex around handling of n_direofoffset to make it SMP safe. I have not seen any failure that could be attributed to n_direofoffset being manipulated concurrently by multiple processors, but I think this is possible, since directories are read with shared vnode locking, plus locks only on individual buffer cache blocks. However, there have been as yet unexplained issues w.r.t reading large directories over NFS that could have conceivably been caused by concurrent manipulation of n_direofoffset. MFC after: 2 weeks
This commit is contained in:
@@ -584,11 +584,14 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred)
|
|||||||
break;
|
break;
|
||||||
case VDIR:
|
case VDIR:
|
||||||
NFSINCRGLOBAL(nfsstatsv1.biocache_readdirs);
|
NFSINCRGLOBAL(nfsstatsv1.biocache_readdirs);
|
||||||
|
NFSLOCKNODE(np);
|
||||||
if (np->n_direofoffset
|
if (np->n_direofoffset
|
||||||
&& uio->uio_offset >= np->n_direofoffset) {
|
&& uio->uio_offset >= np->n_direofoffset) {
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
error = 0;
|
error = 0;
|
||||||
goto out;
|
goto out;
|
||||||
}
|
}
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
lbn = (uoff_t)uio->uio_offset / NFS_DIRBLKSIZ;
|
lbn = (uoff_t)uio->uio_offset / NFS_DIRBLKSIZ;
|
||||||
on = uio->uio_offset & (NFS_DIRBLKSIZ - 1);
|
on = uio->uio_offset & (NFS_DIRBLKSIZ - 1);
|
||||||
bp = nfs_getcacheblk(vp, lbn, NFS_DIRBLKSIZ, td);
|
bp = nfs_getcacheblk(vp, lbn, NFS_DIRBLKSIZ, td);
|
||||||
@@ -620,11 +623,14 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred)
|
|||||||
* NFSERR_BAD_COOKIE (double yuch!).
|
* NFSERR_BAD_COOKIE (double yuch!).
|
||||||
*/
|
*/
|
||||||
for (i = 0; i <= lbn && !error; i++) {
|
for (i = 0; i <= lbn && !error; i++) {
|
||||||
|
NFSLOCKNODE(np);
|
||||||
if (np->n_direofoffset
|
if (np->n_direofoffset
|
||||||
&& (i * NFS_DIRBLKSIZ) >= np->n_direofoffset) {
|
&& (i * NFS_DIRBLKSIZ) >= np->n_direofoffset) {
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
error = 0;
|
error = 0;
|
||||||
goto out;
|
goto out;
|
||||||
}
|
}
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
bp = nfs_getcacheblk(vp, i, NFS_DIRBLKSIZ, td);
|
bp = nfs_getcacheblk(vp, i, NFS_DIRBLKSIZ, td);
|
||||||
if (!bp) {
|
if (!bp) {
|
||||||
error = newnfs_sigintr(nmp, td);
|
error = newnfs_sigintr(nmp, td);
|
||||||
@@ -667,11 +673,13 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred)
|
|||||||
* (You need the current block first, so that you have the
|
* (You need the current block first, so that you have the
|
||||||
* directory offset cookie of the next block.)
|
* directory offset cookie of the next block.)
|
||||||
*/
|
*/
|
||||||
|
NFSLOCKNODE(np);
|
||||||
if (nmp->nm_readahead > 0 &&
|
if (nmp->nm_readahead > 0 &&
|
||||||
(bp->b_flags & B_INVAL) == 0 &&
|
(bp->b_flags & B_INVAL) == 0 &&
|
||||||
(np->n_direofoffset == 0 ||
|
(np->n_direofoffset == 0 ||
|
||||||
(lbn + 1) * NFS_DIRBLKSIZ < np->n_direofoffset) &&
|
(lbn + 1) * NFS_DIRBLKSIZ < np->n_direofoffset) &&
|
||||||
incore(&vp->v_bufobj, lbn + 1) == NULL) {
|
incore(&vp->v_bufobj, lbn + 1) == NULL) {
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
rabp = nfs_getcacheblk(vp, lbn + 1, NFS_DIRBLKSIZ, td);
|
rabp = nfs_getcacheblk(vp, lbn + 1, NFS_DIRBLKSIZ, td);
|
||||||
if (rabp) {
|
if (rabp) {
|
||||||
if ((rabp->b_flags & (B_CACHE|B_DELWRI)) == 0) {
|
if ((rabp->b_flags & (B_CACHE|B_DELWRI)) == 0) {
|
||||||
@@ -688,6 +696,7 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred)
|
|||||||
brelse(rabp);
|
brelse(rabp);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
NFSLOCKNODE(np);
|
||||||
}
|
}
|
||||||
/*
|
/*
|
||||||
* Unlike VREG files, whos buffer size ( bp->b_bcount ) is
|
* Unlike VREG files, whos buffer size ( bp->b_bcount ) is
|
||||||
@@ -704,6 +713,7 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred)
|
|||||||
n = lmin(uio->uio_resid, NFS_DIRBLKSIZ - bp->b_resid - on);
|
n = lmin(uio->uio_resid, NFS_DIRBLKSIZ - bp->b_resid - on);
|
||||||
if (np->n_direofoffset && n > np->n_direofoffset - uio->uio_offset)
|
if (np->n_direofoffset && n > np->n_direofoffset - uio->uio_offset)
|
||||||
n = np->n_direofoffset - uio->uio_offset;
|
n = np->n_direofoffset - uio->uio_offset;
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
break;
|
break;
|
||||||
default:
|
default:
|
||||||
printf(" ncl_bioread: type %x unexpected\n", vp->v_type);
|
printf(" ncl_bioread: type %x unexpected\n", vp->v_type);
|
||||||
|
|||||||
@@ -118,6 +118,7 @@ ncl_uninit(struct vfsconf *vfsp)
|
|||||||
#endif
|
#endif
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/* Returns with NFSLOCKNODE() held. */
|
||||||
void
|
void
|
||||||
ncl_dircookie_lock(struct nfsnode *np)
|
ncl_dircookie_lock(struct nfsnode *np)
|
||||||
{
|
{
|
||||||
@@ -125,7 +126,6 @@ ncl_dircookie_lock(struct nfsnode *np)
|
|||||||
while (np->n_flag & NDIRCOOKIELK)
|
while (np->n_flag & NDIRCOOKIELK)
|
||||||
(void) msleep(&np->n_flag, &np->n_mtx, PZERO, "nfsdirlk", 0);
|
(void) msleep(&np->n_flag, &np->n_mtx, PZERO, "nfsdirlk", 0);
|
||||||
np->n_flag |= NDIRCOOKIELK;
|
np->n_flag |= NDIRCOOKIELK;
|
||||||
NFSUNLOCKNODE(np);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
void
|
void
|
||||||
@@ -330,6 +330,7 @@ ncl_invaldir(struct vnode *vp)
|
|||||||
KASSERT(vp->v_type == VDIR, ("nfs: invaldir not dir"));
|
KASSERT(vp->v_type == VDIR, ("nfs: invaldir not dir"));
|
||||||
ncl_dircookie_lock(np);
|
ncl_dircookie_lock(np);
|
||||||
np->n_direofoffset = 0;
|
np->n_direofoffset = 0;
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
np->n_cookieverf.nfsuquad[0] = 0;
|
np->n_cookieverf.nfsuquad[0] = 0;
|
||||||
np->n_cookieverf.nfsuquad[1] = 0;
|
np->n_cookieverf.nfsuquad[1] = 0;
|
||||||
if (LIST_FIRST(&np->n_cookies))
|
if (LIST_FIRST(&np->n_cookies))
|
||||||
|
|||||||
@@ -2369,8 +2369,10 @@ nfs_readdir(struct vop_readdir_args *ap)
|
|||||||
/*
|
/*
|
||||||
* First, check for hit on the EOF offset cache
|
* First, check for hit on the EOF offset cache
|
||||||
*/
|
*/
|
||||||
|
NFSLOCKNODE(np);
|
||||||
if (np->n_direofoffset > 0 && uio->uio_offset >= np->n_direofoffset &&
|
if (np->n_direofoffset > 0 && uio->uio_offset >= np->n_direofoffset &&
|
||||||
(np->n_flag & NMODIFIED) == 0) {
|
(np->n_flag & NMODIFIED) == 0) {
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
if (VOP_GETATTR(vp, &vattr, ap->a_cred) == 0) {
|
if (VOP_GETATTR(vp, &vattr, ap->a_cred) == 0) {
|
||||||
NFSLOCKNODE(np);
|
NFSLOCKNODE(np);
|
||||||
if ((NFS_ISV4(vp) && np->n_change == vattr.va_filerev) ||
|
if ((NFS_ISV4(vp) && np->n_change == vattr.va_filerev) ||
|
||||||
@@ -2383,7 +2385,8 @@ nfs_readdir(struct vop_readdir_args *ap)
|
|||||||
} else
|
} else
|
||||||
NFSUNLOCKNODE(np);
|
NFSUNLOCKNODE(np);
|
||||||
}
|
}
|
||||||
}
|
} else
|
||||||
|
NFSUNLOCKNODE(np);
|
||||||
|
|
||||||
/*
|
/*
|
||||||
* NFS always guarantees that directory entries don't straddle
|
* NFS always guarantees that directory entries don't straddle
|
||||||
@@ -2436,6 +2439,7 @@ ncl_readdirrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred,
|
|||||||
* If there is no cookie, assume directory was stale.
|
* If there is no cookie, assume directory was stale.
|
||||||
*/
|
*/
|
||||||
ncl_dircookie_lock(dnp);
|
ncl_dircookie_lock(dnp);
|
||||||
|
NFSUNLOCKNODE(dnp);
|
||||||
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0);
|
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0);
|
||||||
if (cookiep) {
|
if (cookiep) {
|
||||||
cookie = *cookiep;
|
cookie = *cookiep;
|
||||||
@@ -2458,12 +2462,15 @@ ncl_readdirrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred,
|
|||||||
* We are now either at the end of the directory or have filled
|
* We are now either at the end of the directory or have filled
|
||||||
* the block.
|
* the block.
|
||||||
*/
|
*/
|
||||||
if (eof)
|
if (eof) {
|
||||||
|
NFSLOCKNODE(dnp);
|
||||||
dnp->n_direofoffset = uiop->uio_offset;
|
dnp->n_direofoffset = uiop->uio_offset;
|
||||||
else {
|
NFSUNLOCKNODE(dnp);
|
||||||
|
} else {
|
||||||
if (uiop->uio_resid > 0)
|
if (uiop->uio_resid > 0)
|
||||||
printf("EEK! readdirrpc resid > 0\n");
|
printf("EEK! readdirrpc resid > 0\n");
|
||||||
ncl_dircookie_lock(dnp);
|
ncl_dircookie_lock(dnp);
|
||||||
|
NFSUNLOCKNODE(dnp);
|
||||||
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1);
|
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1);
|
||||||
*cookiep = cookie;
|
*cookiep = cookie;
|
||||||
ncl_dircookie_unlock(dnp);
|
ncl_dircookie_unlock(dnp);
|
||||||
@@ -2496,6 +2503,7 @@ ncl_readdirplusrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred,
|
|||||||
* If there is no cookie, assume directory was stale.
|
* If there is no cookie, assume directory was stale.
|
||||||
*/
|
*/
|
||||||
ncl_dircookie_lock(dnp);
|
ncl_dircookie_lock(dnp);
|
||||||
|
NFSUNLOCKNODE(dnp);
|
||||||
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0);
|
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0);
|
||||||
if (cookiep) {
|
if (cookiep) {
|
||||||
cookie = *cookiep;
|
cookie = *cookiep;
|
||||||
@@ -2517,12 +2525,15 @@ ncl_readdirplusrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred,
|
|||||||
* We are now either at end of the directory or have filled the
|
* We are now either at end of the directory or have filled the
|
||||||
* the block.
|
* the block.
|
||||||
*/
|
*/
|
||||||
if (eof)
|
if (eof) {
|
||||||
|
NFSLOCKNODE(dnp);
|
||||||
dnp->n_direofoffset = uiop->uio_offset;
|
dnp->n_direofoffset = uiop->uio_offset;
|
||||||
else {
|
NFSUNLOCKNODE(dnp);
|
||||||
|
} else {
|
||||||
if (uiop->uio_resid > 0)
|
if (uiop->uio_resid > 0)
|
||||||
printf("EEK! readdirplusrpc resid > 0\n");
|
printf("EEK! readdirplusrpc resid > 0\n");
|
||||||
ncl_dircookie_lock(dnp);
|
ncl_dircookie_lock(dnp);
|
||||||
|
NFSUNLOCKNODE(dnp);
|
||||||
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1);
|
cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1);
|
||||||
*cookiep = cookie;
|
*cookiep = cookie;
|
||||||
ncl_dircookie_unlock(dnp);
|
ncl_dircookie_unlock(dnp);
|
||||||
|
|||||||
Reference in New Issue
Block a user