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.

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

Verify nvme from interface instead of name #94

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

BjoernWaechter
Copy link

Could this PR fix the nvme windows issue?

Closes #93

@jackeichen
Copy link
Contributor

Althrough most nvme disk is solid state disk, i still suggest try to check identify Namespace data structure (CNS 08h).
HDD may also use nvme.

@BjoernWaechter
Copy link
Author

Hi,

thanks for the quick reply. True, there could be HDDs with NVMe but from my investigations only seagate has a prototype, but this is still not commercially available (see: https://www.tomshardware.com/news/seagate-demonstrates-hdd-with-pcie-nvme-interface)
I think the current state is: All (100%) of SSDs on windows systems are reported wrong with is_sdd=False and with this fix this is gone. I verified (via internet search), all the drives in the unittests data are real SSDs:

  • Netac NVMe SSD 2TB - nvme_10_issue_73
  • KBG30ZMV256G TOSHIBA - nvme_11_issue_75
  • KINGSTON SA2000M8500G - nvme_9_issue_72
  • Samsung SSD 980 PRO 1TB - nvme_win10_0_issue_64
  • WDS100T3X0C-00SJG0 - nvme_win10_mingw32_0_issue_48

I think this potential error of wrongly reported HDDs with NVME is much better than 100% wrong SSDs on Windows.

Is the Namespace data structure (CNS 08h) even reported by smartctl? I cannot find it. If you could give me a hint and/or sample data I could do some further investigation.

@ralequi
Copy link
Collaborator

ralequi commented Feb 14, 2025

Nice contrib @BjoernWaechter , I think we should notice that the smart is enabled by default on any nvme interface rather than devices with "nvme" in it.

On the other hand @jackeichen has its point on that we have to check if nvme is actually a SSD.
Let me check if we can do this without enforcing on any nvme

@ralequi
Copy link
Collaborator

ralequi commented Feb 14, 2025

After checking this in deep, I don't see an easy way to detect if an NVMe is SSD. Probably, that's why the past behaviour also was that if a disk had "nvme" on its name, it must be an SSD. So, this change keeps that "bug" (if we should say that) to nvme = SSD.

@jackeichen do you have a way to detect if an NVMe is SSD? Maybe some specific call to smartctl?

This, is an example of smartctl output of an nvme, tell me if you see something that points to a "ssd" device type that I'm missing please:

smartctl 7.3 2022-02-28 r5338 [x86_64-w64-mingw32-w10-21H2] (sf-7.3-1)
Copyright (C) 2002-22, Bruce Allen, Christian Franke, www.smartmontools.org

=== START OF INFORMATION SECTION ===
Model Number: WDS100T3X0C-00SJG0
Serial Number: 20093F800129
Firmware Version: 111110WD
PCI Vendor/Subsystem ID: 0x15b7
IEEE OUI Identifier: 0x001b44
Total NVM Capacity: 1,000,204,886,016 [1.00 TB]
Unallocated NVM Capacity: 0
Controller ID: 8215
NVMe Version: 1.3
Number of Namespaces: 1
Namespace 1 Size/Capacity: 1,000,204,886,016 [1.00 TB]
Namespace 1 Formatted LBA Size: 512
Namespace 1 IEEE EUI-64: 001b44 8b46ac714d
Local Time is: Tue Sep 13 22:57:28 2022
Firmware Updates (0x14): 2 Slots, no Reset required
Optional Admin Commands (0x0017): Security Format Frmw_DL Self_Test
Optional NVM Commands (0x005f): Comp Wr_Unc DS_Mngmt Wr_Zero Sav/Sel_Feat Timestmp
Log Page Attributes (0x0e): Cmd_Eff_Lg Ext_Get_Lg Telmtry_Lg
Maximum Data Transfer Size: 128 Pages
Warning Comp. Temp. Threshold: 84 Celsius
Critical Comp. Temp. Threshold: 88 Celsius
Namespace 1 Features (0x02): NA_Fields

Supported Power States
St Op Max Active Idle RL RT WL WT Ent_Lat Ex_Lat
0 + 6.00W - - 0 0 0 0 0 0
1 + 3.50W - - 1 1 1 1 0 0
2 + 3.00W - - 2 2 2 2 0 0
3 - 0.1000W - - 3 3 3 3 4000 10000
4 - 0.0025W - - 4 4 4 4 4000 40000

Supported LBA Sizes (NSID 0x1)
Id Fmt Data Metadt Rel_Perf
0 + 512 0 2
1 - 4096 0 1

=== START OF SMART DATA SECTION ===
SMART overall-health self-assessment test result: PASSED

SMART/Health Information (NVMe Log 0x02)
Critical Warning: 0x00
Temperature: 25 Celsius
Available Spare: 100%
Available Spare Threshold: 10%
Percentage Used: 0%
Data Units Read: 19,767,201 [10.1 TB]
Data Units Written: 24,283,729 [12.4 TB]
Host Read Commands: 256,329,848
Host Write Commands: 207,404,109
Controller Busy Time: 496
Power Cycles: 508
Power On Hours: 2,608
Unsafe Shutdowns: 39
Media and Data Integrity Errors: 0
Error Information Log Entries: 261
Warning Comp. Temperature Time: 2
Critical Comp. Temperature Time: 0

Error Information (NVMe Log 0x01, 16 of 256 entries)
No Errors Logged

@BjoernWaechter
Copy link
Author

I looked a bit into where the interface information comes from. As far as I get it its read here:
https://github.com/truenas/py-SMART/blob/master/pySMART/device_list.py#L121 and takes for example nvme from this --scan result -d argument:
/dev/nvme0 -d nvme # /dev/nvme0, NVMe device
And this output is created here: https://github.com/smartmontools/smartmontools/blob/4b851206498b30c4abac273683d2accaffe98918/smartmontools/smartctl.cpp#L1562

The second occurrence is here:
https://github.com/truenas/py-SMART/blob/master/pySMART/device.py#L261-L278
It takes the type in the single quotes. e.g. nvme here:

smartctl pre-7.4 2023-03-21 r5470 [x86_64-linux-6.2.7-200.fc37.x86_64] (CircleCI)
Copyright (C) 2002-23, Bruce Allen, Christian Franke, www.smartmontools.org

/dev/nvme0: Device of type 'nvme' [NVMe] detected
/dev/nvme0: Device of type 'nvme' [NVMe] opened

This output is created here:
https://github.com/smartmontools/smartmontools/blob/4b851206498b30c4abac273683d2accaffe98918/smartmontools/smartctl.cpp#L1635

I'm not familiar with this C++ code but it looks like a reliable source to me.

@ralequi
Copy link
Collaborator

ralequi commented Feb 14, 2025

I looked a bit into where the interface information comes from. As far as I get it its read here: https://github.com/truenas/py-SMART/blob/master/pySMART/device_list.py#L121 and takes for example nvme from this --scan result -d argument: /dev/nvme0 -d nvme # /dev/nvme0, NVMe device And this output is created here: https://github.com/smartmontools/smartmontools/blob/4b851206498b30c4abac273683d2accaffe98918/smartmontools/smartctl.cpp#L1562

The second occurrence is here: https://github.com/truenas/py-SMART/blob/master/pySMART/device.py#L261-L278 It takes the type in the single quotes. e.g. nvme here:

smartctl pre-7.4 2023-03-21 r5470 [x86_64-linux-6.2.7-200.fc37.x86_64] (CircleCI)
Copyright (C) 2002-23, Bruce Allen, Christian Franke, www.smartmontools.org

/dev/nvme0: Device of type 'nvme' [NVMe] detected
/dev/nvme0: Device of type 'nvme' [NVMe] opened

This output is created here: https://github.com/smartmontools/smartmontools/blob/4b851206498b30c4abac273683d2accaffe98918/smartmontools/smartctl.cpp#L1635

I'm not familiar with this C++ code but it looks like a reliable source to me.

Thank you, but I think none of that code reveals if the underlying storage mechanism is SSD or HDD.

Don't misunderstand me, for me this PR is OK, but if we can improve somehow the code SSD detection, the better.
That's why I would like to know if there is any other way to detect if an NVMe is (or not) an SSD, rather than simply consider it as SSD as default.

From my perspective, and, according to the data I have there it isn't (unless, maybe, accesing ourselves to the smart data...). That's why, maybe @jackeichen may bring us some light that we are missing and probably help us on this.

@jackeichen
Copy link
Contributor

In NVMe Spec 2.0 section 5.17.2.8, I/O Command Set Independent Identify Namespace is Mandatory.
Add Here is what i found:
1739765020767

@jackeichen
Copy link
Contributor

I cannot say that the above method is a common type, at least two challenges here:

  1. nvme spec < 2.0 may not support CNS 08h;
  2. I can't confirm if windows could support pass through CNS 08h;

But it is not a bad ideas to add it to the code.

@BjoernWaechter
Copy link
Author

@jackeichen Do you know if this is somehow visible in the result created by smartctl?

@ralequi ralequi merged commit 92b6329 into truenas:master Feb 17, 2025
15 checks passed
@ralequi ralequi mentioned this pull request Feb 17, 2025
@ralequi
Copy link
Collaborator

ralequi commented Feb 17, 2025

Let's follow the discussion here: #95

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.

NVME interface devices on Windows show is_ssd=False
3 participants