Changeset View
Standalone View
sys/kern/vfs_lookup.c
Show First 20 Lines • Show All 109 Lines • ▼ Show 20 Lines | crossmp_vop_lock1(struct vop_lock1_args *ap) | ||||
vp = ap->a_vp; | vp = ap->a_vp; | ||||
lk = vp->v_vnlock; | lk = vp->v_vnlock; | ||||
flags = ap->a_flags; | flags = ap->a_flags; | ||||
file = ap->a_file; | file = ap->a_file; | ||||
line = ap->a_line; | line = ap->a_line; | ||||
if ((flags & LK_SHARED) == 0) | if ((flags & LK_SHARED) == 0) | ||||
panic("invalid lock request for crossmp"); | panic("invalid lock request for crossmp"); | ||||
markj: Should we perhaps make this a KASSERT? | |||||
Done Inline Actionssounds good to me. jah: sounds good to me. | |||||
WITNESS_CHECKORDER(&lk->lock_object, LOP_NEWORDER, file, line, | WITNESS_CHECKORDER(&lk->lock_object, LOP_NEWORDER, file, line, | ||||
flags & LK_INTERLOCK ? &VI_MTX(vp)->lock_object : NULL); | flags & LK_INTERLOCK ? &VI_MTX(vp)->lock_object : NULL); | ||||
WITNESS_LOCK(&lk->lock_object, 0, file, line); | WITNESS_LOCK(&lk->lock_object, 0, file, line); | ||||
if ((flags & LK_INTERLOCK) != 0) | if ((flags & LK_INTERLOCK) != 0) | ||||
Not Done Inline ActionsIf you are dropping support for LK_INTERLOCK for crossmp vnode, then should it be asserted that LK_INTERLOCK is never specified for it? Also, I think that crossmp_vop_lock1() changes should be a separate commit. kib: If you are dropping support for LK_INTERLOCK for crossmp vnode, then should it be asserted that… | |||||
Done Inline ActionsI need to re-test, because it's been so long since I wrote this patch that I no longer remember the exact set of issues I was having with this function. But I do recall that dropping the interlock here did not make sense to me, especially given that the corresponding vop_unlock doesn't re-acquire it, so it seemed as though that would cause problems for any caller holding the interlock. Is there some non-obvious (to me) reason why this would have been done? jah: I need to re-test, because it's been so long since I wrote this patch that I no longer remember… | |||||
Done Inline ActionsEDIT: nevermind, I'd forgotten that dropping the interlock is the documented behavior of VOP_LOCK(). jah: EDIT: nevermind, I'd forgotten that dropping the interlock is the documented behavior of… | |||||
Done Inline ActionsI've restored support for LK_INTERLOCK, this does not cause any issues in my testing. jah: I've restored support for LK_INTERLOCK, this does not cause any issues in my testing.
The… | |||||
Not Done Inline ActionsThe directive is useful because it records a 'logical' lock owner, visible for instance with the db 'show alllocks' command. I believe actual lockmgr() call was removed for the sake of optimization. So I am not sure this is very useful now. kib: The directive is useful because it records a 'logical' lock owner, visible for instance with… | |||||
Done Inline ActionsWould you prefer that I also remove the WITNESS_LOCK and WITNESS_UNLOCK calls here and in crossmp_vop_unlock (as part of a separate commit of course)? jah: Would you prefer that I also remove the WITNESS_LOCK and WITNESS_UNLOCK calls here and in… | |||||
Not Done Inline ActionsI suspect that yes, the calls have to be removed. kib: I suspect that yes, the calls have to be removed. | |||||
VI_UNLOCK(vp); | VI_UNLOCK(vp); | ||||
LOCK_LOG_LOCK("SLOCK", &lk->lock_object, 0, 0, ap->a_file, ap->a_line); | LOCK_LOG_LOCK("SLOCK", &lk->lock_object, 0, 0, ap->a_file, ap->a_line); | ||||
return (0); | return (0); | ||||
} | } | ||||
static int | static int | ||||
crossmp_vop_unlock(struct vop_unlock_args *ap) | crossmp_vop_unlock(struct vop_unlock_args *ap) | ||||
{ | { | ||||
▲ Show 20 Lines • Show All 764 Lines • ▼ Show 20 Lines | vfs_lookup(struct nameidata *ndp) | ||||
int wantparent; /* 1 => wantparent or lockparent flag */ | int wantparent; /* 1 => wantparent or lockparent flag */ | ||||
int rdonly; /* lookup read-only flag bit */ | int rdonly; /* lookup read-only flag bit */ | ||||
int error = 0; | int error = 0; | ||||
int dpunlocked = 0; /* dp has already been unlocked */ | int dpunlocked = 0; /* dp has already been unlocked */ | ||||
int relookup = 0; /* do not consume the path component */ | int relookup = 0; /* do not consume the path component */ | ||||
struct componentname *cnp = &ndp->ni_cnd; | struct componentname *cnp = &ndp->ni_cnd; | ||||
int lkflags_save; | int lkflags_save; | ||||
int ni_dvp_unlocked; | int ni_dvp_unlocked; | ||||
int crosslkflags; | |||||
bool crosslock; | |||||
/* | /* | ||||
* Setup: break out flag bits into variables. | * Setup: break out flag bits into variables. | ||||
*/ | */ | ||||
ni_dvp_unlocked = 0; | ni_dvp_unlocked = 0; | ||||
wantparent = cnp->cn_flags & (LOCKPARENT | WANTPARENT); | wantparent = cnp->cn_flags & (LOCKPARENT | WANTPARENT); | ||||
KASSERT(cnp->cn_nameiop == LOOKUP || wantparent, | KASSERT(cnp->cn_nameiop == LOOKUP || wantparent, | ||||
("CREATE, DELETE, RENAME require LOCKPARENT or WANTPARENT.")); | ("CREATE, DELETE, RENAME require LOCKPARENT or WANTPARENT.")); | ||||
▲ Show 20 Lines • Show All 315 Lines • ▼ Show 20 Lines | #endif | ||||
dp = ndp->ni_vp; | dp = ndp->ni_vp; | ||||
/* | /* | ||||
* Check to see if the vnode has been mounted on; | * Check to see if the vnode has been mounted on; | ||||
* if so find the root of the mounted filesystem. | * if so find the root of the mounted filesystem. | ||||
*/ | */ | ||||
while (dp->v_type == VDIR && (mp = dp->v_mountedhere) && | while (dp->v_type == VDIR && (mp = dp->v_mountedhere) && | ||||
(cnp->cn_flags & NOCROSSMOUNT) == 0) { | (cnp->cn_flags & NOCROSSMOUNT) == 0) { | ||||
crosslock = (dp->v_vflag & VV_CROSSLOCK) != 0; | |||||
Not Done Inline ActionsOuter () are not needed kib: Outer () are not needed | |||||
crosslkflags = compute_cn_lkflags(mp, cnp->cn_lkflags, | |||||
cnp->cn_flags); | |||||
if (__predict_false(crosslock) && | |||||
(crosslkflags & LK_EXCLUSIVE) != 0 && | |||||
VOP_ISLOCKED(dp) != LK_EXCLUSIVE) { | |||||
vn_lock(dp, LK_UPGRADE | LK_RETRY); | |||||
if (VN_IS_DOOMED(dp)) { | |||||
error = ENOENT; | |||||
goto bad2; | |||||
} | |||||
} | |||||
if (vfs_busy(mp, 0)) | if (vfs_busy(mp, 0)) | ||||
Not Done Inline ActionsWould you please update the comment above vfs_busy()'s definition explaining this new special case? markj: Would you please update the comment above vfs_busy()'s definition explaining this new special… | |||||
Done Inline Actionsi'll definitely do that before committing, if in fact everyone is ok with this approach. jah: i'll definitely do that before committing, if in fact everyone is ok with this approach. | |||||
Not Done Inline ActionsI don't think there is any need to pessimize the common case here. I would argue the thing to do here is to:
mjg: I don't think there is any need to pessimize the common case here.
I would argue the thing to… | |||||
Not Done Inline Actionsinstead of checking dp->v_type == VDIR && (mp = dp->v_mountedhere) you can (vn_irflag_read(vp) & VIRF_MOUNTPOINT) != 0) mjg: instead of checking dp->v_type == VDIR && (mp = dp->v_mountedhere) you can (vn_irflag_read(vp)… | |||||
Done Inline ActionsThis point, as well as your points 1) and 2) above are definitely good suggestions, but I think they should be done as part of a separate change. As for points 3) and 4), in this case NOWAIT vfs_busy() wouldn't help. The deadlock happens after vfs_busy() has already successfully completed (almost certainly without blocking), while the following call to VFS_ROOT() blocks while trying to lock the root vnode. At the same time, dounmount() on another thread blocks waiting for the mp busy count to drain while holding the covered vnode lock (which in this case happens to be the same as the root vnode lock being waited on by the first thread). On the other hand, I did mention this alternative approach in my earlier comment, which would reqiure NOWAIT vfs_busy():
jah: This point, as well as your points 1) and 2) above are definitely good suggestions, but I think… | |||||
continue; | continue; | ||||
if (__predict_true(!crosslock)) | |||||
vput(dp); | vput(dp); | ||||
if (dp != ndp->ni_dvp) | if (dp != ndp->ni_dvp) | ||||
vput(ndp->ni_dvp); | vput(ndp->ni_dvp); | ||||
else | else | ||||
vrele(ndp->ni_dvp); | vrele(ndp->ni_dvp); | ||||
vrefact(vp_crossmp); | vrefact(vp_crossmp); | ||||
ndp->ni_dvp = vp_crossmp; | ndp->ni_dvp = vp_crossmp; | ||||
error = VFS_ROOT(mp, compute_cn_lkflags(mp, cnp->cn_lkflags, | error = VFS_ROOT(mp, crosslkflags, &tdp); | ||||
cnp->cn_flags), &tdp); | |||||
vfs_unbusy(mp); | vfs_unbusy(mp); | ||||
if (__predict_false(crosslock)) | |||||
vput(dp); | |||||
if (vn_lock(vp_crossmp, LK_SHARED | LK_NOWAIT)) | if (vn_lock(vp_crossmp, LK_SHARED | LK_NOWAIT)) | ||||
panic("vp_crossmp exclusively locked or reclaimed"); | panic("vp_crossmp exclusively locked or reclaimed"); | ||||
if (error) { | if (error) { | ||||
dpunlocked = 1; | dpunlocked = 1; | ||||
goto bad2; | goto bad2; | ||||
} | } | ||||
ndp->ni_vp = dp = tdp; | ndp->ni_vp = dp = tdp; | ||||
} | } | ||||
▲ Show 20 Lines • Show All 451 Lines • Show Last 20 Lines |
Should we perhaps make this a KASSERT?