Skip to content

feat(ublk): optional Discarder and ZeroWriter capabilities#33

Open
ValentaTomas wants to merge 1 commit into
mainfrom
feat/write-zeroes-discard
Open

feat(ublk): optional Discarder and ZeroWriter capabilities#33
ValentaTomas wants to merge 1 commit into
mainfrom
feat/write-zeroes-discard

Conversation

@ValentaTomas
Copy link
Copy Markdown
Member

@ValentaTomas ValentaTomas commented May 14, 2026

DISCARD and WRITE_ZEROES are distinct in the block layer (discard leaves the range undefined; write-zeroes must read back as zeros), so they're exposed as separate optional Backend capabilities — implement either or both:

type Discarder interface {
    DiscardAt(off, length int64) (int, error)
}
type ZeroWriter interface {
    WriteZeroesAt(off, length int64, flags ZeroFlags) (int, error)
}

ZeroFlags carries ZeroNoUnmap (UBLK_IO_F_NOUNMAP, kernel bit 15) so backends that want to keep the range physically allocated can do so. UBLK_PARAM_TYPE_DISCARD (max_discard_sectors / max_write_zeroes_sectors) is advertised per capability, so the kernel only issues an op the backend can serve. The DISCARD / WRITE_ZEROES path bypasses the data-plane buffer and isn't capped by WithMaxIOSize since neither op transfers user data.

@cla-bot cla-bot Bot added the cla-signed label May 14, 2026
@cursor
Copy link
Copy Markdown

cursor Bot commented May 14, 2026

PR Summary

Medium Risk
Touches the IO hot path and kernel parameter advertisement; mistakes can surface as kernel I/O errors or unexpected op dispatch under real filesystems. Coverage is improved with new unit and integration tests, but behavior depends on kernel expectations for DISCARD/WRITE_ZEROES semantics.

Overview
This change adds optional Discarder and ZeroWriter backend capabilities and wires them into worker.handleIO for OP_DISCARD and OP_WRITE_ZEROES, including ZeroNoUnmap flag plumbing.

Device.setParams now conditionally advertises discard/zeroing limits only when the backend implements the matching interface, but OP_FLUSH remains unhandled and large discard/zero requests can now bypass the data-plane buffer size constraints.

Reviewed by Cursor Bugbot for commit ebd5deb. Bugbot is set up for automated code reviews on this repo. Configure here.

@ValentaTomas
Copy link
Copy Markdown
Member Author

Refactored per feedback:

  • Split ZeroWriter into two interfaces — Discarder (DiscardAt) and ZeroWriter (WriteZeroesAt(off, length, flags)). Same semantic split the block layer uses (DISCARD = data undefined; WRITE_ZEROES = reads must return zeros), so a backend can implement them independently or together.
  • Added ZeroFlags carrying ZeroNoUnmap (UBLK_IO_F_NOUNMAP, kernel bit 15). The orchestrator can ignore it today; exposing it here means we don't silently drop the kernel signal.
  • Per-capability params: max_discard_sectors / max_write_zeroes_sectors set only when the corresponding interface is implemented.
  • Compacted tests: 4 unit + 4 integration → 2 unit + 2 integration. Dropped the BLKZEROOUT-without-ZeroWriter case since the kernel falls back to plain WRITE there (not observable from an ioctl).

@ValentaTomas ValentaTomas marked this pull request as ready for review May 14, 2026 23:09
@ValentaTomas ValentaTomas requested a review from arkamar as a code owner May 14, 2026 23:09
@ValentaTomas ValentaTomas enabled auto-merge (squash) May 14, 2026 23:09
ValentaTomas added a commit that referenced this pull request May 14, 2026
## Summary

- `New(backend, size)` becomes `New(backend, Config{...})`. Required
field: `Size`. Optional and defaulted on zero: `BlockSize` (512),
`QueueDepth` (128), `MaxIOSize` (128 KiB).
- Lets callers (e2b orchestrator) wire ublk with explicit knobs without
forking the library or recompiling.
- Breaking change — every example and test callsite updated.

Stacked on top of #33 (ZeroWriter) because both PRs touch \`setParams\`.
Will rebase on main once #33 lands.

## Test plan

- [x] Unit: `TestConfigValidate` covers good/bad shapes.
- [x] Integration: \`BlockSize=4096\` reaches the kernel (verified via
\`BLKBSZGET\`), custom \`QueueDepth\` propagates to the worker, custom
\`MaxIOSize\` clamps the block-layer request size (verified via
\`BLKSECTGET\`).
- [x] Full integration suite still green; \`make lint\` clean.
@ValentaTomas ValentaTomas changed the title feat(ublk): optional ZeroWriter for DISCARD / WRITE_ZEROES feat(ublk): Discarder + ZeroWriter capabilities and Config options May 15, 2026
@ValentaTomas ValentaTomas disabled auto-merge May 16, 2026 00:29
@ValentaTomas ValentaTomas force-pushed the feat/write-zeroes-discard branch from 0f2ddbf to b901422 Compare May 16, 2026 01:04
@ValentaTomas ValentaTomas changed the title feat(ublk): Discarder + ZeroWriter capabilities and Config options feat(ublk): optional Discarder and ZeroWriter capabilities May 16, 2026
@ValentaTomas ValentaTomas changed the base branch from main to feat/config May 16, 2026 01:04
@ValentaTomas ValentaTomas force-pushed the feat/write-zeroes-discard branch 2 times, most recently from 6271089 to 2e64e54 Compare May 16, 2026 01:26
Base automatically changed from feat/config to main May 16, 2026 01:34
DISCARD and WRITE_ZEROES are different ops in the block layer
(discard = data undefined; write-zeroes = reads must return zeros),
so they're separate optional interfaces — implement either or both.

ZeroFlags carries ZeroNoUnmap (UBLK_IO_F_NOUNMAP, bit 15) so the
backend can choose whether to keep the range physically allocated.

UBLK_PARAM_TYPE_DISCARD is advertised per capability, so the kernel
never issues an op the backend can't serve. The DISCARD/WRITE_ZEROES
path bypasses the data-plane buffer and isn't capped by MaxIOBytes.
@ValentaTomas ValentaTomas force-pushed the feat/write-zeroes-discard branch from 2e64e54 to ebd5deb Compare May 16, 2026 01:37
@ValentaTomas ValentaTomas enabled auto-merge (squash) May 16, 2026 01:49
@ValentaTomas
Copy link
Copy Markdown
Member Author

Both points in the Bugbot summary are intentional:

  • OP_FLUSH unhandled — out of scope for this PR. We don't advertise UBLK_ATTR_VOLATILE_CACHE, so the kernel won't emit OP_FLUSH under normal use. Tracked as the new "Handle OP_FLUSH explicitly" entry in TODO.md.
  • DISCARD / WRITE_ZEROES bypass the data-plane buffer — by design. Neither op transfers user data (the kernel passes only offset + length), so capping at WithMaxIOSize (the read/write buffer) would artificially limit pure-metadata ops. The advertised cap is 1 << 21 sectors = 1 GiB per op, and TestBlkDiscardAndZeroOut exercises a 1 MiB discard explicitly to prove the no-buffer path isn't capped by bufSize.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants