vm_page_free_invalid(): Relax the xbusy assertion.
ClosedPublic
Actions

Authored by markj on Jul 26 2020, 5:54 PM.

Details

Reviewers

chs
kib
alc

Commits

rS363607: vm_page_free_invalid(): Relax the xbusy assertion.

Summary

vm_page_assert_xbusied() asserts that the busying thread is the current
thread. For some uses of vm_page_free_invalid() (e.g., error handling
in vnode_pager_generic_getpages_done()), this condition does not hold.

Reported by: Jenkins (armv7 tests) via trasz

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

markj requested review of this revision.Jul 26 2020, 5:54 PM

markj created this revision.

Harbormaster completed remote builds in B32570: Diff 74973.Jul 26 2020, 5:54 PM

markj added reviewers: chs, kib, alc.Jul 26 2020, 5:55 PM

markj added a subscriber: trasz.

kib accepted this revision.Jul 26 2020, 6:23 PM

This revision is now accepted and ready to land.Jul 26 2020, 6:23 PM

But does this mean that vm_page_free_prep() needs similar change ? Or at least, make assertion conditional on the page validity ?

In D25828#571965, @kib wrote:

But does this mean that vm_page_free_prep() needs similar change ? Or at least, make assertion conditional on the page validity ?

Hmm. vm_page_remove() unbusies the page before freeing it, but it has the same bug.

Use vm_page_xbusy_claim() in vm_page_free_invalid().

This revision now requires review to proceed.Jul 26 2020, 6:50 PM

Harbormaster completed remote builds in B32571: Diff 74978.Jul 26 2020, 6:50 PM

kib added inline comments.Jul 26 2020, 7:04 PM

sys/vm/vm_page.c
1377 ↗	(On Diff #74978)	I think a comment would be useful saying that the function is used in contexts where the page is xbusy but not owned by us, because we are typically io completion thread.

Add a comment.

Harbormaster completed remote builds in B32573: Diff 74980.Jul 26 2020, 7:22 PM

kib accepted this revision.Jul 26 2020, 8:00 PM

This revision is now accepted and ready to land.Jul 26 2020, 8:00 PM

ahh, right. the test I added for this fails the I/O in such a way (using gnop) that the iodone is actually called in the start path, so it's the same thread that xbusied the pages in that case. you said this problem was reported by other automated testing, how does that test fail I/O such that the iodone is called in a separate thread?

In D25828#572106, @chs wrote:

ahh, right. the test I added for this fails the I/O in such a way (using gnop) that the iodone is actually called in the start path, so it's the same thread that xbusied the pages in that case. you said this problem was reported by other automated testing, how does that test fail I/O such that the iodone is called in a separate thread?

It was triggered by your test: https://ci.freebsd.org/job/FreeBSD-head-armv7-test/lastBuild/console

I think the difference here is that ARMv7 does not have GEOM direct dispatch enabled, so the completion was handled by the GEOM "up" thread rather than the thread initiating I/O.

Closed by commit rS363607: vm_page_free_invalid(): Relax the xbusy assertion. (authored by markj). · Explain WhyJul 27 2020, 2:25 PM

This revision was automatically updated to reflect the committed changes.

markj added a commit: rS363607: vm_page_free_invalid(): Relax the xbusy assertion..

Herald added a subscriber: imp. · View Herald TranscriptJul 27 2020, 2:25 PM