Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Windows: SPDK backend support #275

Merged
merged 5 commits into from
Sep 13, 2023
Merged

Windows: SPDK backend support #275

merged 5 commits into from
Sep 13, 2023

Conversation

rs-sam
Copy link
Collaborator

@rs-sam rs-sam commented Apr 3, 2023

Adding support for SPDK backend for Windows OS, using WPDK repo.
Have a look at this initial work to do support SPDK backend for xNVMe on Windows OS.

Created a new subproject called 'spdk-win'. It used mix environment MSYS2-MinGW and WSL for the compilation.
A helper 'spdk_win_build.bat' batch file is used to do compilation of dependency repos like
WPDK, DPDK and SPDK.

@safl
Copy link
Member

safl commented Apr 4, 2023

Yesterday the self-hosted xNVMe github-actions-runners were "migrated" from one hosting provider to another.
Consequently, then the changes described on #276 were needed.

To avoid blocking progress on this PR (#275), due to CI-failures, then this PR was rebased.

Curiously, the rebase of this PR shows in a surprising way on the history. That is, the two commits introduced on #276 looks like they are part of the PR, as the first two commits. Even though they have the same SHAs as the commits on next. It seems inconsequential, e.g. something cosmetically wrong in the GitHUB UI, regardless, writing this to let you know.

@rs-sam rs-sam force-pushed the wpdk branch 4 times, most recently from 1395a6e to 8e5562a Compare April 12, 2023 14:18
@safl safl force-pushed the wpdk branch 2 times, most recently from 7a3d734 to 4fca490 Compare April 13, 2023 15:22
@safl
Copy link
Member

safl commented Apr 13, 2023

HI @rs-sam I rebased this, to get the changes to work around the ci-blockage.

@rs-sam
Copy link
Collaborator Author

rs-sam commented Jul 4, 2023

I have rebased this PR with latest next. It has a new issue that is latest SPDK repo has a non windows friendly file name "https://github.com/spdk/spdk/blob/master/test/common/config/pkgdep/patches/ice/0001-stats-fetch_*_irq.patch"
due to this spdk-win.wrap not able to clone it properly, looking for any workaround for this.

@safl
Copy link
Member

safl commented Jul 12, 2023 •
edited
Loading

To resolve the issue of:

meson.build:224:15: ERROR: Git command failed: ['C:\\Program Files\\Git\\bin/git.EXE', 'clone', 'https://github.com/spdk/spdk.git', 'spdk-win']

Caused by the patch-file not being compatible with WIndows FS, then let's try:

  • Point the subproject to https://github.com/safl/spdk (temporary work-around)
  • Fix this in upstream SPDK (send patch fixing name of the patch)
    • Report issue to SPDK
    • Send Patch
  • Figure out how to get meson to checkout a branch directly, rather than checking out master and then switching.

safl
safl requested changes Jul 12, 2023
View reviewed changes
Copy link
Member

@safl safl left a comment

Choose a reason for hiding this comment

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

To avoid multiple SPDK subprojects e.g. subprojects/spdk and subprojects/spdk-win then it would be great to combine these into a single subproject.
However, ss I understand it then WPDK currently uses v22.05 and xNVMe uses v22.09.
Thus, we need to track to different SPDK versions. Let's make sure this is documented, and then we should work on updating both xNVMe and WPDK to the latest SPDK version v23.??.

subprojects/packagefiles/wpdk/patches/wpdk-0001-fixed-clang-compilation-warning.patch Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson.build Outdated Show resolved Hide resolved
meson_options.txt Outdated Show resolved Hide resolved
safl
safl requested changes Jul 19, 2023
View reviewed changes
Copy link
Member

@safl safl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM, however, we seem to face some CI issues with this.
I would propose:

  • Remove call of xnvme enum
  • Fix what is causing xnvme library-info to fail
  • Drop support / testing for Windows 2019
    • Remove from .gtihub workflow, documentation, etc.
  • Determine whether this can be built using clang instead of gcc
    • Compilation issue needs to be resolved
    • advantage: "direct" use of clang + meson
    • advantage: we would not need to use wsl
    • advantage: a lot of scripts patching dos2unix etc. would no longer we needed

@rs-sam rs-sam force-pushed the wpdk branch 2 times, most recently from 809918c to fc64fdf Compare July 26, 2023 18:42
@safl safl force-pushed the wpdk branch 3 times, most recently from 9a9ad50 to e42ddcd Compare August 14, 2023 08:31
@ankit-sam ankit-sam self-requested a review August 16, 2023 05:30
@safl safl mentioned this pull request Aug 31, 2023
Draft
@safl safl added this to the v0.7.2 milestone Sep 4, 2023
@rs-sam @safl
be/win:spdk-win: add wpdk and spdk-win wrap and subprojects files
6083890 
Including WPDK and SPDK backend, it works to expose spdk as one the
backend on windows machine.

Signed-off-by: Rishabh Shukla <rishabh.sh@samsung.com>
@rs-sam @safl
fix(subprojects/spdk-win): change spdk repo path
353a022 
current spdk master repo is not compatiable for Windows FS,
thats why changing it to older forked repo.

Signed-off-by: Rishabh Shukla <rishabh.sh@samsung.com>
@rs-sam @safl
fix(windows/runner): dropped Windows 2019 runner
a215ed8 
After adding changes related to WPDK and SPDK,
Windows 2019 have some header related compilation errors.
Looks like it has compatibility issue, which is not fixable.

Signed-off-by: Rishabh Shukla <rishabh.sh@samsung.com>
@rs-sam @safl
fix(build): change msys2 install path, update windows pkgs batch file
5dc75a9 
Signed-off-by: Rishabh Shukla <rishabh.sh@samsung.com>
@safl
fix(cli,enum): use xnvme_buf_virt_{alloc|free}() instead of malloc/free
1c4a772 
The enumeration functions allocate virtual memory using what matches the
platform via the backend. However, when this memory was free'd, then it
was done using free().
This fixes it for the use in the xNVMe command-line tool.

Signed-off-by: Simon A. F. Lund <simon.lund@samsung.com>
@safl safl marked this pull request as ready for review September 13, 2023 06:09
karlowich
karlowich reviewed Sep 13, 2023
View reviewed changes
Copy link
Contributor

@karlowich karlowich left a comment

Choose a reason for hiding this comment

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

Everything looks good, I just have a single comment

subprojects/packagefiles/spdk-win/meson.build Show resolved Hide resolved
@safl safl merged commit a86aa8f into xnvme:next Sep 13, 2023
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@safl safl safl approved these changes

@karlowich karlowich karlowich approved these changes

@ankit-sam ankit-sam Awaiting requested review from ankit-sam

Assignees
No one assigned
Labels
None yet
Milestone
v0.7.2
Development

Successfully merging this pull request may close these issues.
3 participants
@rs-sam @safl @karlowich