Add block 4k device unaligned discard test #4377
Conversation
WalkthroughAdds a new QEMU test and config to validate unaligned discard behavior for a host scsi_debug 4K disk exposed to guests as 512B or 4K sectors; the test discovers scsi_debug devices, boots a VM with the disk, runs parameterized blkdiscard/lsblk guest IOs, checks exit codes, and cleans up. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as Test Runner
participant Host
participant scsi_debug as scsi_debug (host)
participant VM as Guest VM
participant Guest as Guest Session
Host->>scsi_debug: modprobe scsi_debug sector_size=4096 (pre_command)
Runner->>Host: (optional) env_process.process(preprocess_image, preprocess_vm)
Runner->>VM: Boot VM with scsi_debug-backed data disk
VM-->>Runner: VM alive
Runner->>Guest: Open login session
Runner->>Host: lsscsi -> discover scsi_debug device(s)
Host-->>Runner: return {path, sg, wwn, size}
Runner->>Guest: Resolve drive path (serial_stg1 or discovered WWN/sg)
loop For each guest_io_cmdN
Runner->>Guest: Execute formatted blkdiscard/lsblk command
Guest-->>Runner: Exit status
Runner->>Runner: Compare exit status to expected_result
end
Runner->>VM: Destroy VM (force if needed)
Host->>scsi_debug: rmmod scsi_debug (post_command)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing touches
🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
qemu/tests/cfg/block_4k_discard.cfg (2)
23-23: Duplicate guest_io_cmd0; last one wins and changes semantics.You define guest_io_cmd0 twice; the latter (with grep) overrides the first. This is confusing and can skew expectations.
Apply either (remove the trailing duplicate):
- guest_io_cmd0 = "${lsblk_cmd}|egrep '${sec_size}.*${sec_size}'"or (if the filtered output is desired) remove the earlier unfiltered one:
- guest_io_cmd0 = "${lsblk_cmd}"Also applies to: 50-50
34-35: Typo in parameter name “expected_resultes”.Prefer “expected_results” for clarity; keep backward-compat in the Python to avoid breaking existing cfgs.
If you adopt the rename, update these lines:
- expected_resultes = 0 0 0 0 0 + expected_results = 0 0 0 0 0(and similarly for other variants), and accept the Python-side fallback in the companion change below.
Also applies to: 40-40, 44-44, 49-49
qemu/tests/block_4k_discard.py (4)
4-7: Remove unused imports.-import os -import re -import time +import re
72-85: Guard against missing WWN when serial_stg1 is unset.If serial isn’t provided (block/generic variants), ensure we found a WWN; otherwise the guest disk lookup will fail.
- if scsi_debug_devs: + if scsi_debug_devs: dev = scsi_debug_devs[0] - disk_wwn = dev["wwn"] + disk_wwn = dev.get("wwn") if params["drive_format_stg1"] == "scsi-generic": params["image_name_stg1"] = dev["sg"] else: params["image_name_stg1"] = dev["path"] else: test.fail("Can not find scsi_debug devices") + if not params.get("serial_stg1") and not disk_wwn: + test.cancel("No serial_stg1 and no WWN found; cannot reliably locate disk in guest")
87-96: Preprocess toggle is non-intuitive; add a brief comment.Pattern is fine (call preprocess when not_preprocess=yes), but it reads backwards.
Add a comment above explaining that this test self-preprocesses when the framework is instructed to skip preprocessing.
111-111: Add trailing newline.- +\n
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/block_4k_discard.py(1 hunks)qemu/tests/cfg/block_4k_discard.cfg(1 hunks)
🧰 Additional context used
🪛 Ruff (0.12.2)
qemu/tests/block_4k_discard.py
111-111: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (1)
qemu/tests/cfg/block_4k_discard.cfg (1)
1-1: Revert—leading dashes on cfg section headers are the norm
tp-qemu cfg files predominantly declare top-level sections as- name:(3629 vs 2641 without) so- block_4k_discard:is correct.Likely an incorrect or invalid review comment.
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
a6cfffb to
81f06b3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
qemu/tests/block_4k_discard.py (1)
72-79: Null-safe WWN parsing; avoid AttributeError and support common formats.
wwn_tok.split("0x", 1)[1]will crash if no0x*token exists;lsscsi -w -soften printswwn:0x…ornaa.…. Parse robustly and allow missing WWN.- wwn_tok = next((t for t in tokens if t.startswith("0x")), None) - wwn = wwn_tok.split("0x", 1)[1] + # WWN may appear as 'wwn:0x...' (preferred) or as a bare '0x...'/ 'naa.*' + wwn = None + wwn_tok = next((t for t in tokens if t.startswith("wwn:")), None) + if wwn_tok and ":" in wwn_tok: + wwn = wwn_tok.split(":", 1)[1] + else: + alt = next((t for t in tokens if re.fullmatch(r"(?:0x[0-9a-fA-F]+|naa\..+)", t)), None) + wwn = alt
🧹 Nitpick comments (2)
qemu/tests/block_4k_discard.py (2)
55-58: Drop redundant '|| true'.
ignore_status=Truealready suppresses non-zero exit codes.- cmd = "lsscsi -g -w -s | grep scsi_debug || true" + cmd = "lsscsi -g -w -s | grep scsi_debug"
92-92: Remove stray print; use logger.- print(disk_wwn) + logger.debug("Selected scsi_debug WWN: %s", disk_wwn)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/block_4k_discard.py(1 hunks)qemu/tests/cfg/block_4k_discard.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/block_4k_discard.cfg
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
81f06b3 to
81b6484
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
qemu/tests/block_4k_discard.py (3)
35-40: Clarify mismatch message; use correct param name.Message says
io_cmd_number; the param isguest_io_cmd_number. Also helpful to include the raw list for debugging.- test.cancel( - "Mismatch: io_cmd_number={} but expected_results has {} items".format( - io_cmd_num, len(results_raw) - ) - ) + test.cancel( + "Mismatch: guest_io_cmd_number={} but expected_results has {} items: {}" + .format(io_cmd_num, len(results_raw), results_raw) + )
77-84: Fix WWN parsing; current code can crash and misseswwn:token.
wwn_toklooks for tokens starting with"0x"and then unconditionally calls.split, which will raise when no match is found.lsscsi -wemitswwn:0x...; parse that and guard for None.Apply:
- wwn_tok = next((t for t in tokens if t.startswith("0x")), None) - wwn = wwn_tok.split("0x", 1)[1] + wwn_tok = next((t for t in tokens if t.startswith("wwn:")), None) + wwn = wwn_tok.split(":", 1)[1] if wwn_tok and ":" in wwn_tok else None
105-113: Invert the not_preprocess gate; currently runs when it should skip.Preprocessing executes when
not_preprocess=yes. Only run when it’s not “yes”.- if params.get("not_preprocess", "no") == "yes": - logger.debug("Ready boot VM : %s", params["images"]) + if params.get("not_preprocess", "no") != "yes": + logger.debug("Preprocessing and booting VM: %s", params.get("images")) env_process.process( test, params, env, env_process.preprocess_image, env_process.preprocess_vm, )
🧹 Nitpick comments (4)
qemu/tests/block_4k_discard.py (4)
60-63: Drop redundant|| true;ignore_status=Truealready suppresses errors.Simplifies the host command while keeping behavior.
- cmd = "lsscsi -g -w -s | grep scsi_debug || true" + cmd = "lsscsi -g -w -s | grep scsi_debug"
97-97: Remove stray print; use logger if needed.
print(disk_wwn)is a leftover debug.- print(disk_wwn) + logger.debug("Selected scsi_debug WWN: %s", disk_wwn)
64-66: Unify wording and logger usage; minor copy edits.Prefer
logger.warning(...)overtest.log.warning(...)for consistency, and use “Cannot …” phrasing.- logger.info("Host cmd output '%s'", out) - if not out: - test.log.warning("Can not find scsi_debug disk") + logger.info("Host cmd output '%s'", out) + if not out: + logger.warning("Cannot find scsi_debug disk") @@ - logger.warning("Unable to parse scsi_debug line: %s", line) + logger.warning("Unable to parse scsi_debug line: %s", line) @@ - test.fail("Can not find disk by {}".format(serial)) + test.fail("Cannot find disk by {}".format(serial)) @@ - test.fail("Can not find scsi_debug devices") + test.fail("Cannot find scsi_debug devices")Also applies to: 80-81, 29-30, 103-103
1-2: Tighten docstring language.Use explicit units and clearer phrasing.
-"""Attach the host 4k disk but QEMU is exposing a 512 to the Guest. -Test the unaligned discard operation on the disk.""" +"""Attach a host 4K disk while QEMU exposes 512B sectors to the guest. +Test unaligned discard behavior on that disk."""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/block_4k_discard.py(1 hunks)qemu/tests/cfg/block_4k_discard.cfg(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- qemu/tests/cfg/block_4k_discard.cfg
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/block_4k_discard.py (1)
provider/storage_benchmark.py (1)
session(83-90)
🔇 Additional comments (2)
qemu/tests/block_4k_discard.py (2)
41-58: Good hardening of result parsing and logging.Safe int conversion, structured logging, and detailed failure messages look solid.
121-124: Good guard on missing identifier before guest lookup.Prevents passing an empty identifier to
get_linux_drive_path.
|
@XueqiangWei @fbq815 Please help to review, thanks. |
Signed-off-by: qingwangrh <[email protected]>
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
81b6484 to
c677377
Compare
ID:4056
Summary by CodeRabbit