Replace cv_{timed}wait_sig with cv_{timed}wait_idle where appropriate #10843

mattmacy · 2020-08-28T21:53:09Z

There are a number of places where cv_?_sig is used simply for
accounting purposes but the surrounding code has no ability to
cope with actually receiving a signal. On FreeBSD it is possible
to send signals to individual kernel threads so this could
enable undesirable behavior.

This patch adds routines on Linux that will do the same idle
accounting as _sig without making the task interruptible. On
FreeBSD cv_*_idle are all aliases for cv_*

Signed-off-by: Matt Macy mmacy@FreeBSD.org

Motivation and Context

Description

How Has This Been Tested?

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.
I have run the ZFS Test Suite with this change applied.
All commit messages are properly formatted and contain Signed-off-by.

module/zfs/txg.c

codecov · 2020-08-29T06:07:53Z

Codecov Report

Merging #10843 into master will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #10843      +/-   ##
==========================================
+ Coverage   79.64%   79.70%   +0.05%     
==========================================
  Files         395      395              
  Lines      125087   125072      -15     
==========================================
+ Hits        99624    99687      +63     
+ Misses      25463    25385      -78

Flag	Coverage Δ
#kernel	`80.34% <100.00%> (-0.03%)`	⬇️
#user	`65.80% <100.00%> (+0.54%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
module/os/linux/spl/spl-condvar.c	`91.83% <100.00%> (-1.79%)`	⬇️
module/zfs/arc.c	`89.45% <100.00%> (+0.10%)`	⬆️
module/zfs/dbuf.c	`93.09% <100.00%> (-0.06%)`	⬇️
module/zfs/mmp.c	`95.72% <100.00%> (ø)`
module/zfs/txg.c	`93.96% <100.00%> (ø)`
module/zfs/vdev_trim.c	`96.10% <100.00%> (+0.59%)`	⬆️
module/zfs/zthr.c	`100.00% <100.00%> (ø)`
module/os/linux/spl/spl-zlib.c	`55.35% <0.00%> (-28.58%)`	⬇️
cmd/zdb/zdb_il.c	`30.86% <0.00%> (-24.08%)`	⬇️
module/zstd/zfs_zstd.c	`80.74% <0.00%> (-4.28%)`	⬇️
... and 72 more

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 6fffc88...ed16fa7. Read the comment docs.

include/sys/zfs_context.h

include/os/freebsd/spl/sys/condvar.h

module/os/linux/spl/spl-condvar.c

There are a number of places where cv_?_sig is used simply for accounting purposes but the surrounding code has no ability to cope with actually receiving a signal. On FreeBSD it is possible to send signals to individual kernel threads so this could enable undesirable behavior. This patch adds routines on Linux that will do the same idle accounting as _sig without making the task interruptible. On FreeBSD cv_*_idle are all aliases for cv_* Signed-off-by: Matt Macy <mmacy@FreeBSD.org>

There are a number of places where cv_?_sig is used simply for accounting purposes but the surrounding code has no ability to cope with actually receiving a signal. On FreeBSD it is possible to send signals to individual kernel threads so this could enable undesirable behavior. This patch adds routines on Linux that will do the same idle accounting as _sig without making the task interruptible. On FreeBSD cv_*_idle are all aliases for cv_* Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matt Macy <mmacy@FreeBSD.org> Closes #10843

There are a number of places where cv_?_sig is used simply for accounting purposes but the surrounding code has no ability to cope with actually receiving a signal. On FreeBSD it is possible to send signals to individual kernel threads so this could enable undesirable behavior. This patch adds routines on Linux that will do the same idle accounting as _sig without making the task interruptible. On FreeBSD cv_*_idle are all aliases for cv_* Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matt Macy <mmacy@FreeBSD.org> Closes openzfs#10843

This change adds SIGSTOP and SIGTSTP handling to the issig function; this mirrors its behavior on Solaris. This way, long running kernel tasks can be stopped with the appropriate signals. Note that doing so with ctrl-z on the command line doesn't return control of the tty to the shell, because tty handling is done separately from stopping the process. That can be future work, if people feel that it is a necessary addition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Issue #810 Issue #10843 Closes #11801

This change adds SIGSTOP and SIGTSTP handling to the issig function; this mirrors its behavior on Solaris. This way, long running kernel tasks can be stopped with the appropriate signals. Note that doing so with ctrl-z on the command line doesn't return control of the tty to the shell, because tty handling is done separately from stopping the process. That can be future work, if people feel that it is a necessary addition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Issue openzfs#810 Issue openzfs#10843 Closes openzfs#11801

There are a number of places where cv_?_sig is used simply for accounting purposes but the surrounding code has no ability to cope with actually receiving a signal. On FreeBSD it is possible to send signals to individual kernel threads so this could enable undesirable behavior. This patch adds routines on Linux that will do the same idle accounting as _sig without making the task interruptible. On FreeBSD cv_*_idle are all aliases for cv_* Reviewed-by: Alexander Motin <mav@FreeBSD.org> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Matt Macy <mmacy@FreeBSD.org> Closes openzfs#10843

This change adds SIGSTOP and SIGTSTP handling to the issig function; this mirrors its behavior on Solaris. This way, long running kernel tasks can be stopped with the appropriate signals. Note that doing so with ctrl-z on the command line doesn't return control of the tty to the shell, because tty handling is done separately from stopping the process. That can be future work, if people feel that it is a necessary addition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Issue openzfs#810 Issue openzfs#10843 Closes openzfs#11801

This change adds SIGSTOP and SIGTSTP handling to the issig function; this mirrors its behavior on Solaris. This way, long running kernel tasks can be stopped with the appropriate signals. Note that doing so with ctrl-z on the command line doesn't return control of the tty to the shell, because tty handling is done separately from stopping the process. That can be future work, if people feel that it is a necessary addition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Issue #810 Issue #10843 Closes #11801

This change adds SIGSTOP and SIGTSTP handling to the issig function; this mirrors its behavior on Solaris. This way, long running kernel tasks can be stopped with the appropriate signals. Note that doing so with ctrl-z on the command line doesn't return control of the tty to the shell, because tty handling is done separately from stopping the process. That can be future work, if people feel that it is a necessary addition. Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Reviewed-by: Matthew Ahrens <mahrens@delphix.com> Signed-off-by: Paul Dagnelie <pcd@delphix.com> Issue openzfs#810 Issue openzfs#10843 Closes openzfs#11801

amotin reviewed Aug 29, 2020

View reviewed changes

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

behlendorf added the Status: Code Review Needed Ready for review and testing label Aug 30, 2020

behlendorf reviewed Aug 30, 2020

View reviewed changes

include/sys/zfs_context.h Outdated Show resolved Hide resolved

mattmacy force-pushed the projects/cv_wait_idle branch from 255c7ef to f011957 Compare August 31, 2020 21:49

behlendorf reviewed Sep 2, 2020

View reviewed changes

include/os/freebsd/spl/sys/condvar.h Show resolved Hide resolved

module/os/linux/spl/spl-condvar.c Outdated Show resolved Hide resolved

mattmacy force-pushed the projects/cv_wait_idle branch from f011957 to ad329d5 Compare September 2, 2020 21:20

mattmacy force-pushed the projects/cv_wait_idle branch from ad329d5 to ed16fa7 Compare September 2, 2020 23:50

amotin approved these changes Sep 3, 2020

View reviewed changes

behlendorf approved these changes Sep 3, 2020

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 3, 2020

behlendorf merged commit ac6e5fb into openzfs:master Sep 4, 2020

mattmacy deleted the projects/cv_wait_idle branch September 10, 2020 23:00

behlendorf mentioned this pull request Nov 11, 2020

'zfs send' aborts on any signal #810

Closed

pcd1193182 mentioned this pull request Mar 25, 2021

Add SIGSTOP and SIGTSTP handling to issig #11801

Merged

13 tasks

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace cv_{timed}wait_sig with cv_{timed}wait_idle where appropriate #10843

Replace cv_{timed}wait_sig with cv_{timed}wait_idle where appropriate #10843

mattmacy commented Aug 28, 2020 •

edited

Loading

codecov bot commented Aug 29, 2020 •

edited

Loading

Replace cv_{timed}wait_sig with cv_{timed}wait_idle where appropriate #10843

Replace cv_{timed}wait_sig with cv_{timed}wait_idle where appropriate #10843

Conversation

mattmacy commented Aug 28, 2020 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

codecov bot commented Aug 29, 2020 • edited Loading

Codecov Report

mattmacy commented Aug 28, 2020 •

edited

Loading

codecov bot commented Aug 29, 2020 •

edited

Loading