Page MenuHomeFreeBSD

vnode_pager_generic_putpages(): correctly handle clean block at EOF
ClosedPublic

Authored by kib on Jan 8 2024, 1:25 PM.
Tags
None
Referenced Files
F102553310: D43358.id.diff
Wed, Nov 13, 11:31 PM
Unknown Object (File)
Thu, Nov 7, 2:26 AM
Unknown Object (File)
Tue, Oct 29, 7:32 PM
Unknown Object (File)
Oct 7 2024, 9:44 AM
Unknown Object (File)
Sep 30 2024, 11:36 AM
Unknown Object (File)
Sep 30 2024, 10:54 AM
Unknown Object (File)
Sep 27 2024, 12:56 PM
Unknown Object (File)
Sep 27 2024, 6:33 AM
Subscribers

Details

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

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 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 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?

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.

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.

kib edited the summary of this revision. (Show Details)

Update summary for the main change.
Rename local variable.

This revision now requires review to proceed.Jan 9 2024, 11:37 AM
kib marked an inline comment as done.Jan 9 2024, 11:37 AM
This revision is now accepted and ready to land.Jan 10 2024, 10:31 PM