Skip to content

Conversation

mattkur
Copy link
Contributor

@mattkur mattkur commented Sep 3, 2025

This test generates IO against the translated devices. This has found a bug in in-development code, so I believe there is value here.

  • Up the size of the attached disk, so that there is enough space for IO.
  • Use dd, since that's inbox. 1MB IO matches the IO pattern seen for the bug offline.
  • Also make it easier to debug, so print more output.

@Copilot Copilot AI review requested due to automatic review settings September 3, 2025 22:39
@mattkur mattkur requested a review from a team as a code owner September 3, 2025 22:39
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the storvsp VMM test by adding actual I/O operations to test storage devices more thoroughly. The test previously only verified device presence and sizing but now performs 100MB write operations to both SCSI and NVMe devices to validate their functionality.

Key changes:

  • Increased disk sizes from ~4MB to ~128-160MB to accommodate the new I/O operations
  • Added comprehensive device enumeration with improved logging and error handling
  • Implemented actual I/O testing using dd commands to write 100MB to each storage device

@mattkur mattkur requested a review from a team September 3, 2025 22:42
@jstarks
Copy link
Member

jstarks commented Sep 3, 2025

Wasn't there already a PR for this?

@jstarks
Copy link
Member

jstarks commented Sep 3, 2025

#1928

@mattkur
Copy link
Contributor Author

mattkur commented Sep 3, 2025

Wasn't there already a PR for this?

yes. something happened on my end where I could no longer update that branch. I've closed that out. I also see your feedback about reading the data back.

Copy link

github-actions bot commented Sep 3, 2025

Copy link

github-actions bot commented Sep 4, 2025

Copy link
Member

@jstarks jstarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some style feedback

@mattkur mattkur requested a review from a team as a code owner September 30, 2025 23:20
@mattkur mattkur marked this pull request as draft September 30, 2025 23:21
…t are shown

through OpenHCL. This also verifies that the data was correctly written to disk.

Design choices:
 - Use `dd`, since that is what's inbox in BusyBox
 - Ditto for the parameters to `dd` (e.g. it would have been ideal to use
   `oflags=sync`, but that is not supported by the inbox version of
   `dd`).
 - 1MB IOs, for 100MB, is enough to provoke a bug that we saw in other
   changes.
@mattkur mattkur marked this pull request as ready for review October 1, 2025 19:58
@mattkur
Copy link
Contributor Author

mattkur commented Oct 1, 2025

@gurasinghMS , @jstarks - I deprioritized this. I've now cleaned it up. Thanks for the previous reviews; I'm ready for a second look from you.

@mattkur mattkur merged commit 808ac0e into microsoft:main Oct 1, 2025
56 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants