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

module param callbacks check for initialized spa #7521

Merged
merged 1 commit into from May 11, 2018

Conversation

ofaaland
Copy link
Contributor

@ofaaland ofaaland commented May 9, 2018

Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Instead of immediately taking the namespace lock and attemping to
iterate over initialized spa structures, check whether spa_mode_global
is nonzero. This is set by spa_init() after it has initialized the
namespace lock.

Description

In callbacks registered for module parameters, check whether spa_mode_global
is nonzero before taking the namespace lock and attemping to iterate over initialized spa structures.

This is set by spa_init() after it has initialized the namespace lock.

Motivation and Context

Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Without this change, providing these module parameters when loading the zfs module triggers a crash. Applies to zfs_multihost_interval and zfs_vdev_scheduler parameters.

Fixes #7496

How Has This Been Tested?

Local tests loading module.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
	echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
	modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Instead of immediately taking the namespace lock and attemping to
iterate over initialized spa structures, check whether spa_mode_global
is nonzero.  This is set by spa_init() after it has initialized the
namespace lock.

Signed-off-by: Olaf Faaland <faaland1@llnl.gov>
@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #7521 into master will increase coverage by 0.04%.
The diff coverage is 15.38%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7521      +/-   ##
==========================================
+ Coverage   77.26%    77.3%   +0.04%     
==========================================
  Files         336      336              
  Lines      107520   107521       +1     
==========================================
+ Hits        83072    83121      +49     
+ Misses      24448    24400      -48
Flag Coverage Δ
#kernel 77.98% <15.38%> (-0.02%) ⬇️
#user 66.44% <ø> (+0.3%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d1043e2...96b86be. Read the comment docs.

Copy link
Contributor

@dweeezil dweeezil left a comment

Choose a reason for hiding this comment

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

LGTM. Similar case as #7499.

@behlendorf behlendorf merged commit bc5f51c into openzfs:master May 11, 2018
@behlendorf behlendorf added this to To do in 0.7.10 May 11, 2018
@tonyhutter tonyhutter moved this from To do to In progress in 0.7.10 Aug 14, 2018
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 15, 2018
Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
	echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
	modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Instead of immediately taking the namespace lock and attemping to
iterate over initialized spa structures, check whether spa_mode_global
is nonzero.  This is set by spa_init() after it has initialized the
namespace lock.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Olaf Faaland <faaland1@llnl.gov>
Closes openzfs#7496 
Closes openzfs#7521
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2018
Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
	echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
	modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Instead of immediately taking the namespace lock and attemping to
iterate over initialized spa structures, check whether spa_mode_global
is nonzero.  This is set by spa_init() after it has initialized the
namespace lock.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Olaf Faaland <faaland1@llnl.gov>
Closes openzfs#7496 
Closes openzfs#7521
@tonyhutter tonyhutter moved this from In progress to Done in 0.7.10 Aug 30, 2018
@ofaaland ofaaland deleted the b_issue_7496 branch September 5, 2018 15:55
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 5, 2018
Callbacks provided for module parameters are executed both
after the module is loaded, when a user alters it via sysfs, e.g
	echo bar > /sys/modules/zfs/parameters/foo

as well as when the module is loaded with an argument, e.g.
	modprobe zfs foo=bar

In the latter case, the init functions likely have not run yet,
including spa_init() which initializes the namespace lock so it is safe
to use.

Instead of immediately taking the namespace lock and attemping to
iterate over initialized spa structures, check whether spa_mode_global
is nonzero.  This is set by spa_init() after it has initialized the
namespace lock.

Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Tim Chase <tim@chase2k.com>
Signed-off-by: Olaf Faaland <faaland1@llnl.gov>
Closes openzfs#7496 
Closes openzfs#7521
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.7.10
  
Done
Development

Successfully merging this pull request may close these issues.

Crash when setting zfs_multihost_interval at module load
3 participants