Page MenuHomeFreeBSD

Various fixes related to VV_CROSSLOCK handling
AcceptedPublic

Authored by jah on Tue, Nov 22, 3:54 AM.

Details

Reviewers
kib
markj
mjg
pho
Summary

Commit 1:

Generalize the VV_CROSSLOCK logic in vfs_lookup()

When VV_CROSSLOCK is present, the lock for the vnode at the current
stage of lookup must be held across the VFS_ROOT() call for the
filesystem mounted at the vnode. Since VV_CROSSLOCK implies that
the root vnode reuses the already-held lock, the possibility for
recursion should be made clear in the flags passed to VFS_ROOT().

For cases in which the lock is held exclusive, this means passing
LK_CANRECURSE. For cases in which the lock is held shared, it
means clearing LK_NODDLKTREAT to allow VFS_ROOT() to potentially
recurse on the shared lock even in the presence of an exclusive
waiter.

That the existing code works for unionfs is due to a coincidence
of the current unionfs implementation.

Commit 2:

unionfs: allow recursion on covered vnode lock during mount/unmount

When taking the covered vnode lock during mount and unmount operations,
specify LK_CANRECURSE as the existing lock state of the covered vnode
is not guaranteed (AFAIK) either by assertion or documentation for
these code paths.

For the mount path, this is done only for completeness as the covered
vnode lock is not currently held when VFS_MOUNT() is called.
For the unmount path, the covered vnode is currently held across
VFS_UNMOUNT(), and the existing code only happens to work when unionfs
is mounted atop FFS because FFS sets LO_RECURSABLE on its vnode locks.

This of course doesn't cover a hypothetical case in which the covered
vnode may be held shared, but for the mount and unmount paths such a
scenario seems unlikely to materialize.

Commit 3:

nullfs: adopt VV_CROSSLOCK

When the lower filesystem directory hierarchy is the same as the nullfs
mount point (admittedly not likely to be a useful situation in
practice), nullfs is subject to the exact deadlock between the busy
count drain and the covered vnode lock that VV_CROSSLOCK is intended
to address.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48457
Build 45343: arc lint + arc unit

Event Timeline

jah requested review of this revision.Tue, Nov 22, 3:54 AM
sys/fs/nullfs/null_vfsops.c
135

Is there a situation in which this 'isvnunlocked' logic is still needed? I don't see an apparent need for given the VFS today; it appears to date back to a 25-year-old commit (https://cgit.freebsd.org/src/commit/sys/fs/nullfs/null_vfsops.c?id=f85e8fc5ca2828207b777bd8dff8191941dd241a), when I would imagine the VFS locking scheme looked very different.

sys/fs/nullfs/null_vfsops.c
135

I think it is reverse, the logic should be made unconditional. Generally you cannot call namei() while having random vnode locked.

sys/kern/vfs_lookup.c
1281

extra ()

sys/fs/nullfs/null_vfsops.c
135

Right, I was tempted to say that we could replace the 'isvnunlocked' logic with an assertion that the covered node isn't locked, since it appears that vfs_domount_first()/vfs_domount_update() don't hold the covered node lock across VFS_MOUNT(). But it looks as though vfs_remount_ro() does. Also I'd be somewhat worried about the locking in vfs_domount_first()/vfs_domount_update() changing in the future, since covered node locking assumptions don't seem to be documented anywhere.

sys/fs/nullfs/null_vfsops.c
205

I don't think this lock/unlock is actually necessary; lowerrootvp should be locked here, because it was returned locked from namei(), and null_nodeget() then transferred the lock to the new nullfs node. I think I'd prefer to leave it here for clarity though.

This revision is now accepted and ready to land.Tue, Nov 22, 10:54 PM