From owner-dev-commits-src-branches@freebsd.org Mon Mar 15 20:50:04 2021 Return-Path: Delivered-To: dev-commits-src-branches@mailman.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.nyi.freebsd.org (Postfix) with ESMTP id 769175AF6B8; Mon, 15 Mar 2021 20:50:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4DzpT82cVsz3wWr; Mon, 15 Mar 2021 20:50:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4C9FE171F1; Mon, 15 Mar 2021 20:50:04 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 12FKo441009229; Mon, 15 Mar 2021 20:50:04 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 12FKo43t009225; Mon, 15 Mar 2021 20:50:04 GMT (envelope-from git) Date: Mon, 15 Mar 2021 20:50:04 GMT Message-Id: <202103152050.12FKo43t009225@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Rick Macklem Subject: git: a1224bee90c6 - stable/12 - nfsclient: add nfs node locking around uses of n_direofoffset MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: rmacklem X-Git-Repository: src X-Git-Refname: refs/heads/stable/12 X-Git-Reftype: branch X-Git-Commit: a1224bee90c6a54909d279ec631ea2ad8241ddf4 Auto-Submitted: auto-generated X-BeenThere: dev-commits-src-branches@freebsd.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Commits to the stable branches of the FreeBSD src repository List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Mar 2021 20:50:04 -0000 The branch stable/12 has been updated by rmacklem: URL: https://cgit.FreeBSD.org/src/commit/?id=a1224bee90c6a54909d279ec631ea2ad8241ddf4 commit a1224bee90c6a54909d279ec631ea2ad8241ddf4 Author: Rick Macklem AuthorDate: 2021-02-28 22:53:54 +0000 Commit: Rick Macklem CommitDate: 2021-03-15 19:48:37 +0000 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. (cherry picked from commit 15bed8c46b32dec19e922cb89e12c8970867a303) --- sys/fs/nfsclient/nfs_clbio.c | 10 ++++++++++ sys/fs/nfsclient/nfs_clsubs.c | 3 ++- sys/fs/nfsclient/nfs_clvnops.c | 21 ++++++++++++++++----- 3 files changed, 28 insertions(+), 6 deletions(-) diff --git a/sys/fs/nfsclient/nfs_clbio.c b/sys/fs/nfsclient/nfs_clbio.c index 01595eea414b..5ba75491a800 100644 --- a/sys/fs/nfsclient/nfs_clbio.c +++ b/sys/fs/nfsclient/nfs_clbio.c @@ -584,11 +584,14 @@ ncl_bioread(struct vnode *vp, struct uio *uio, int ioflag, struct ucred *cred) break; case VDIR: NFSINCRGLOBAL(nfsstatsv1.biocache_readdirs); + NFSLOCKNODE(np); if (np->n_direofoffset && uio->uio_offset >= np->n_direofoffset) { + NFSUNLOCKNODE(np); error = 0; goto out; } + NFSUNLOCKNODE(np); lbn = (uoff_t)uio->uio_offset / NFS_DIRBLKSIZ; on = uio->uio_offset & (NFS_DIRBLKSIZ - 1); 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!). */ for (i = 0; i <= lbn && !error; i++) { + NFSLOCKNODE(np); if (np->n_direofoffset && (i * NFS_DIRBLKSIZ) >= np->n_direofoffset) { + NFSUNLOCKNODE(np); error = 0; goto out; } + NFSUNLOCKNODE(np); bp = nfs_getcacheblk(vp, i, NFS_DIRBLKSIZ, td); if (!bp) { 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 * directory offset cookie of the next block.) */ + NFSLOCKNODE(np); if (nmp->nm_readahead > 0 && (bp->b_flags & B_INVAL) == 0 && (np->n_direofoffset == 0 || (lbn + 1) * NFS_DIRBLKSIZ < np->n_direofoffset) && incore(&vp->v_bufobj, lbn + 1) == NULL) { + NFSUNLOCKNODE(np); rabp = nfs_getcacheblk(vp, lbn + 1, NFS_DIRBLKSIZ, td); if (rabp) { 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); } } + NFSLOCKNODE(np); } /* * 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); if (np->n_direofoffset && n > np->n_direofoffset - uio->uio_offset) n = np->n_direofoffset - uio->uio_offset; + NFSUNLOCKNODE(np); break; default: printf(" ncl_bioread: type %x unexpected\n", vp->v_type); diff --git a/sys/fs/nfsclient/nfs_clsubs.c b/sys/fs/nfsclient/nfs_clsubs.c index 1c69d935f25f..6f6df7f938d3 100644 --- a/sys/fs/nfsclient/nfs_clsubs.c +++ b/sys/fs/nfsclient/nfs_clsubs.c @@ -118,6 +118,7 @@ ncl_uninit(struct vfsconf *vfsp) #endif } +/* Returns with NFSLOCKNODE() held. */ void ncl_dircookie_lock(struct nfsnode *np) { @@ -125,7 +126,6 @@ ncl_dircookie_lock(struct nfsnode *np) while (np->n_flag & NDIRCOOKIELK) (void) msleep(&np->n_flag, &np->n_mtx, PZERO, "nfsdirlk", 0); np->n_flag |= NDIRCOOKIELK; - NFSUNLOCKNODE(np); } void @@ -330,6 +330,7 @@ ncl_invaldir(struct vnode *vp) KASSERT(vp->v_type == VDIR, ("nfs: invaldir not dir")); ncl_dircookie_lock(np); np->n_direofoffset = 0; + NFSUNLOCKNODE(np); np->n_cookieverf.nfsuquad[0] = 0; np->n_cookieverf.nfsuquad[1] = 0; if (LIST_FIRST(&np->n_cookies)) diff --git a/sys/fs/nfsclient/nfs_clvnops.c b/sys/fs/nfsclient/nfs_clvnops.c index 9fbf9173434f..fadcf26c686a 100644 --- a/sys/fs/nfsclient/nfs_clvnops.c +++ b/sys/fs/nfsclient/nfs_clvnops.c @@ -2324,8 +2324,10 @@ nfs_readdir(struct vop_readdir_args *ap) /* * First, check for hit on the EOF offset cache */ + NFSLOCKNODE(np); if (np->n_direofoffset > 0 && uio->uio_offset >= np->n_direofoffset && (np->n_flag & NMODIFIED) == 0) { + NFSUNLOCKNODE(np); if (VOP_GETATTR(vp, &vattr, ap->a_cred) == 0) { NFSLOCKNODE(np); if ((NFS_ISV4(vp) && np->n_change == vattr.va_filerev) || @@ -2338,7 +2340,8 @@ nfs_readdir(struct vop_readdir_args *ap) } else NFSUNLOCKNODE(np); } - } + } else + NFSUNLOCKNODE(np); /* * NFS always guarantees that directory entries don't straddle @@ -2391,6 +2394,7 @@ ncl_readdirrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred, * If there is no cookie, assume directory was stale. */ ncl_dircookie_lock(dnp); + NFSUNLOCKNODE(dnp); cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0); if (cookiep) { cookie = *cookiep; @@ -2413,12 +2417,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 * the block. */ - if (eof) + if (eof) { + NFSLOCKNODE(dnp); dnp->n_direofoffset = uiop->uio_offset; - else { + NFSUNLOCKNODE(dnp); + } else { if (uiop->uio_resid > 0) printf("EEK! readdirrpc resid > 0\n"); ncl_dircookie_lock(dnp); + NFSUNLOCKNODE(dnp); cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1); *cookiep = cookie; ncl_dircookie_unlock(dnp); @@ -2451,6 +2458,7 @@ ncl_readdirplusrpc(struct vnode *vp, struct uio *uiop, struct ucred *cred, * If there is no cookie, assume directory was stale. */ ncl_dircookie_lock(dnp); + NFSUNLOCKNODE(dnp); cookiep = ncl_getcookie(dnp, uiop->uio_offset, 0); if (cookiep) { cookie = *cookiep; @@ -2472,12 +2480,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 * the block. */ - if (eof) + if (eof) { + NFSLOCKNODE(dnp); dnp->n_direofoffset = uiop->uio_offset; - else { + NFSUNLOCKNODE(dnp); + } else { if (uiop->uio_resid > 0) printf("EEK! readdirplusrpc resid > 0\n"); ncl_dircookie_lock(dnp); + NFSUNLOCKNODE(dnp); cookiep = ncl_getcookie(dnp, uiop->uio_offset, 1); *cookiep = cookie; ncl_dircookie_unlock(dnp);