search

davidker / unisys

Master repository for new changes to drivers/staging/unisys and drivers/visorbus
Other
2 stars 1 forks source link

Revise and expose visorhba visordisk error variables #74

Open mriswyth opened 8 years ago

mriswyth commented 8 years ago

visorhba's visordisk_info ios_threshold and error_count members are implemented for error messages that were logged in earlier versions of the driver. Now they are only counted and not exposed at all.

The most useful way to address this is to remove the ios_threshold since we won't be printing a message on each error occurrence and split error_count into abort_count, device_reset_count, bus_reset_count, command_error_count, and busy_count for more granular tracking of disk errors.

These values should then also be exposed as debugfs values to be useful.

mriswyth commented 8 years ago

Kanban 1566

mriswyth commented 8 years ago

I removed the ios_threshold macros and variables.

I replaced visordisk_info error_count and abort_count, device_reset_count, bus_reset_count, and command_error_count. These are now displayed along with the channel:id:lun of each visordisk when displaying the debugfs info entry for the visorhba.

I added a busy_count to visorhba_devdata to track the number of times the hba returned a busy result.

The upstream-next version of these changes was pushed to githubissue#74-upstream-next.

I compared these changes to what would happen in for-later and all the changes are reasonable in for-later as well. There are some merge conflicts for for-later that will need to be fixed up, but that is all.

selltc commented 8 years ago

I looked at all of your patches and just have a few comments:

mriswyth commented 8 years ago

Thank you so much, Tim. I have made changes based on your comments and pushed a new branch at githubissue#74-upstream-next-v2.

Commits are now

selltc commented 8 years ago

Looks good now. Thanks Bryan.

Reviewed-by: Tim Sell <Timothy.Sell@unisys.com>

selltc commented 8 years ago

I just realized that there is a mistake in commit aada7ce7f28008a829f523910d542c7b2f4726cf (staging: unisys: visorhba: Remove unused logging threshold), revealed to me when this went thru SonarQube analysis (SQ-AVVbdcvm49CIuPuSzhop) via upstream-next commit 68c96570504ff42878ab7dd284ce38f8492ec658:

    } else {
        devdata = (struct visorhba_devdata *)scsidev->host->hostdata;
-       for_each_vdisk_match(vdisk, devdata, scsidev) {
-           if (atomic_read(&vdisk->ios_threshold) > 0) {
-               atomic_dec(&vdisk->ios_threshold);
-               if (atomic_read(&vdisk->ios_threshold) == 0)
-                   atomic_set(&vdisk->error_count, 0);
-           }
-       }

In this code, the devdata assignment is extraneous, because the value is never used.

davidker commented 8 years ago

Okay, I can pull those as well! Thanks for the catch!

davidker commented 8 years ago

Tim, I think I need to sit down with you sometime or do something on T710-1 and R730-1 to get the SonarQube automated so we don't run into these during the daily build.

selltc commented 8 years ago

The instructions at https://ustr-wiki-1.na.uis.unisys.com/spar/index.php/S-Par_Use_of_SonarQube_Static_Source_Analysis#Upstream_kernel explain how to use

build_upstream.sh -q

to do developer analysis, which is the basis for what I think we need here.

E.g., this will provide you with a .html file detailing SonarQube issues that have been introduced or fixed as compared to the most-recent server analysis:

[selltc@mac davidker-unisys]$ set | grep SONAR
SONAR_ANALYSIS_MODE=preview-html
SONAR_LOG_LEVEL=DEBUG
SONAR_PROJECT_VERSION=trunk
SONAR_SERVER=tr-spardev.tr.unisys.com
SONAR_SERVER_PORT=9000
SONAR_SERVER_JDBC_PORT=3306
[selltc@mac davidker-unisys]$ cat build_upstream.settings
GIT_REPO_URL=https://github.com/davidker/unisys.git
GIT_REPO=origin
GIT_BRANCH=upstream
KERNEL_SOURCE_DIR=.
KERNEL_BASE_DOT_CONFIG=build_upstream.config
export GIT_SSL_NO_VERIFY=1
[selltc@mac davidker-unisys]$ ../spar/trunk/sp-buildroot/scripts/build_upstream.sh -fsm -q s-Par-Linux-Upstream build_upstream.settings
...
15:33:20.963 INFO  - ANALYSIS SUCCESSFUL
15:33:20.965 DEBUG - Post-jobs :
INFO: ------------------------------------------------------------------------
INFO: EXECUTION SUCCESS
INFO: ------------------------------------------------------------------------
Total time: 30.143s
Final Memory: 22M/430M
INFO: ------------------------------------------------------------------------
Local SonarQube analysis results in s-Par-Linux-Upstream-issues/issues-report.html (in /home/selltc/projects/davidker-unisys)
SonarQube s-Par-Linux-Upstream: success.
[selltc@mac davidker-unisys]$ firefox ./s-Par-Linux-Upstream-issues/issues-report.html

For automation purposes, we can make use of:

export SONAR_ANALYSIS_MODE=preview-terse

which will produce more-parseable output in response to:

../spar/trunk/sp-buildroot/scripts/build_upstream.sh -fsm -q s-Par-Linux-Upstream build_upstream.settings

like this:

15:56:31.838 INFO  -

-------------  Issues Report  -------------

        +5 issues

        +5 minor

-------------------------------------------

15:56:31.838 INFO  - ANALYSIS SUCCESSFUL
15:56:31.840 DEBUG - Post-jobs :
INFO: ------------------------------------------------------------------------
INFO: EXECUTION SUCCESS
INFO: ------------------------------------------------------------------------
Total time: 28.784s
Final Memory: 20M/420M
INFO: ------------------------------------------------------------------------
SonarQube s-Par-Linux-Upstream: success.

Soon I will include an example of what the SONAR_ANALYSIS_MODE=preview-terse output would look like when there are NO new SonarQube issues introduced. That will undoubtedly be useful for automation.

davidker commented 8 years ago

Awesome Tim, thanks! I think this is a good start for what I'm looking for. I will most likely be running this on the new upstream-next. I assume the deltas are from the last "official" run of upstream-next from the daily-build time?

selltc commented 8 years ago

Yep.

selltc commented 8 years ago

So here's what SONAR_ANALYSIS_MODE=preview-terse output looks like when there are NO new SonarQube issues introduced:

[selltc@mac davidker-unisys]$ ../spar/trunk/sp-buildroot/scripts/build_upstream.sh -fsm -q s-Par-Linux-Upstream build_upstream.settings
...
22:34:36.751 INFO  -

-------------  Issues Report  -------------

  No new issue

-------------------------------------------

22:34:36.752 INFO  - ANALYSIS SUCCESSFUL
INFO: ------------------------------------------------------------------------
INFO: EXECUTION SUCCESS
INFO: ------------------------------------------------------------------------
Total time: 16.858s
Final Memory: 8M/170M
INFO: ------------------------------------------------------------------------
SonarQube s-Par-Linux-Upstream: success.
selltc commented 8 years ago

If you include smatch results, e.g. with:

$ ../spar/trunk/sp-buildroot/scripts/build_upstream.sh -fs -q s-Par-Linux-Upstream build_upstream.settings

then this subsequent grep will show if there are any smatch issues reported for s-Par drivers:

$ grep -e visorbus -e visorhba -e visornic -e visorinput s-Par-Linux-Upstream.analyze.smatch | grep -e 'warn:' -e 'error:'

I.e., if the grep produces NO output, then there are no s-Par smatch issues.

selltc commented 8 years ago

FWIW, I have added all the SonarQube and smatch info from above to our wiki at https://ustr-wiki-1.na.uis.unisys.com/spar/index.php/S-Par_Use_of_SonarQube_Static_Source_Analysis#Upstream_kernel.

davidker commented 7 years ago

These patches are in for-later can this be closed?