OpenZFS 9425 - channel programs can be interrupted #8904

don-brady · 2019-06-14T04:41:37Z

Motivation and Context

OpenZFS-issue: https://www.illumos.org/issues/9425
OpenZFS-commit: illumos/illumos-gate@d0cb1fb926

Description

Problem Statement
ZFS Channel program scripts currently require a timeout, so that hung or long-running scripts return a timeout error instead of causing ZFS to get wedged. This limit can currently be set up to 100 million Lua instructions. Even with a limit in place, it would be desirable to have a sys admin (support engineer) be able to cancel a script that is taking a long time.

Proposed Solution
Make it possible to abort a channel program by sending an interrupt signal. In the underlying txg_wait_sync function, switch the cv_wait to a cv_wait_sig to catch the signal. Once a signal is encountered, the dsl_sync_task function can install a Lua hook that will get called before the Lua interpreter executes a new line of code. The dsl_sync_task can resume with a standard txg_wait_sync call and wait for the txg to complete. Meanwhile, the hook will abort the script and indicate that the channel program was canceled. The kernel returns a EINTR to indicate that the channel program run was canceled.

Porting notes: Added missing return value from cv_wait_sig()

How Has This Been Tested?

ztest
scripts/zfs-tests.sh -T channel_program

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.

Problem Statement ================= ZFS Channel program scripts currently require a timeout, so that hung or long-running scripts return a timeout error instead of causing ZFS to get wedged. This limit can currently be set up to 100 million Lua instructions. Even with a limit in place, it would be desirable to have a sys admin (support engineer) be able to cancel a script that is taking a long time. Proposed Solution ================= Make it possible to abort a channel program by sending an interrupt signal.In the underlying txg_wait_sync function, switch the cv_wait to a cv_wait_sig to catch the signal. Once a signal is encountered, the dsl_sync_task function can install a Lua hook that will get called before the Lua interpreter executes a new line of code. The dsl_sync_task can resume with a standard txg_wait_sync call and wait for the txg to complete. Meanwhile, the hook will abort the script and indicate that the channel program was canceled. The kernel returns a EINTR to indicate that the channel program run was canceled. Porting notes: Added missing return value from cv_wait_sig() Authored by: Don Brady <don.brady@delphix.com> Reviewed by: Sebastien Roy <sebastien.roy@delphix.com> Reviewed by: Serapheim Dimitropoulos <serapheim.dimitro@delphix.com> Reviewed by: Matt Ahrens <matt@delphix.com> Approved by: Robert Mustacchi <rm@joyent.com> Ported-by: Don Brady <don.brady@delphix.com> OpenZFS-issue: https://www.illumos.org/issues/9425 OpenZFS-commit: illumos/illumos-gate@d0cb1fb926

kithrup · 2019-06-14T06:51:22Z

I don't know the Lua environment stuff, so I only paid attention to the rest of it. And it seems fairly straightforward, mostly depending on cv_wait_sig. And then a bunch of context management.

codecov · 2019-06-14T13:21:32Z

Codecov Report

Merging #8904 into master will increase coverage by 0.17%.
The diff coverage is 83.14%.

@@            Coverage Diff             @@
##           master    #8904      +/-   ##
==========================================
+ Coverage   78.51%   78.69%   +0.17%     
==========================================
  Files         382      382              
  Lines      117840   117849       +9     
==========================================
+ Hits        92526    92736     +210     
+ Misses      25314    25113     -201

Flag	Coverage Δ
#kernel	`79.34% <84.33%> (+0.11%)`	⬆️
#user	`67.31% <52.38%> (+0.28%)`	⬆️

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 b1b4ac2...d476a0f. Read the comment docs.

module/zfs/dsl_synctask.c

module/zfs/txg.c

module/zfs/zcp.c

Signed-off-by: Don Brady <don.brady@delphix.com>

shartse

I left a few small notes, but overall this seems good to me.

shartse · 2019-06-21T18:43:13Z

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.terminate_by_signal.ksh

+#
+sleep 10
+log_pos pkill -P $CHILD
+log_pos kill $CHILD


Are the exit statuses we could check here to get more information about whether or not the interrupt was successful?

shartse · 2019-06-21T18:43:37Z

tests/zfs-tests/tests/functional/channel_program/synctask_core/tst.terminate_by_signal.ksh

+# After waiting, send a kill signal to the channel program process.
+# This should stop the ZCP near a million instructions but still have
+# created some of the snapshots. Note that since the above zfs program
+# command might get wrapped, we also issue a kill to the group.


What does "might get wrapped" mean in this context?

When the ZTS is run in-tree the zfs command is a script that wraps the actual binary.

module/zfs/zcp.c

shartse · 2019-06-21T18:54:59Z

module/zfs/txg.c

+		if (wait_sig) {
+			/*
+			 * Condition wait here but stop if the thread receives a
+			 * signal. The caller may call txg_wait_synced*() again


This wording was a little confusing to me - stop is the opposite of continue, but we continue if we're no longer waiting.
Maybe something like: "Condition wait here but continue execution if the thread receives a signal."

Problem Statement ================= ZFS Channel program scripts currently require a timeout, so that hung or long-running scripts return a timeout error instead of causing ZFS to get wedged. This limit can currently be set up to 100 million Lua instructions. Even with a limit in place, it would be desirable to have a sys admin (support engineer) be able to cancel a script that is taking a long time. Proposed Solution ================= Make it possible to abort a channel program by sending an interrupt signal.In the underlying txg_wait_sync function, switch the cv_wait to a cv_wait_sig to catch the signal. Once a signal is encountered, the dsl_sync_task function can install a Lua hook that will get called before the Lua interpreter executes a new line of code. The dsl_sync_task can resume with a standard txg_wait_sync call and wait for the txg to complete. Meanwhile, the hook will abort the script and indicate that the channel program was canceled. The kernel returns a EINTR to indicate that the channel program run was canceled. Porting notes: Added missing return value from cv_wait_sig() Authored by: Don Brady <don.brady@delphix.com> Reviewed by: Sebastien Roy <sebastien.roy@delphix.com> Reviewed by: Serapheim Dimitropoulos <serapheim.dimitro@delphix.com> Reviewed by: Matt Ahrens <matt@delphix.com> Reviewed by: Sara Hartse <sara.hartse@delphix.com> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Approved by: Robert Mustacchi <rm@joyent.com> Ported-by: Don Brady <don.brady@delphix.com> Signed-off-by: Don Brady <don.brady@delphix.com> OpenZFS-issue: https://www.illumos.org/issues/9425 OpenZFS-commit: illumos/illumos-gate@d0cb1fb926 Closes openzfs#8904

Problem Statement ================= ZFS Channel program scripts currently require a timeout, so that hung or long-running scripts return a timeout error instead of causing ZFS to get wedged. This limit can currently be set up to 100 million Lua instructions. Even with a limit in place, it would be desirable to have a sys admin (support engineer) be able to cancel a script that is taking a long time. Proposed Solution ================= Make it possible to abort a channel program by sending an interrupt signal.In the underlying txg_wait_sync function, switch the cv_wait to a cv_wait_sig to catch the signal. Once a signal is encountered, the dsl_sync_task function can install a Lua hook that will get called before the Lua interpreter executes a new line of code. The dsl_sync_task can resume with a standard txg_wait_sync call and wait for the txg to complete. Meanwhile, the hook will abort the script and indicate that the channel program was canceled. The kernel returns a EINTR to indicate that the channel program run was canceled. Porting notes: Added missing return value from cv_wait_sig() Authored by: Don Brady <don.brady@delphix.com> Reviewed by: Sebastien Roy <sebastien.roy@delphix.com> Reviewed by: Serapheim Dimitropoulos <serapheim.dimitro@delphix.com> Reviewed by: Matt Ahrens <matt@delphix.com> Reviewed by: Sara Hartse <sara.hartse@delphix.com> Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov> Approved by: Robert Mustacchi <rm@joyent.com> Ported-by: Don Brady <don.brady@delphix.com> Signed-off-by: Don Brady <don.brady@delphix.com> OpenZFS-issue: https://www.illumos.org/issues/9425 OpenZFS-commit: illumos/illumos-gate@d0cb1fb926 Closes #8904

behlendorf reviewed Jun 14, 2019

View reviewed changes

module/zfs/dsl_synctask.c Outdated Show resolved Hide resolved

module/zfs/txg.c Outdated Show resolved Hide resolved

module/zfs/zcp.c Show resolved Hide resolved

behlendorf added the Status: Code Review Needed Ready for review and testing label Jun 14, 2019

Add cv_wait_io_sig

d476a0f

Signed-off-by: Don Brady <don.brady@delphix.com>

behlendorf approved these changes Jun 19, 2019

View reviewed changes

behlendorf requested a review from sdimitro June 19, 2019 18:56

shartse approved these changes Jun 21, 2019

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 22, 2019

behlendorf merged commit 186898b into openzfs:master Jun 22, 2019

h1z1 mentioned this pull request May 7, 2022

Allow some zpool commands to timeout #13427

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenZFS 9425 - channel programs can be interrupted #8904

OpenZFS 9425 - channel programs can be interrupted #8904

don-brady commented Jun 14, 2019

kithrup commented Jun 14, 2019

codecov bot commented Jun 14, 2019 •

edited

Loading

shartse left a comment

shartse Jun 21, 2019

shartse Jun 21, 2019

don-brady Jun 22, 2019

shartse Jun 21, 2019

OpenZFS 9425 - channel programs can be interrupted #8904

OpenZFS 9425 - channel programs can be interrupted #8904

Conversation

don-brady commented Jun 14, 2019

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

kithrup commented Jun 14, 2019

codecov bot commented Jun 14, 2019 • edited Loading

Codecov Report

shartse left a comment

Choose a reason for hiding this comment

shartse Jun 21, 2019

Choose a reason for hiding this comment

shartse Jun 21, 2019

Choose a reason for hiding this comment

don-brady Jun 22, 2019

Choose a reason for hiding this comment

shartse Jun 21, 2019

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2019 •

edited

Loading