Page MenuHomeFreeBSD
Differential D19596

VOP_INACTIVE(9): clarify wording
ClosedPublic
Actions

Authored by asomers on Mar 15 2019, 4:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 12:57 PM
Unknown Object (File)
Fri, Nov 22, 7:36 AM
Unknown Object (File)
Tue, Nov 12, 2:01 PM
Unknown Object (File)
Tue, Oct 29, 9:41 PM
Unknown Object (File)
Sep 29 2024, 11:22 PM
Unknown Object (File)
Sep 24 2024, 9:08 AM
Unknown Object (File)
Sep 18 2024, 8:10 PM
Unknown Object (File)
Sep 18 2024, 8:10 PM
View All 68 Files
Subscribers
0mp
cem
imp

Details

Reviewers
kib
0mp
Group Reviewers
manpages
Commits
rS348461: MFC r345202:
rS347478: MFC r345202:
rS345202: VOP_INACTIVE(9): clarify wording
rS345198: VOP_INACTIVE(9): clarify wording
Summary

VOP_INACTIVE(9): clarify wording

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 23117
Build 22171: arc lint + arc unit

Event Timeline

asomers created this revision.Mar 15 2019, 4:27 PM
Herald added a reviewer: manpages. · View Herald TranscriptMar 15 2019, 4:27 PM
Herald added a subscriber: imp. · View Herald Transcript
Harbormaster completed remote builds in B23111: Diff 55109.Mar 15 2019, 4:27 PM
kib accepted this revision.Mar 15 2019, 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.Mar 15 2019, 4:35 PM
asomers updated this revision to Diff 55113.Mar 15 2019, 4:38 PM

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

This revision now requires review to proceed.Mar 15 2019, 4:38 PM
Harbormaster completed remote builds in B23115: Diff 55113.Mar 15 2019, 4:38 PM
asomers added a subscriber: cem.Mar 15 2019, 5:02 PM
0mp accepted this revision.Mar 15 2019, 5:25 PM
0mp added a subscriber: 0mp.

ok from manpages

This revision is now accepted and ready to land.Mar 15 2019, 5:25 PM
kib added inline comments.Mar 15 2019, 5:35 PM
share/man/man9/VOP_INACTIVE.9
61

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.Mar 15 2019, 5:40 PM

Respond to kib's criticism

This revision now requires review to proceed.Mar 15 2019, 5:40 PM
Harbormaster completed remote builds in B23117: Diff 55115.Mar 15 2019, 5:40 PM
cem added inline comments.Mar 15 2019, 5:43 PM
share/man/man9/VOP_INACTIVE.9
53

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

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.Mar 15 2019, 5:44 PM
share/man/man9/VOP_INACTIVE.9
61

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

asomers marked an inline comment as done.Mar 15 2019, 5:44 PM
asomers added inline comments.
share/man/man9/VOP_INACTIVE.9
60–61

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.Mar 15 2019, 5:48 PM
share/man/man9/VOP_INACTIVE.9
61

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.Mar 15 2019, 5:50 PM
share/man/man9/VOP_INACTIVE.9
60–61

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.Mar 15 2019, 6:05 PM
share/man/man9/VOP_INACTIVE.9
61

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

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

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

Use kib's preferred wording.

Harbormaster completed remote builds in B23119: Diff 55118.Mar 15 2019, 6:08 PM
cem added inline comments.Mar 15 2019, 6:11 PM
share/man/man9/VOP_INACTIVE.9
61

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.Mar 15 2019, 6:45 PM
share/man/man9/VOP_INACTIVE.9
61

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.Mar 15 2019, 6:46 PM
This revision is now accepted and ready to land.Mar 15 2019, 6:46 PM
Closed by commit rS345198: VOP_INACTIVE(9): clarify wording (authored by asomers). · Explain WhyMar 15 2019, 6:51 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
share/
man/
man9/
10 lines

Diff 55115

View Options

share/man/man9/VOP_INACTIVE.9

Loading...