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

Sector Size empty for 512e drives #1590

Closed
chrstphrchvz opened this Issue Dec 19, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@chrstphrchvz
Contributor

chrstphrchvz commented Dec 19, 2016

For 512e hard drives, the smartctl information section contains

Sector Sizes:     512 bytes logical, 4096 bytes physical

However, this doesn't match the Sector Size item that Rockstor expects, so the field on the Web UI is empty:
screen shot 2016-12-18 at 6 59 21 pm

For reference, Sector Size as would appears for other hard drives:

Sector Size:      512 bytes logical/physical

(Not sure about 4k native; I don't have such a device to test, and haven't come across other people's examples.)

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet Dec 19, 2016

Member

@chrstphrchvz Thanks for yet another excellent bug report. Yes as you say it's looking to match "Sector Size:" and consequently not finding "Sector Sizes:" (with the additional s). This will hopefully be an easy enough fix by simply modifying the string match criteria that strips this info from the smartctl output.

Nice find and thanks for taking the time to report this.

Do you fancy having a go at the fix? Might be a nice one to start with.

Our Developers section of the Contributing to Rockstor - Overview should help with getting setup ready for a pull request:
http://rockstor.com/docs/contribute.html#developers

I'd start by looking at the 'info()' function in src/rockstor/system/smart.py and in particular the following line:

https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/system/smart.py#L57

There is already a dual string match there so you could try adding an additional third 'or' directive that includes your finding of "Sector Sizes:" that should do it hopefully.

A Rockstor service restart will be required after the change for it to take effect though. That and a smart Refresh button press of course.

Let us know how you get on and note that you only really have to get your dev environment setup once and there after you can just rinse and repeat for each thing you tackle.

If you do fancy having a go at this one just note down here that you are looking into it, that way others can leave it to you. Also any questions on this issue can also go here to keep things contained and focused.

Hope you take up the challenge as you've pretty much worked out what's wrong already anyway, just the final proof of fix to go.

Member

phillxnet commented Dec 19, 2016

@chrstphrchvz Thanks for yet another excellent bug report. Yes as you say it's looking to match "Sector Size:" and consequently not finding "Sector Sizes:" (with the additional s). This will hopefully be an easy enough fix by simply modifying the string match criteria that strips this info from the smartctl output.

Nice find and thanks for taking the time to report this.

Do you fancy having a go at the fix? Might be a nice one to start with.

Our Developers section of the Contributing to Rockstor - Overview should help with getting setup ready for a pull request:
http://rockstor.com/docs/contribute.html#developers

I'd start by looking at the 'info()' function in src/rockstor/system/smart.py and in particular the following line:

https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/system/smart.py#L57

There is already a dual string match there so you could try adding an additional third 'or' directive that includes your finding of "Sector Sizes:" that should do it hopefully.

A Rockstor service restart will be required after the change for it to take effect though. That and a smart Refresh button press of course.

Let us know how you get on and note that you only really have to get your dev environment setup once and there after you can just rinse and repeat for each thing you tackle.

If you do fancy having a go at this one just note down here that you are looking into it, that way others can leave it to you. Also any questions on this issue can also go here to keep things contained and focused.

Hope you take up the challenge as you've pretty much worked out what's wrong already anyway, just the final proof of fix to go.

@chrstphrchvz

This comment has been minimized.

Show comment
Hide comment
@chrstphrchvz

chrstphrchvz Dec 19, 2016

Contributor

Thanks @phillxnet for the developer info and narrowing down what part of the code to try. This may indeed be something I can come up with a patch for.

I also came across another library called pySMART (smartctl wrapper for Python); I haven't tried it, but I wonder if it would be of use to Rockstor.

Contributor

chrstphrchvz commented Dec 19, 2016

Thanks @phillxnet for the developer info and narrowing down what part of the code to try. This may indeed be something I can come up with a patch for.

I also came across another library called pySMART (smartctl wrapper for Python); I haven't tried it, but I wonder if it would be of use to Rockstor.

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet Dec 20, 2016

Member

Glad your up for the fix and no time pressure as this is quite a corner case and not critical but would definitely be a nice to have, akin to having an additional code contributor as well of course.

As for the pySMART library I'm personally a little reluctant to load on additional dependencies and given it hasn't seen an update for 18 months I'm not too sure how it's going to deal with newer drives that end up presenting things just a little differently, or even older drives for that matter. Which would mean we would end up taking on the maintenance of that library too. If you look at Rockstor existing SMART parsing code there have been numurous modifications required, such as reading smart data through various raid controllers etc, such adaptations may very well not be developed or accounted for in other libraries / code bases. Essentially Rockstor own existing smart code already does what this library purports to do but is more frequently maintained and has an existing and current track record. So it's definitely good to know about but then there are python libraries for everything really, and each may bring their own pros and cons. In this case from a quick look I don't think the pros out way the cons of unknown and untested field performance, which Rockstor already has (see the numerous smart fixes and improvements in the past 18 months alone) and the non trivial task of moving everything over to a different way of working with this data. Especial as our own smart parsers are now just settling down.

See what you thing once you've had a good look at what's there already and good luck with getting the dev enviroment up and running. I use VMM / KVM but I know a number of other Rockstor developers use VirtualBox.

Member

phillxnet commented Dec 20, 2016

Glad your up for the fix and no time pressure as this is quite a corner case and not critical but would definitely be a nice to have, akin to having an additional code contributor as well of course.

As for the pySMART library I'm personally a little reluctant to load on additional dependencies and given it hasn't seen an update for 18 months I'm not too sure how it's going to deal with newer drives that end up presenting things just a little differently, or even older drives for that matter. Which would mean we would end up taking on the maintenance of that library too. If you look at Rockstor existing SMART parsing code there have been numurous modifications required, such as reading smart data through various raid controllers etc, such adaptations may very well not be developed or accounted for in other libraries / code bases. Essentially Rockstor own existing smart code already does what this library purports to do but is more frequently maintained and has an existing and current track record. So it's definitely good to know about but then there are python libraries for everything really, and each may bring their own pros and cons. In this case from a quick look I don't think the pros out way the cons of unknown and untested field performance, which Rockstor already has (see the numerous smart fixes and improvements in the past 18 months alone) and the non trivial task of moving everything over to a different way of working with this data. Especial as our own smart parsers are now just settling down.

See what you thing once you've had a good look at what's there already and good luck with getting the dev enviroment up and running. I use VMM / KVM but I know a number of other Rockstor developers use VirtualBox.

@chrstphrchvz

This comment has been minimized.

Show comment
Hide comment
@chrstphrchvz

chrstphrchvz Dec 20, 2016

Contributor

Thanks for the explanation.

I may need some assistance in testing the patch. I normally use VirtualBox, but it doesn't appear to currently have a way to control the emulated block size, so I may have to try something else like QEMU since I don't have a USB adapter with SMART passthrough or machine supporting PCI passthrough.

Contributor

chrstphrchvz commented Dec 20, 2016

Thanks for the explanation.

I may need some assistance in testing the patch. I normally use VirtualBox, but it doesn't appear to currently have a way to control the emulated block size, so I may have to try something else like QEMU since I don't have a USB adapter with SMART passthrough or machine supporting PCI passthrough.

@phillxnet

This comment has been minimized.

Show comment
Hide comment
@phillxnet

phillxnet Dec 20, 2016

Member

@chrstphrchvz Yes of course, I neglected to mention the script recently added to Rockstor that grabs the raw output of all the smart command run internally by Rockstor and zips them up ready to be posted on the forum.
It was added in pull request #1507 and you will find it in:

src/rockstor/scripts/dump-smart.sh

so on a regular rpm installed Rockstor system it would be at:

/opt/rockstor/src/rockstor/scripts/dump-smart.sh

If you run that program on your real hw Rockstor machine used in the original report with the relevant device name it should execute and zip up the output of all the smart commands and an lsblk command ready for you to unzip them into your development environments install so that the output dump files end up directly in:

/root/smartdumps/

With their names as there were. If you look to the code in the mentioned file you will see what file names it looks for when in test mode, and the script has been made to output these same file names.

Enable SMART test mode by changing line 34:
https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/system/smart.py#L34
from:

TESTMODE = False

to read:

TESTMODE = True

That way the smart output of all your drives in your development environment will appear to have the exact same SMART output as is seen on your real hw as per the designated drive used with the script.

Not perfect but great for reproducing user submitted SMART issues as one can reproduce locally exactly what they see on their hardware. Of course in your case you also have the real hardware to try your fix out on once you've played around with it in the safety of your vm development environment.

And remember to change that TESTMODE constant back prior to submitting your patch, else if it gets missed everyone SMART will suddenly fail to work as it looks for those smart dump file in the /root/smartdumps and obviously fails.

I'm anticipating the fix to be no more than a change to that one line mentioned in my earlier post so should be pretty easy to hand duplicate on your real hw once you've proved it in the VM. Best you see the issue reproduced first in the vm via the above method prior to attempting the fix of course.

Hopefully that should do you. Sorry I should have thought about this rather tricky element whilst writing my last post. This SMART development method does need documenting so maybe that could be a new issue over in the https://github.com/rockstor/rockstor-doc repo if you fancy creating it. It may even serve as a nice follow-up to this one if you fancied tacking that too, later down the line, maybe as a little how-to linked from the contributor dev section or something.

The above method is how I have done a number of reported smart parsing issues from forum members and although clunky does do the job.

Hope that helps.

Member

phillxnet commented Dec 20, 2016

@chrstphrchvz Yes of course, I neglected to mention the script recently added to Rockstor that grabs the raw output of all the smart command run internally by Rockstor and zips them up ready to be posted on the forum.
It was added in pull request #1507 and you will find it in:

src/rockstor/scripts/dump-smart.sh

so on a regular rpm installed Rockstor system it would be at:

/opt/rockstor/src/rockstor/scripts/dump-smart.sh

If you run that program on your real hw Rockstor machine used in the original report with the relevant device name it should execute and zip up the output of all the smart commands and an lsblk command ready for you to unzip them into your development environments install so that the output dump files end up directly in:

/root/smartdumps/

With their names as there were. If you look to the code in the mentioned file you will see what file names it looks for when in test mode, and the script has been made to output these same file names.

Enable SMART test mode by changing line 34:
https://github.com/rockstor/rockstor-core/blob/master/src/rockstor/system/smart.py#L34
from:

TESTMODE = False

to read:

TESTMODE = True

That way the smart output of all your drives in your development environment will appear to have the exact same SMART output as is seen on your real hw as per the designated drive used with the script.

Not perfect but great for reproducing user submitted SMART issues as one can reproduce locally exactly what they see on their hardware. Of course in your case you also have the real hardware to try your fix out on once you've played around with it in the safety of your vm development environment.

And remember to change that TESTMODE constant back prior to submitting your patch, else if it gets missed everyone SMART will suddenly fail to work as it looks for those smart dump file in the /root/smartdumps and obviously fails.

I'm anticipating the fix to be no more than a change to that one line mentioned in my earlier post so should be pretty easy to hand duplicate on your real hw once you've proved it in the VM. Best you see the issue reproduced first in the vm via the above method prior to attempting the fix of course.

Hopefully that should do you. Sorry I should have thought about this rather tricky element whilst writing my last post. This SMART development method does need documenting so maybe that could be a new issue over in the https://github.com/rockstor/rockstor-doc repo if you fancy creating it. It may even serve as a nice follow-up to this one if you fancied tacking that too, later down the line, maybe as a little how-to linked from the contributor dev section or something.

The above method is how I have done a number of reported smart parsing issues from forum members and although clunky does do the job.

Hope that helps.

@schakrava schakrava closed this in 03f1c2a Feb 6, 2017

schakrava added a commit that referenced this issue Feb 6, 2017

Merge pull request #1620 from chrstphrchvz/patch-1
Show sector sizes for 512e disks. Fixes #1590

@schakrava schakrava added bug enhancement and removed bug labels Mar 17, 2017

@schakrava schakrava added this to the Pinnacles milestone Mar 17, 2017

@schakrava schakrava assigned chrstphrchvz and unassigned phillxnet Mar 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment