block_performance_test:fix mq number count error #4286
Conversation
|
python ConfigTest.py --testcase=block_performance_test.with_multi_queue --guestname=RHEL.9.6.0 --firmware=default_bios --clone=no It can not hit same issue on same host (many cpus) |
qingwangrh
force-pushed
the
3493_fix_guest_check_mq_number
branch
from
a8a0adb to
e99aee7
Compare
|
@XueqiangWei @fbq815 Please help to review, thanks. |
|
@fbq815 Could you please help to review it, thanks. |
|
@qingwangrh I've check but all the tests failed on s390x, could you please check details in jira task? I've check but all the tests failed on s390x, could you please check details in jira task? Also, this case is breaking my host environment when it failed, have you try to replace the bin in this case? I didn't find it by checking. I would get this after the case failed: [root@l47 yum.repos.d]# ls |
|
@qingwangrh after offline discussion, there's some product issue block this test, maybe it's better if you link the product issue to the jira issue for this patch, thanks! |
|
Some machines will hit This KVMAUTOMA-4505 block_performance_test: find wrong empty disk result in data corruption. |
|
@qingwangrh after offline discussion, based on the issue mentioned, test environment enfluence and test strategy on s390x, I prefer you set this issue to x86 only, please help update test case and test case tracker google sheet as well. |
qingwangrh
force-pushed
the
3493_fix_guest_check_mq_number
branch
from
e99aee7 to
8d2bae4
Compare
|
@fbq815 Please re-test it. thanks |
|
After offline dicussion with @PaulYuuu, this case was not enabled on s390x before, the test strategy on s390x don't include performance and the original test would ruin the /usr/bin on my host. It's not really relate to this patch, but I cannot approve it, I suggest you set this as x86 only to merge it. |
1. fix mq number count error 2. fix find empty disk error There is error in previous code if it has big number queues. It only count the first line result of command output. Signed-off-by: qingwangrh <[email protected]>
qingwangrh
force-pushed
the
3493_fix_guest_check_mq_number
branch
from
8d2bae4 to
68490bb
Compare
WalkthroughUpdates to the block performance test module and configuration: removed unused import, enhanced disk validation with wipefs checks, simplified command output parsing, added architecture exclusions, and refined multi-queue detection command logic. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 1
🧹 Nitpick comments (3)
qemu/tests/cfg/block_performance_test.cfg (1)
122-122: Consider a more robust directory counting approach.The current command
ls /sys/block/%s/mq/ -p|grep /|wc -lworks but useslswith output parsing, which is generally discouraged in scripts.Consider this more robust alternative:
- check_default_mq_cmd = "ls /sys/block/%s/mq/ -p|grep /|wc -l" + check_default_mq_cmd = "find /sys/block/%s/mq/ -mindepth 1 -maxdepth 1 -type d | wc -l"That said, for the predictable
/sysfilesystem structure, the current approach should work reliably.qemu/tests/block_performance_test.py (2)
432-433: Consider adding error handling to the cleanup command.The addition of
wipefs -ais good for ensuring clean disk state. However, the current command uses forced lazy unmount (-fl) and has no error checking.Consider making the cleanup more robust:
- umount_cmd = "umount -fl %s;sleep 3; wipefs -a %s;" % (fio_dir, disk) + umount_cmd = "umount -fl %s && sleep 3 && wipefs -a %s || true" % (fio_dir, disk)Using
&&ensures wipefs only runs if umount succeeds, and|| trueprevents the cleanup from failing the test if the disk is already clean.
526-527: Add error handling for the int conversion.The direct conversion to int is cleaner than string parsing, but lacks error handling if the command output is unexpected.
Consider adding validation:
- default_mq_nums = int(session.cmd_output(check_default_mq_cmd)) - logger.debug(default_mq_nums) + mq_output = session.cmd_output(check_default_mq_cmd).strip() + try: + default_mq_nums = int(mq_output) + except ValueError: + test.fail("Failed to parse mq count from output: %s" % mq_output) + logger.debug("Found %d mq queues", default_mq_nums)This provides clearer error messages if the command output format changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
qemu/tests/block_performance_test.py(3 hunks)qemu/tests/cfg/block_performance_test.cfg(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
qemu/tests/block_performance_test.py (2)
provider/storage_benchmark.py (1)
session(83-90)qemu/deps/win_driver_install/win_driver_install.py (1)
cmd_output(12-28)
🔇 Additional comments (2)
qemu/tests/cfg/block_performance_test.cfg (2)
3-3: Architecture exclusion aligns with PR discussion.The exclusion of s390 and s390x architectures directly addresses the test failures and host environment damage reported in the PR comments. As noted by fbq815, s390x test strategy does not include performance testing, and the test could break the host environment.
64-64: LGTM - allows enhanced auto-selection.Commenting out the hard-coded disk selection enables the improved auto-selection logic with wipefs signature checks added in the Python test file.
| sig = process.getoutput("wipefs -ni %s 2>/dev/null" % name) | ||
| if sig: | ||
| logger.debug("Skip %s due to has signature: %s", name, sig) | ||
| continue |
There was a problem hiding this comment.
🧩 Analysis chain
Verify the wipefs command flags.
The command uses wipefs -ni but standard wipefs does not have an -i flag. The stderr redirection (2>/dev/null) will hide any "invalid option" errors, potentially causing the signature check to fail silently.
Please verify the correct wipefs syntax. If checking for signatures, consider:
- sig = process.getoutput("wipefs -ni %s 2>/dev/null" % name)
+ sig = process.getoutput("wipefs -n %s 2>/dev/null" % name)Let me verify the wipefs flags in the environment:
🏁 Script executed:
#!/bin/bash
# Check wipefs version and available flags
wipefs --help 2>&1 | grep -E "^\s*-[a-z]"
# Test the command syntax with a non-existent device to see the error
wipefs -ni /dev/null 2>&1 || true
wipefs -n /dev/null 2>&1 || trueLength of output: 89
The -n flag in wipefs means "Do not print a header line" (noheadings), not signature checking. The -a flag erases all available signatures, with the set of erased signatures restricted using the -t option. No -i flag exists in the official wipefs documentation.
The command wipefs -ni %s 2>/dev/null will silently fail because:
- The
-iflag is invalid - The stderr redirection hides the error message
- The signature check never actually executes
Fix the invalid wipefs flag.
Remove the -i flag:
- sig = process.getoutput("wipefs -ni %s 2>/dev/null" % name)
+ sig = process.getoutput("wipefs -n %s 2>/dev/null" % name)Note: This invokes wipefs without options to list signatures (the -n suppresses header output). If the intent is to erase signatures, use wipefs -a instead.
🤖 Prompt for AI Agents
In qemu/tests/block_performance_test.py around lines 487 to 490, the wipefs call
uses an invalid "-i" flag and redirects stderr so the failure is hidden; remove
the "-i" flag so wipefs lists signatures correctly (keep "-n" to suppress
headings) — if the original intent was to erase signatures instead of listing
them, replace the call with "wipefs -a" (and avoid redirecting stderr) so
signature removal is performed and errors are visible.
3 participants
ID:3493,4505
Summary by CodeRabbit
Tests
Chores