vnode_pager_generic_putpages(): correctly handle clean block at EOF
ClosedPublic
Actions

Authored by kib on Jan 8 2024, 1:25 PM.

Details

Reviewers

alc
markj
asomers

Commits

rGed1a88a3116a: vnode_pager_generic_putpages(): rename maxblksz local to max_offset
rGbdb46c21a3e6: vnode_pager_generic_putpages(): correctly handle clean block at EOF

Summary

vnode_pager_generic_putpages(): correctly handle clean block at EOF

The loop 'skip clean blocks' checking for the clean blocks in the dirty
pages might end up setting the in_hole to true when exactly at EOF at
the middle of the block, without advancing the prev_offset value. Then
the next block is not dirty, and next_offset is clipped back to poffset
+ maxsize, equal to prev_offset, failing the assertion.

Instead of asserting prev_offset < next_offset, we must skip the write.

Reported by:    asomers
PR:     276191

vnode_pager_generic_putpages(): rename maxblksz local to max_offset

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

kib created this revision.Jan 8 2024, 1:25 PM

Herald added a subscriber: imp. · View Herald TranscriptJan 8 2024, 1:25 PM

kib requested review of this revision.Jan 8 2024, 1:25 PM

I don't grok this page cache stuff. But your patch works for me. BTW, fspacectl was a red herring. I can actually reproduce the panic with just a single mapwrite, as long as the size and offset are suitable.

This revision is now accepted and ready to land.Jan 8 2024, 3:56 PM

The loop 'skip clean blocks' checking for the clean blocks in the dirty
pages might end up without setting the in_hole locals value to false, if
we started right at EOF. In this case there is no dirty blocks to write
out, and prev_offset is equal to next_offset, failing the assertion.

In that case, we hit the if (in_hole) goto write_done;, so the explanation doesn't quite make sense.

In the problematic scenario, we are writing pages starting at EOF. (How exactly does that happen?) If so, then maxsize == 0, so maxblksz == poffset, so the "Find longest run of dirty blocks" loop will terminate immediately. In this scenario, putpages is supposed to give up and return VM_PAGER_BAD for each page, right? If so then I think the patch is ok.

sys/vm/vnode_pager.c
1434	The name `maxblksz` doesn't make sense to me, BTW. It sounds like it's referring to a block size but it's really an offset.

In D43358#988241, @markj wrote:

In the problematic scenario, we are writing pages starting at EOF. (How exactly does that happen?)

The test case creates an empty file, extends it with ftruncate, then mmaps it and writes to it. Finally it unmaps and closes the file. Does that make sense?

In D43358#988307, @asomers wrote:

In D43358#988241, @markj wrote:

In the problematic scenario, we are writing pages starting at EOF. (How exactly does that happen?)

The test case creates an empty file, extends it with ftruncate, then mmaps it and writes to it. Finally it unmaps and closes the file. Does that make sense?

Does this all happen sequentially, e.g., from a single thread? I'm a bit confused specifically by how we end up in putpages. Do you have a stack trace handy?

In D43358#988308, @markj wrote:

Does this all happen sequentially, e.g., from a single thread? I'm a bit confused specifically by how we end up in putpages. Do you have a stack trace handy?

Yes, it's single-threaded. See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=276191 for the stack trace.

In D43358#988241, @markj wrote:

The loop 'skip clean blocks' checking for the clean blocks in the dirty
pages might end up without setting the in_hole locals value to false, if
we started right at EOF. In this case there is no dirty blocks to write
out, and prev_offset is equal to next_offset, failing the assertion.

In that case, we hit the if (in_hole) goto write_done;, so the explanation doesn't quite make sense.

In the problematic scenario, we are writing pages starting at EOF. (How exactly does that happen?) If so, then maxsize == 0, so maxblksz == poffset, so the "Find longest run of dirty blocks" loop will terminate immediately. In this scenario, putpages is supposed to give up and return VM_PAGER_BAD for each page, right? If so then I think the patch is ok.

The Peter' trace is in log0510.txt; it happens on the vnode inactivation where vinactivef() does the vm_object_page_clean().

Note that the main loop would re-enter after EOF. Then partially dirty page with prev_offset pointing at EOF get the in_hole cleared, but next_offset advanced by DEV_BSIZE. The clamp of next_offset at start_write returns next_offset back to prev_offset.