Page MenuHomeFreeBSD
Differential D14778

Elide the object lock in vfs_vmio_truncate() in the common case.
ClosedPublic
Actions

Authored by markj on Mar 20 2018, 9:17 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 19 2024, 8:35 AM
Unknown Object (File)
Feb 23 2024, 7:19 PM
Unknown Object (File)
Jan 3 2024, 5:45 PM
Unknown Object (File)
Dec 20 2023, 2:52 AM
Unknown Object (File)
Dec 5 2023, 11:22 AM
Unknown Object (File)
Nov 6 2023, 10:47 PM
Unknown Object (File)
Oct 12 2023, 4:50 AM
Unknown Object (File)
Sep 20 2023, 4:42 AM
View All 25 Files
Subscribers
imp

Details

Reviewers
alc
kib
jeff
Commits
rS331325: Elide the object lock in the common case in vfs_vmio_unwire().
Summary

Rather attempting to free invalid pages, accelerate their reclamation
by moving them near the head of the inactive queue. With this change,
bufspace threads don't touch the object lock.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj created this revision.Mar 20 2018, 9:17 PM
Harbormaster completed remote builds in B15673: Diff 40518.Mar 20 2018, 9:17 PM
markj added reviewers: alc, kib, jeff.Mar 20 2018, 9:21 PM
jeff accepted this revision.Mar 20 2018, 9:23 PM
jeff added inline comments.
sys/kern/vfs_bio.c
2919 ↗(On Diff #40518)

I think this can only race when multiple bufs reference the same page. It might be worth commenting on that specifically.

This revision is now accepted and ready to land.Mar 20 2018, 9:23 PM
kib accepted this revision.Mar 20 2018, 9:47 PM
alc accepted this revision.Mar 21 2018, 7:55 AM
alc added inline comments.
sys/kern/vfs_bio.c
2919 ↗(On Diff #40518)

Generally, what I like to see in these cases is a comment explaining why the race is benign.

markj updated this revision to Diff 40538.Mar 21 2018, 2:56 PM

Update comment and make the control flow in vfs_vmio_unwire() a bit more explicit.

This revision now requires review to proceed.Mar 21 2018, 2:56 PM
Harbormaster completed remote builds in B15685: Diff 40538.Mar 21 2018, 2:56 PM
alc added inline comments.Mar 21 2018, 4:43 PM
sys/kern/vfs_bio.c
2931 ↗(On Diff #40538)

How is the buffer lock stopping the VM system from changing the state of the valid field? Suppose that the containing object is logically mapped in an address space. In that case, it's not clear to me that everything that might affect the valid field is going to acquire a buffer lock.

markj added inline comments.Mar 21 2018, 4:54 PM
sys/kern/vfs_bio.c
2931 ↗(On Diff #40538)

It isn't. I guess "most cases" is misleading, but I added it because of the cases that you and Jeff described.

How about, "The valid bits will be stable unless the page is mapped or referenced by multiple buffers, and we expect races to be rare in those cases."

alc added inline comments.Mar 21 2018, 5:03 PM
sys/kern/vfs_bio.c
2931 ↗(On Diff #40538)

Okay. I would suggest swapping the second and third sentences. In other words, I would talk about the likelihood of the race before talking about the consequences.

markj updated this revision to Diff 40552.Mar 21 2018, 5:06 PM
  • Update wording in the comment.
Harbormaster completed remote builds in B15691: Diff 40552.Mar 21 2018, 5:06 PM
alc accepted this revision.Mar 21 2018, 5:11 PM
This revision is now accepted and ready to land.Mar 21 2018, 5:11 PM
kib added inline comments.Mar 21 2018, 6:52 PM
sys/kern/vfs_bio.c
2928 ↗(On Diff #40552)

What do you mean by 'mapped' there ? And what is the difference with 'referenced' ?

markj added inline comments.Mar 21 2018, 6:58 PM
sys/kern/vfs_bio.c
2928 ↗(On Diff #40552)

"mapped into a process' address space" as opposed to having multiple buffers which contain pointers to the same page. I thought the latter is possible e.g., with sub-page FFS block fragments.

kib added inline comments.Mar 21 2018, 7:10 PM
sys/kern/vfs_bio.c
2928 ↗(On Diff #40552)

Then I miss something. Mapped page must be valid.

Other part, about sub-page fragments, sounds correct.

markj added inline comments.Mar 21 2018, 7:16 PM
sys/kern/vfs_bio.c
2928 ↗(On Diff #40552)

"in the process of being mapped" is closer to what I meant. I think there is a possibility (perhaps impossible in practice) that a concurrent fault might mark the page valid.

kib added inline comments.Mar 21 2018, 7:30 PM
sys/kern/vfs_bio.c
2928 ↗(On Diff #40552)

So it is really any invalid page. But the page is still fine.

markj updated this revision to Diff 40569.Mar 21 2018, 8:20 PM

Clarify wording a bit.

This revision now requires review to proceed.Mar 21 2018, 8:20 PM
Harbormaster completed remote builds in B15700: Diff 40569.Mar 21 2018, 8:20 PM
kib accepted this revision.Mar 21 2018, 8:58 PM
This revision is now accepted and ready to land.Mar 21 2018, 8:58 PM
Closed by commit rS331325: Elide the object lock in the common case in vfs_vmio_unwire(). (authored by markj). · Explain WhyMar 21 2018, 9:16 PM
This revision was automatically updated to reflect the committed changes.
Herald added a subscriber: imp. · View Herald TranscriptMar 21 2018, 9:16 PM

Revision Contents
Changeset List

PathSize
head/
sys/
kern/
35 lines

Diff 40573

View Options

head/sys/kern/vfs_bio.c

Loading...