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)
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
Unknown Object (File)
Sep 18 2024, 8:09 PM
Unknown Object (File)
Sep 18 2024, 8:09 PM
View All 66 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 Not Applicable
Unit
Tests Not Applicable

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 ↗(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.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 ↗(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.Mar 15 2019, 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.Mar 15 2019, 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.Mar 15 2019, 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.Mar 15 2019, 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.Mar 15 2019, 6:05 PM
share/man/man9/VOP_INACTIVE.9
61 ↗(On Diff #55115)

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

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.

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 ↗(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.Mar 15 2019, 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.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
projects/
fuse2/
share/
man/
man9/
11 lines

Diff 55119

View Options

projects/fuse2/share/man/man9/VOP_INACTIVE.9

Loading...