From: Mark Vitale Date: Tue, 30 Jun 2015 05:54:21 +0000 (-0400) Subject: SOLARIS: Avoid vcache locks when flushing pages for RO vnodes X-Git-Tag: openafs-devel-1_9_0~625 X-Git-Url: https://git.openafs.org/?p=openafs.git;a=commitdiff_plain;h=5e09a694ec2c0cd20f5dee500eff6bc3dd04c097 SOLARIS: Avoid vcache locks when flushing pages for RO vnodes We have multiple code paths that hold the following locks at the same time: - avc->lock for a vcache - The page lock for a page in 'avc' In order to avoid deadlocks, we need a consistent ordering for obtaining these two locks. The code in afs_putpage() currently obtains avc->lock before the page lock (Obtain*Lock is called before pvn_vplist_dirty). The code in afs_getpages() also obtains avc->lock before the page lock, but it does so in a loop for all requested pages (via pvn_getpages()). On the second iteration of that loop, it obtains avc->lock, and the page from the first iteration of the loop is still locked. Thus, it obtains a page lock before locking avc->lock in some cases. Since we have two code paths that obtain those two locks in a different order, a deadlock can occur. Fixing this properly requires changing at least one of those code paths, so the locks are taken in a consistent order. However, doing so is complex and will be done in a separate future commit. For this commit, we can avoid the deadlock for RO volumes by simply avoiding taking avc->lock in afs_putpages() at all while the pages are locked. Normally, we lock avc->lock because pvn_vplist_dirty() will call afs_putapage() for each dirty page (and afs_putapage() requires avc->lock held). But for RO volumes, we will have no dirty pages (because RO volumes cannot be written to from a client), and so afs_putapage() will never be called. So to avoid this deadlock issue for RO volumes, avoid taking avc->lock across the pvn_vplist_dirty() call in afs_putpage(). We now pass a dummy pageout callback function to pvn_vplist_dirty() instead, which should never be called, and which panics if it ever is. We still need to hold avc->lock a few other times during afs_putpage() for other minor reasons, but none of these hold page locks at the same time, so the deadlock issue is still avoided. [mmeffie: comments, and fix missing write lock, fix lock releases] [adeason: revised commit message] Change-Id: Iec11101147220828f319dae4027e7ab1f08483a6 Reviewed-on: https://gerrit.openafs.org/12247 Tested-by: BuildBot Reviewed-by: Andrew Deason Reviewed-by: Benjamin Kaduk --- diff --git a/src/afs/SOLARIS/osi_vnodeops.c b/src/afs/SOLARIS/osi_vnodeops.c index 7a8277a..9aa4f17 100644 --- a/src/afs/SOLARIS/osi_vnodeops.c +++ b/src/afs/SOLARIS/osi_vnodeops.c @@ -463,6 +463,19 @@ afs_GetOnePage(struct vnode *vp, u_offset_t off, u_int alen, u_int *protp, return code; } +/** + * Dummy pvn_vplist_dirty() handler for non-writable vnodes. + */ +static int +afs_never_putapage(struct vnode *vp, struct page *pages, u_offset_t * offp, + size_t * lenp, int flags, afs_ucred_t *credp) +{ + struct vcache *avc = VTOAFS(vp); + osi_Assert((avc->f.states & CRO) != 0); + osi_Panic("Dirty pages while flushing a read-only volume vnode."); + return EIO; /* unreachable */ +} + int afs_putpage(struct vnode *vp, offset_t off, u_int len, int flags, afs_ucred_t *cred) @@ -474,7 +487,7 @@ afs_putpage(struct vnode *vp, offset_t off, u_int len, int flags, afs_offs_t endPos; afs_int32 NPages = 0; u_offset_t toff = off; - int didWriteLock; + int didLock = 0; AFS_STATCNT(afs_putpage); if (vp->v_flag & VNOMAP) /* file doesn't allow mapping */ @@ -488,11 +501,11 @@ afs_putpage(struct vnode *vp, offset_t off, u_int len, int flags, (afs_int32) vp, ICL_TYPE_OFFSET, ICL_HANDLE_OFFSET(off), ICL_TYPE_INT32, (afs_int32) len, ICL_TYPE_LONG, (int)flags); avc = VTOAFS(vp); - ObtainSharedLock(&avc->lock, 247); - didWriteLock = 0; /* Get a list of modified (or whatever) pages */ if (len) { + ObtainSharedLock(&avc->lock, 247); + didLock = SHARED_LOCK; endPos = (afs_offs_t) off + len; /* position we're supposed to write up to */ while ((afs_offs_t) toff < endPos && (afs_offs_t) toff < avc->f.m.Length) { @@ -507,9 +520,9 @@ afs_putpage(struct vnode *vp, offset_t off, u_int len, int flags, if (!pages || !pvn_getdirty(pages, flags)) tlen = PAGESIZE; else { - if (!didWriteLock) { + if (didLock == SHARED_LOCK) { AFS_GLOCK(); - didWriteLock = 1; + didLock = WRITE_LOCK; UpgradeSToWLock(&avc->lock, 671); AFS_GUNLOCK(); } @@ -524,27 +537,50 @@ afs_putpage(struct vnode *vp, offset_t off, u_int len, int flags, AFS_GLOCK(); } } else { - if (!didWriteLock) { - UpgradeSToWLock(&avc->lock, 670); - didWriteLock = 1; + /* + * We normally arrive here due to a vm flush. + * + * If this vnode belongs to a writable volume, obtain a vcache lock + * then call pvn_vplist_dirty to free, invalidate, or to write out + * dirty pages with afs_putapage. The afs_putapage routine requires a + * vcache lock, so we obtain it here before any page locks are taken. + * This locking order is done to avoid deadlocking due to races with + * afs_getpage, which also takes vcache and page locks. + * + * If this vnode belongs to a non-writable volume, then it will not + * contain dirty pages, so we do not need to lock the vcache and since + * afs_putapage will not be called. Instead, forgo the vcache lock and + * call pvn_vplist_dirty to free, or invalidate pages. Pass a dummy + * page out handler to pvn_vplist_dirty which we do not expect to be + * called. Panic if the dummy handler is called, since something went + * horribly wrong. + */ + if ((avc->f.states & CRO) == 0) { + ObtainWriteLock(&avc->lock, 670); + didLock = WRITE_LOCK; } - AFS_GUNLOCK(); - code = pvn_vplist_dirty(vp, toff, afs_putapage, flags, cred); + if ((avc->f.states & CRO) == 0) + code = pvn_vplist_dirty(vp, toff, afs_putapage, flags, cred); + else + code = pvn_vplist_dirty(vp, toff, afs_never_putapage, flags, cred); AFS_GLOCK(); } if (code && !avc->vc_error) { - if (!didWriteLock) { + if (didLock == 0) { + ObtainWriteLock(&avc->lock, 668); + didLock = WRITE_LOCK; + } else if (didLock == SHARED_LOCK) { UpgradeSToWLock(&avc->lock, 669); - didWriteLock = 1; + didLock = WRITE_LOCK; } avc->vc_error = code; } - if (didWriteLock) + if (didLock == WRITE_LOCK) ReleaseWriteLock(&avc->lock); - else + else if (didLock == SHARED_LOCK) ReleaseSharedLock(&avc->lock); afs_Trace2(afs_iclSetp, CM_TRACE_PAGEOUTDONE, ICL_TYPE_LONG, code, ICL_TYPE_LONG, NPages);