Add block 4k device unaligned discard test #4377
Conversation
WalkthroughAdds a new QEMU test and config that load a host scsi_debug device with 4K sectors, expose it to a guest as different drive types, run host/guest lsblk and guest blkdiscard commands, verify expected exit codes per variant, and clean up VM and scsi_debug. 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 / login available
Runner->>Host: lsscsi -> discover scsi_debug device(s)
Host-->>Runner: return {path, sg, wwn, size}
Runner->>Guest: Resolve target disk (serial_stg1 or discovered id)
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 Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. |
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
81b6484 to
c677377
Compare
|
@qingwangrh Could you help add test results? Many thanks. |
|
python ConfigTest.py --testcase=block_4k_discard --guestname=RHEL.10.2 --firmware=default_bios --clone=no (1/4) Host_RHEL.m10.u1.qcow2.virtio_scsi.up.virtio_net.Guest.RHEL.10.2.x86_64.io-github-autotest-qemu.block_4k_discard.with_blk.q35: STARTED |
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
c677377 to
5f15dbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
qemu/tests/block_4k_discard.py (1)
104-104: Fix inverted preprocessing gate (preprocessing runs when it should be skipped).The condition
params.get("not_preprocess", "no") == "yes"means preprocessing runs whennot_preprocessis"yes", which is backwards. The parameter name implies "skip preprocessing when yes", so preprocessing should run whennot_preprocess != "yes".Apply this diff:
- if params.get("not_preprocess", "no") == "yes": + if params.get("not_preprocess", "no") != "yes":
🧹 Nitpick comments (4)
qemu/tests/block_4k_discard.py (4)
37-37: Clarify the parameter name in the error message.The error message references
io_cmd_numberbut the actual parameter name isguest_io_cmd_number. Update the message for clarity.Apply this diff:
- "Mismatch: io_cmd_number={} but expected_results has {} items".format( + "Mismatch: guest_io_cmd_number={} but expected_results has {} items".format(
55-56: Use consistent string formatting.Line 55-56 uses
%formatting while the rest of the function uses.format(). Prefer.format()for consistency.Apply this diff:
- "Unexpected rc=%s:%s, cmd=%r, out=%s" - % (rc, expected, cmd, out.strip()) + "Unexpected rc={}:{}, cmd={!r}, out={}".format( + rc, expected, cmd, out.strip() + )
66-66: Use consistent logger reference.Line 66 uses
test.log.warningwhile the rest of the function useslogger(which is assigned fromtest.logat line 89). Uselogger.warningfor consistency.Apply this diff:
- test.log.warning("Can not find scsi_debug disk") + logger.warning("Can not find scsi_debug disk")
105-105: Address feedback and guard against missing parameter.Per XueqiangWei's comment, consider using
logger.infoinstead oflogger.debugor removing this line. Additionally,params["images"]should use.get()to avoidKeyError.Apply this diff:
- logger.debug("Ready boot VM : %s", params["images"]) + logger.info("Preprocessing and booting VM: %s", params.get("images"))
📜 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)
Signed-off-by: qingwangrh <[email protected]>
qingwangrh
force-pushed
the
4056_4k_unaligned_discard
branch
from
5f15dbc to
799acb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
qemu/tests/block_4k_discard.py (3)
77-78: CRITICAL: Fix AttributeError when WWN token is missing.Line 78 calls
.split()onwwn_tokwithout checking if it'sNone. This causesAttributeError: 'NoneType' object has no attribute 'split'when no token starting with"0x"is found. This issue was confirmed by fbq815 on s390x where the test fails with this exact error.Apply this diff:
wwn_tok = next((t for t in tokens if t.startswith("0x")), None) - wwn = wwn_tok.split("0x", 1)[1] + wwn = wwn_tok.split("0x", 1)[1] if wwn_tok else NoneBased on past review comments and s390x failure reports.
104-112: MAJOR: Fix inverted not_preprocess logic.The condition is inverted: when
not_preprocess="yes"(meaning "do not preprocess"), the code currently runs preprocessing. It should skip preprocessing instead. The configuration file setsnot_preprocess = yeson line 11, explicitly requesting no preprocessing.Apply this diff:
- if params.get("not_preprocess", "no") == "yes": + if params.get("not_preprocess", "no") != "yes": logger.debug("Ready boot VM : %s", params["images"]) env_process.process(Based on past review comments.
97-97: MAJOR: Guard against missing drive_format_stg1 parameter.Direct dictionary access
params["drive_format_stg1"]will raiseKeyErrorif the parameter is not defined in the test configuration. This was flagged in a previous review but remains unaddressed.Apply this diff:
- if params["drive_format_stg1"] == "scsi-generic": + if params.get("drive_format_stg1") == "scsi-generic":Based on past review comments.
🧹 Nitpick comments (2)
qemu/tests/cfg/block_4k_discard.cfg (1)
19-26: Consider adding a comment explaining guest_io_cmd0's deferred definition.The configuration defines
guest_io_cmd1throughguest_io_cmd4here butguest_io_cmd0appears later at line 49 after the variants. While this is functionally correct (sinceguest_io_cmd0requires the variant-specific${sec_size}), the non-sequential ordering may confuse maintainers.Consider adding an inline comment:
guest_io_cmd_number = 5 +# Note: guest_io_cmd0 is defined after variants (line 49) as it depends on variant-specific sec_size dd_cmd = "dd if=/dev/zero of={0} bs=1M count=100 oflag=direct "qemu/tests/block_4k_discard.py (1)
35-40: Clarify parameter name in error message.The error message references
io_cmd_numberbut the actual parameter name isguest_io_cmd_number(line 31). Using the exact parameter name improves debuggability.Apply this diff:
if len(results_raw) != io_cmd_num: test.cancel( - "Mismatch: io_cmd_number={} but expected_results has {} items".format( + "Mismatch: guest_io_cmd_number={} but expected_results has {} items".format( io_cmd_num, len(results_raw) ) )
📜 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 comments (1)
qemu/tests/cfg/block_4k_discard.cfg (1)
49-49: LGTM: guest_io_cmd0 correctly placed after variants.Defining
guest_io_cmd0here allows it to use the variant-specific${sec_size}value. The command mirrorshost_io_cmd(line 22) to verify the guest sees the expected sector size.
There was a problem hiding this comment.
LGTM:
RHEL9:
(1/4) block_4k_discard.with_blk.s390-virtio: PASS (33.57 s)
(2/4) block_4k_discard.with_hd.s390-virtio: PASS (37.00 s)
(3/4) block_4k_discard.with_block.s390-virtio: PASS (36.29 s)
(4/4) block_4k_discard.with_generic.s390-virtio: PASS (36.45 s)
RHEL10:
(1/4) block_4k_discard.with_blk.s390-virtio: PASS (38.28 s)
(2/4)block_4k_discard.with_hd.s390-virtio: PASS (38.21 s)
(3/4)block_4k_discard.with_block.s390-virtio: PASS (37.56 s)
(4/4)block_4k_discard.with_generic.s390-virtio: PASS (37.74 s)
3 participants
ID:4056
Summary by CodeRabbit