Skip to content

Linux: avoid prefaulting under the ZFS range lock - #18872

Open
nexicturbo wants to merge 1 commit into
openzfs:masterfrom
nexicturbo:agent/fix-self-mmap-write-deadlock
Open

Linux: avoid prefaulting under the ZFS range lock#18872
nexicturbo wants to merge 1 commit into
openzfs:masterfrom
nexicturbo:agent/fix-self-mmap-write-deadlock

Conversation

@nexicturbo

@nexicturbo nexicturbo commented Jul 31, 2026

Copy link
Copy Markdown

Motivation and Context

Fixes #18135.

The deterministic reproducer writes 32 MiB + 1 byte from an mmap of the
same ZFS file. zfs_write() prefaults only the first transaction-sized
32 MiB before taking the file range lock. On the next loop iteration it
prefaults the final byte while that lock is still held. The page fault
enters zfs_getpage() for the same file and waits forever for the range
lock held by the writing thread.

This matches the reported stack and the existing warning in
zfs_write() that prefaulting under the range lock can deadlock through
zfs_getpage().

Reproducer:
https://github.com/jfly/2026-07-31-zfs-deadlock/blob/main/mmap_snake.py

Description

  • Keep prefaulting bounded to the existing transaction-sized limit of
    MIN(n, DMU_MAX_ACCESS >> 1) before range-lock acquisition.
  • Make Linux iterator copies honor uio_fault_disable, so the normal DMU
    write path cannot fault while holding the range lock.
  • Apply the same no-fault rule to the full-block ARC-buffer path and turn
    an incomplete ARC-buffer copy into EFAULT instead of asserting.
  • Remove later prefault and retry paths under the range lock.
  • Preserve normal metadata and ZIL bookkeeping for bytes copied before a
    late EFAULT; Linux reports that progress as a short write.

Large requests no longer require every source page to be resident at once.
If a page beyond the bounded prefault window is not resident, the syscall
returns the bytes already written and userspace can retry the remainder.
The range lock is never dropped during the write, including O_APPEND, so
append atomicity is not weakened.

How Has This Been Tested?

  • Traced the deterministic 32 MiB + 1 byte reproducer against the current
    zfs_write() and Linux iov_iter control flow.
  • Verified there is one executable zfs_uio_prefaultpages() call in
    zfs_write() and it precedes write range-lock acquisition.
  • Verified both in-lock source-copy paths bracket the copy with
    zfs_uio_fault_disable() and that the Linux iterator implementation now
    enforces that flag with pagefault_disable() / pagefault_enable().
  • Ran git diff --check successfully.
  • Checked the changed lines against OpenZFS formatting conventions.
  • Independently validated by the original reproducer against current master plus
    exact head 54cbbed772fa59221bc9bd118aac43a30acfb8d9; the previously
    hanging reproduction completes:
    Linux: avoid prefaulting under the ZFS range lock #18872 (comment)

This host is Windows without WSL or Docker, so I could not build the Linux
kernel module or run ZTS locally. GitHub CI is validating the amended head.

I did not add an automated deadlock regression because the failing case
waits uninterruptibly on the file's own range lock. A userspace timeout
cannot guarantee cleanup and could wedge a bare-metal ZTS host.

Types of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement
  • Code cleanup
  • Quality assurance
  • Breaking change
  • Library ABI change
  • Documentation

Checklist

  • My code follows the OpenZFS code style requirements.
  • I have updated the documentation accordingly.
  • I have read the contributing document.
  • I have added tests to cover my changes.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@github-actions github-actions Bot added the Status: Work in Progress Not yet ready for general review label Jul 31, 2026
@nexicturbo
nexicturbo force-pushed the agent/fix-self-mmap-write-deadlock branch 3 times, most recently from f7d44ba to 61169b0 Compare July 31, 2026 13:49
@nexicturbo
nexicturbo marked this pull request as ready for review July 31, 2026 13:59
@github-actions github-actions Bot added Status: Code Review Needed Ready for review and testing and removed Status: Work in Progress Not yet ready for general review labels Jul 31, 2026
@amotin

amotin commented Jul 31, 2026

Copy link
Copy Markdown
Member

I'll leave it to somebody with more Linux VM and VFS experience, but I have feeling that pre-faulting everything might be impossible in some extreme cases, when requests are so big, that system simply does not have enough RAM to pull it all in, or it might not have sense to pull it all in.

@nexicturbo
nexicturbo force-pushed the agent/fix-self-mmap-write-deadlock branch from 61169b0 to 6d0c2ac Compare July 31, 2026 15:31
@nexicturbo

Copy link
Copy Markdown
Author

Agreed — prefaulting the complete request is not a sound bound. I revised the patch in 6d0c2acd7 to retain the existing MIN(n, DMU_MAX_ACCESS >> 1) prefault window.

After the range lock is acquired, both source-copy paths now run with page faults disabled. If a later page is not resident, Linux preserves any completed bytes and returns a short write instead of faulting or retrying under the lock. This avoids pulling an arbitrarily large request into RAM and keeps the range lock held for the bytes actually written, including O_APPEND.

The revision also makes the Linux iov_iter copy actually honor the existing uio_fault_disable flag and covers the full-block ARC-buffer path; without that, removing the in-lock prefault would only move the recursive fault into copy_from_iter(). git diff --check and the focused control-flow invariants pass locally; the amended head is now running the full GitHub CI matrix.

zfs_write() prefaults only the first transaction-sized chunk before
taking the file range lock.  A larger write faults each later chunk
while that lock is held.  If its source maps the same ZFS file, the
fault enters zfs_getpage() and waits forever for the lock held by the
writing thread.

Keep prefaulting bounded to one transaction-sized chunk and make Linux
iterator copies honor uio_fault_disable.  Both the normal DMU path and
the full-block ARC buffer path now copy without page faults while the
range lock is held.  If a later source page is not resident, preserve
any completed data and report a Linux short write instead of faulting or
retrying under the lock.

Closes openzfs#18135

Signed-off-by: nexicturbo <turbonexic@gmail.com>
@nexicturbo
nexicturbo force-pushed the agent/fix-self-mmap-write-deadlock branch from dc3f120 to 54cbbed Compare July 31, 2026 16:07
@jfly

jfly commented Jul 31, 2026

Copy link
Copy Markdown

Just confirmed that 596c7a0 (master at time of writing) + 54cbbed gets my repro to start working.

@nexicturbo

Copy link
Copy Markdown
Author

CI note: the amended head 54cbbed772fa59221bc9bd118aac43a30acfb8d9 is green on checkstyle, smatch, unit tests, CodeQL, ARM, zloop, and 11 of 14 QEMU platform jobs. The remaining failures are isolated to one VM each and do not exercise the two changed write-path files:

  • Ubuntu 22: fault/auto_spare_001_pos (job)
  • Fedora 43: l2arc/l2arc_dwpd_reimport_pos and redundancy/redundancy_draid_spare1 (job)
  • Fedora 44: l2arc/l2arc_dwpd_ratelimit_pos (job)

The paired VM summaries in those jobs show no unexpected failure. I tried to rerun only the failed jobs, but GitHub requires repository Actions-admin permission. Could someone with that permission rerun the failed jobs? I have not added unrelated code churn solely to retrigger CI.

@behlendorf

Copy link
Copy Markdown
Contributor

I've resubmitted the failed CI jobs. We do still have a handful of flaky tests which is what these failures looks like.

@nexicturbo

Copy link
Copy Markdown
Author

The maintainer reruns are complete on exact head 54cbbed772fa59221bc9bd118aac43a30acfb8d9: 27 checks passed, 3 were skipped as expected, and 0 failed. No code change was needed after the reruns, and the reporter's independent reproducer confirmation still applies. This is ready for code review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Status: Code Review Needed Ready for review and testing

Projects

None yet

Development

Successfully merging this pull request may close these issues.

File can't be read for days (stuck) (cv_wait_common) (lost lock) [250$ bounty]

4 participants