smart for nvme drives #3013 #3014
Conversation
|
@delboy711 This is nice, and a very welcome addition. Thanks. It's been bugging me for some time - but alas I have no current nvme access to develop against - did have but alas a move/emigration forced some significant downsizing. I've edited the PR text a tad re adding the issue counterpart you created, thanks. This is required so we can tie them together and helps with attribution. Marking for review from someone with access to a real nvme hopefully :) . Thanks also for the detailed issue & PR (reviewers should reference the issue also). If no core developer has nvme to hand on a test machine then no-worries, we are just about to embark on a fresh testing phase and as @delboy711 states: there should be no interaction with the existing non-nvme function anyway. Plus from the issue #3013 (comment) we have:
Also, well done on finding that tab/spaces pattern addition. This side of the SMART data does tent towards fragility - but of late we have had few failure reports. But of course the nvme output is all different again!!! So I guess we are now on another run of tidy-ups re variously outputs as-and-when we encounter them. But all good in time. A small suggestion: maybe if we have no test report info, the spoofing of 1 min and 5 min could be reprenseted as "unknown" or "unreported" or the like: given that is what is the case here. Otherwise we essentially miss-lead folks into expecting a 1 or 5 minute test when in fact we have no idea :) . Also if you could add a description to the commit that indicates what was intended here that would be great: I don't like to depend only on GitHub references and we normally have our non-trivial commits contain the essense of what was attempted. This ensures that our git repo itself - that may not be in GitHub for-ever, will maintain a history of what was intended. |
Since smartctl does not report available self tests or their duration for nvme drives, this information is spoofed in the capabilities Tab.
|
I have found examples of nvme smartctl error logs on the web and have tested them with this PR, and they worked perfectly. |
|
While this PR does provide useful support for nvme drives in Rockstor, it could be better. To get that information on the Identity Tab would require a rewrite of the parsing of smartctl and the model Classes, and would take some time. What do you think @phillxnet is the Identity Tab as shown here "good enough", or should I try to do better? |
|
@delboy711 Re:
I think any improvement on nothing for nvme devices (prior state before this patch) is a win; and as you say the next stage here
which is definitely a larger endeavour. However now you have identified, and implemented, the more obvious matches, it is now clearer how/where we have to adapt. Likely as you say this would be an extension to the existing models, and potentially skipping empty entries within the display. That is deeper than this first pass and so can easily qualify as a follow-up pull request; referencing this pull request as tending to the existing cross-over/commonality within our existing models and display. |
|
Let us know when you think this is ready for final review, and if possible a squash to a single commit would be good before hand. We can then get this into the wild, in testing, and move on to the model extension and parsing improvements as interest and resources (human mainly) present themselves :) . Plus we have some significant progress here already - within the current limitations of our existing structures anyway. Do feel free to extend and re-work as you see fit, but there is definitely no problem in a partial improvement, there are always improvements to be had. When we first did our SMART info parsing & presentation it worked a treat on the simulated output from qemu sata devices, and for all drives held by the developers at that time. But once released into the wild we had a long string of non-conforming output that broke what we had in sometimes quite subtle ways. I'm anticipating the same with this move into nvme smart parsing. So all-in, when tending to real-hardware, and the myriad of ways manufacturers interpret more or less exact specifications, we have to wing-it a tad. Ergo bit-by-bit facilitates adapting our systems as and when folks show an interest in improving them either via feedback, or such as you have just done here, via pull requests. Thanks again for taking a looks at this, and I'm sure you have now seen some of the adaptations we have had to make over the years in our existing parsing. In time, with sufficient attention, we should also have a full presentation for at least the common nvme manufacturers SMART output on these devices. But as always these things take time and testing on a range of real hardware to get to a polished state. |
|
I suggest first finishing the improvement you've started now, and embark on the rewrite in a separate PR. Not sure how far along this is, but I will gladly test the PR on my nvme disk. |
|
Upon build & restart I get the following error: |
|
Thanks for your feedback @kanecko Could you please post the results of the command It will make the message a bit easier to read. |
|
I tried this PR today with an nvme drive connected by a USB adapter. The drive was recognised OK by Rockstor and could be formatted and used, but smartctl errors with the message On giving the option '-d sntjmicron' , smartctl then worked normally. On further investigation I found that smartmontools 7.5 on my Arch Linux desktop works Ok, but version 7.4 as provided in Suse Leap 15-6 does not recognise newer USB adapters. |
|
|
Thanks @kanecko Looks to be a known bug in smartmon tools https://github.com/smartmontools/smartmontools/issues/217 Could you try each of No need to post the results. Just report if each gives the error ' Invalid Field in Command ' Also if you have access to another computer with smartmontools 7.5 we can confirm if it is already fixed in the latest release. |
|
smartmontool 7.4 results:
upgraded to smartmontool 7.5 and retried all cmds:
|
Changes to src/rockstor/system/smart.py to parse smartctl output for nvme drives to populate the existing s.m.a.r.t. pages with nvme data wherever possible.
Fixes #3013
It should be possible to start self tests, and display test logs in the same way as Sata drives.
I have not tested with error logs since I do not have any drives with errors. If anyone can supply error log output for an nvme drive that would be useful.