Page MenuHomeFreeBSD

VOP_INACTIVE(9): clarify wording
ClosedPublic

Authored by asomers on Fri, Mar 15, 4:27 PM.

Details

Summary

VOP_INACTIVE(9): clarify wording

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

asomers created this revision.Fri, Mar 15, 4:27 PM
kib accepted this revision.Fri, Mar 15, 4:35 PM

I believe a note that VOP_INACTIVE() call is not guaranteed is also due.

This revision is now accepted and ready to land.Fri, Mar 15, 4:35 PM
asomers updated this revision to Diff 55113.Fri, Mar 15, 4:38 PM

Add note that there is no guarantee that VOP_INACTIVE will be called

This revision now requires review to proceed.Fri, Mar 15, 4:38 PM
asomers added a subscriber: cem.Fri, Mar 15, 5:02 PM
0mp accepted this revision.Fri, Mar 15, 5:25 PM
0mp added a subscriber: 0mp.

ok from manpages

This revision is now accepted and ready to land.Fri, Mar 15, 5:25 PM
kib added inline comments.Fri, Mar 15, 5:35 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

I do not know what unclean unmounts are. It is not guaranteed to be called because e.g. it might be called while only owning a shared vnode lock, and non-sleeping upgrade failed due to other shared lockers.

asomers updated this revision to Diff 55115.Fri, Mar 15, 5:40 PM

Respond to kib's criticism

This revision now requires review to proceed.Fri, Mar 15, 5:40 PM
cem added inline comments.Fri, Mar 15, 5:43 PM
share/man/man9/VOP_INACTIVE.9
53 ↗(On Diff #55113)

This seems a little less clear than before; it could be interpreted as "may be called sometimes when the kernel is still using the vnode."

I might leave this sentence alone, but bring up your "no guarantee" sentence to follow directly after it.

60–61 ↗(On Diff #55113)

Hm, can you clarify this sentence? It's called as part of vgonel() even during forced umount. So what is an unclean unmount in this context?

cem added inline comments.Fri, Mar 15, 5:44 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

@kib, which path would a VOP_INACTIVE() caller only hold a shared lock?

asomers marked an inline comment as done.Fri, Mar 15, 5:44 PM
asomers added inline comments.
share/man/man9/VOP_INACTIVE.9
60–61 ↗(On Diff #55113)

I meant things like power loss, crash, etc. The filesystem musn't rely exclusively on VOP_INACTIVE to reclaim space in such cases.

cem added inline comments.Fri, Mar 15, 5:48 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

Oh, vput/vunref may have a failed lock upgrade. But they still set VI_OWEINACT, so eventually the last other holder should invoke VOP_INACTIVE (or vgonel will).

cem added inline comments.Fri, Mar 15, 5:50 PM
share/man/man9/VOP_INACTIVE.9
60–61 ↗(On Diff #55113)

We generally only document API guarantees with the understanding that they only apply if power is consistent and the system does not crash. If we added boilerplate about power loss/crash to every API it would bloat the manuals excessively and make it more difficult to read relevant information.

I don't think there's any particular reason these APIs are different in that respect.

kib added inline comments.Fri, Mar 15, 6:05 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

The vnode could be vref-ed while VI_OWEINACT is set, and then if the lock mode is shared, it is still not inactivated. So no, there is no guarantee that vop_inactive necessary called before reclaim.

61 ↗(On Diff #55115)

'while the vnode lock could not be upgraded to exclusive without sleeping'.

asomers updated this revision to Diff 55118.Fri, Mar 15, 6:08 PM
asomers marked an inline comment as done.

Use kib's preferred wording.

cem added inline comments.Fri, Mar 15, 6:11 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

The vnode could be vref-ed while VI_OWEINACT is set, and then if the lock mode is shared, it is still not inactivated. So no, there is no guarantee that vop_inactive necessary called before reclaim.

I don't follow. If it is vrefed, it gains a non-zero usecount and is marked VI_ACTIVE. In vgonel(), it will still be vinactive -> VOP_INACTIVE()ed before RECLAIM.

kib added inline comments.Fri, Mar 15, 6:45 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55113)

VOP_INACTIVE() gerald is to be called on each v_usecount 1->0 transition. This is not guaranteed at least due to the shared locking.

The fact that we might be calling VOP_INACTIVE on next round of 1->0 transition or right before VOP_RECLAIM() call in vgonel() still does not satisfy the promise of calling VOP_INACTIVE on previous v_usecount zeroing.

kib accepted this revision.Fri, Mar 15, 6:46 PM
This revision is now accepted and ready to land.Fri, Mar 15, 6:46 PM
This revision was automatically updated to reflect the committed changes.