Page MenuHomeFreeBSD

vm_page_free_invalid(): Relax the xbusy assertion.
ClosedPublic

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

Details

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

markj requested review of this revision.Sun, Jul 26, 5:54 PM
markj created this revision.
markj added reviewers: chs, kib, alc.Sun, Jul 26, 5:55 PM
markj added a subscriber: trasz.
kib accepted this revision.Sun, Jul 26, 6:23 PM
This revision is now accepted and ready to land.Sun, Jul 26, 6:23 PM
kib added a comment.Sun, Jul 26, 6:24 PM

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

markj added a comment.Sun, Jul 26, 6:47 PM
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.

markj updated this revision to Diff 74978.Sun, Jul 26, 6:50 PM

Use vm_page_xbusy_claim() in vm_page_free_invalid().

This revision now requires review to proceed.Sun, Jul 26, 6:50 PM
kib added inline comments.Sun, Jul 26, 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.

markj updated this revision to Diff 74980.Sun, Jul 26, 7:22 PM
markj marked an inline comment as done.

Add a comment.

kib accepted this revision.Sun, Jul 26, 8:00 PM
This revision is now accepted and ready to land.Sun, Jul 26, 8:00 PM
chs accepted this revision.Mon, Jul 27, 10:14 AM

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?

markj added a comment.Mon, Jul 27, 2:20 PM
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.

This revision was automatically updated to reflect the committed changes.