Page MenuHomeFreeBSD
Differential D20986

Centralize the logic in vfs_vmio_unwire() and sendfile_free_page().
ClosedPublic
Actions

Authored by markj on Jul 18 2019, 6:53 PM.
Tags
None
Referenced Files
F86148224: D20986.id59969.diff
Sun, Jun 16, 3:51 AM
Unknown Object (File)
Mon, Jun 3, 12:20 PM
Unknown Object (File)
Wed, May 29, 12:31 AM
Unknown Object (File)
May 14 2024, 10:50 AM
Unknown Object (File)
May 5 2024, 10:07 AM
Unknown Object (File)
May 4 2024, 12:32 PM
Unknown Object (File)
Apr 28 2024, 8:29 AM
Unknown Object (File)
Apr 25 2024, 5:06 PM
View All 52 Files
Subscribers
imp

Details

Reviewers
alc
dougm
kib
jeff
glebius
Commits
rS350431: Centralize the logic in vfs_vmio_unwire() and sendfile_free_page().
Summary

These functions are similar and I would like to have their functionality
live in generic VM functions so that the details of vm_page
synchronization can be encapsulated in vm_page.c.

The change adds vm_page_release() and vm_page_release_locked(). They
both unwire the page and optionally attempt to free the page.
vm_page_release_locked() requires the object lock. The main difference
is that vfs_vmio_unwire() may attempt to free a mapped page, while
sendfile_free_page() does not. This change thus modifies the handling
of direct I/O and B_NOREUSE to avoid freeing a mapped page.

Diff Detail

Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 25463
Build 24083: arc lint + arc unit

Event Timeline

markj created this revision.Jul 18 2019, 6:53 PM
Harbormaster completed remote builds in B25402: Diff 59888.Jul 18 2019, 6:53 PM
markj added reviewers: alc, dougm, kib, jeff, glebius.Jul 18 2019, 6:54 PM
kib added inline comments.Jul 18 2019, 8:11 PM
sys/vm/vm_page.c
171

Is this unrelated ?

3790

What prevents other thread from busying the page after we tested for !xbusy but before TRYWLOCK() succeeded ?

markj added inline comments.Jul 18 2019, 8:16 PM
sys/vm/vm_page.c
171

No, I renamed the old (static) vm_page_release() to vm_page_zone_release(), and renamed vm_page_import() as well for consistency.

3790

Nothing, but there is a vm_page_busied(m) check after we acquire the object lock. This is identical to the logic in sendfile_free_page(). I accidentally removed a comment explaining that the first check is racy, I will restore it.

markj updated this revision to Diff 59893.Jul 18 2019, 8:21 PM

Restore a lost comment.

Harbormaster completed remote builds in B25406: Diff 59893.Jul 18 2019, 8:21 PM
kib added inline comments.Jul 18 2019, 8:56 PM
sys/vm/vm_page.c
3790

Then why the first (racy) check is not vm_page_busied() ?

markj updated this revision to Diff 59894.Jul 18 2019, 9:01 PM

Don't attempt to free the page if it is busy at all.

Harbormaster completed remote builds in B25407: Diff 59894.Jul 18 2019, 9:01 PM
kib accepted this revision.Jul 18 2019, 9:14 PM
kib added inline comments.
sys/vm/vm_page.c
171

I think it is useful to commit this part separately, and definitely merge it to 12.

This revision is now accepted and ready to land.Jul 18 2019, 9:14 PM
markj updated this revision to Diff 59969.Jul 20 2019, 8:33 PM

Rebase.

This revision now requires review to proceed.Jul 20 2019, 8:33 PM
Harbormaster completed remote builds in B25439: Diff 59969.Jul 20 2019, 8:33 PM
dougm added inline comments.Jul 22 2019, 4:45 AM
sys/vm/vm_page.c
3780

assert that m->object is not locked.

3819

Consider defining these seven lines as a function of m and flags, and using it here and below.

markj updated this revision to Diff 60035.Jul 23 2019, 1:39 AM
markj marked 2 inline comments as done.
  • Use a common subrouting for releasing to the page queues.
  • Assert that the object is unlocked in vm_page_release().
Harbormaster completed remote builds in B25463: Diff 60035.Jul 23 2019, 1:39 AM
markj added inline comments.Jul 23 2019, 1:39 AM
sys/vm/vm_page.c
3780

Note that m->object may be NULL. I added the conditional assertion under the page lock, since the page lock is required in order to modify the object field.

kib accepted this revision.Jul 23 2019, 7:42 AM
This revision is now accepted and ready to land.Jul 23 2019, 7:42 AM
alc added inline comments.Jul 27 2019, 4:10 AM
sys/vm/vm_page.c
3782

"managed" -> "unmanaged"

3823

"managed" -> "unmanaged"

markj updated this revision to Diff 60206.Jul 27 2019, 11:51 PM

Fix typos.

This revision now requires review to proceed.Jul 27 2019, 11:51 PM
Harbormaster completed remote builds in B25548: Diff 60206.Jul 27 2019, 11:51 PM
alc accepted this revision.Jul 28 2019, 9:37 PM
This revision is now accepted and ready to land.Jul 28 2019, 9:37 PM
Closed by commit rS350431: Centralize the logic in vfs_vmio_unwire() and sendfile_free_page(). (authored by markj). · Explain WhyJul 29 2019, 10:01 PM
This revision was automatically updated to reflect the committed changes.
markj added a commit: rS350431: Centralize the logic in vfs_vmio_unwire() and sendfile_free_page()..
Herald added a subscriber: imp. · View Herald TranscriptJul 29 2019, 10:01 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
70 lines
62 lines
vm/
7 lines
97 lines

Diff 60035

View Options

sys/kern/kern_sendfile.c

Loading...
View Options

sys/kern/vfs_bio.c

Loading...
View Options

sys/vm/vm_page.h

Loading...
View Options

sys/vm/vm_page.c

Loading...