Alter zpool status output to indicate MMP suspended the pool #7296
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tonyhutter
approved these changes
Mar 13, 2018
dinatale2
approved these changes
Mar 13, 2018
behlendorf
requested changes
Mar 13, 2018
cmd/zpool/zpool_main.c
Outdated
| "system could import the pool undetected.\n")); | ||
| (void) printf(gettext("action: Make sure the pool's devices " | ||
| "are connected, then reboot your system and import the " | ||
| "pool.\n")); |
There was a problem hiding this comment.
Please mind the 80 char limit in the output, both of these lines need to be manually wrapped with \n\t\.
cmd/zpool/zpool_main.c
Outdated
| @@ -6467,6 +6467,15 @@ status_callback(zpool_handle_t *zhp, void *data) | |||
| "to be recovered.\n")); | |||
| break; | |||
|
|
|||
| case ZPOOL_STATUS_IO_FAILURE_MMP: | |||
| (void) printf(gettext("status: The pool is suspended because " | |||
| "multihost writes failed or were delayed, and another " | |||
There was a problem hiding this comment.
To my ear the phrasing here doesn't flow quite right, maybe replace ", and another system..." with "; another system...".
f7dd71e to
17d79ec
Compare
Codecov Report
@@ Coverage Diff @@
## master #7296 +/- ##
==========================================
- Coverage 76.36% 76.2% -0.17%
==========================================
Files 328 328
Lines 104023 104011 -12
==========================================
- Hits 79439 79263 -176
- Misses 24584 24748 +164
Continue to review full report at Codecov.
|
behlendorf
approved these changes
Mar 14, 2018
gmelikov
approved these changes
Mar 14, 2018
behlendorf
requested changes
Mar 14, 2018
include/sys/zio.h
Outdated
| ZIO_SUSPEND_NONE = 0, | ||
| ZIO_SUSPEND_IOERR, | ||
| ZIO_SUSPEND_MMP, | ||
| }; |
There was a problem hiding this comment.
Please add zio_suspend_reason_t as a typedef and use that throughout.
When the pool is suspended, record whether it was due to an I/O error or due to MMP writes failing to succeed within the required time. Change spa_suspended from uint8_t to zio_suspend_reason_t to store the reason. When userspace queries pool status via spa_tryimport(), report the reason the pool was suspended in a new key, ZPOOL_CONFIG_SUSPENDED_REASON. In libzfs, when interpreting the returned config nvlist, report suspension due to MMP with a new pool status enum value, ZPOOL_STATUS_IO_FAILURE_MMP. In status_callback(), which generates and emits the message when 'zpool status' is executed, add a case to print an appropriate message for the new pool status enum value. Signed-off-by: Olaf Faaland <[email protected]>
17d79ec to
543d0a6
Compare
behlendorf
approved these changes
Mar 14, 2018
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Apr 16, 2018
When the pool is suspended, record whether it was due to an I/O error or due to MMP writes failing to succeed within the required time. Change spa_suspended from uint8_t to zio_suspend_reason_t to store the reason. When userspace queries pool status via spa_tryimport(), report the reason the pool was suspended in a new key, ZPOOL_CONFIG_SUSPENDED_REASON. In libzfs, when interpreting the returned config nvlist, report suspension due to MMP with a new pool status enum value, ZPOOL_STATUS_IO_FAILURE_MMP. In status_callback(), which generates and emits the message when 'zpool status' is executed, add a case to print an appropriate message for the new pool status enum value. Reviewed-by: George Melikov <[email protected]> Reviewed-by: Giuseppe Di Natale <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Olaf Faaland <[email protected]> Closes openzfs#7296
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
May 4, 2018
When the pool is suspended, record whether it was due to an I/O error or due to MMP writes failing to succeed within the required time. Change spa_suspended from uint8_t to zio_suspend_reason_t to store the reason. When userspace queries pool status via spa_tryimport(), report the reason the pool was suspended in a new key, ZPOOL_CONFIG_SUSPENDED_REASON. In libzfs, when interpreting the returned config nvlist, report suspension due to MMP with a new pool status enum value, ZPOOL_STATUS_IO_FAILURE_MMP. In status_callback(), which generates and emits the message when 'zpool status' is executed, add a case to print an appropriate message for the new pool status enum value. Reviewed-by: George Melikov <[email protected]> Reviewed-by: Giuseppe Di Natale <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Olaf Faaland <[email protected]> Closes openzfs#7296
tonyhutter
pushed a commit
that referenced
this pull request
May 10, 2018
When the pool is suspended, record whether it was due to an I/O error or due to MMP writes failing to succeed within the required time. Change spa_suspended from uint8_t to zio_suspend_reason_t to store the reason. When userspace queries pool status via spa_tryimport(), report the reason the pool was suspended in a new key, ZPOOL_CONFIG_SUSPENDED_REASON. In libzfs, when interpreting the returned config nvlist, report suspension due to MMP with a new pool status enum value, ZPOOL_STATUS_IO_FAILURE_MMP. In status_callback(), which generates and emits the message when 'zpool status' is executed, add a case to print an appropriate message for the new pool status enum value. Reviewed-by: George Melikov <[email protected]> Reviewed-by: Giuseppe Di Natale <[email protected]> Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Olaf Faaland <[email protected]> Closes #7296
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
When the pool is suspended, record whether it was due to an I/O error
or due to MMP writes failing to succeed within the required time.
When userspace queries pool status via spa_tryimport(), report the
reason the pool was suspended in a new key,
ZPOOL_CONFIG_SUSPENDED_REASON.
In libzfs, when interpreting the returned config nvlist, report
suspension due to MMP with a new pool status enum value,
ZPOOL_STATUS_IO_FAILURE_MMP.
In status_callback(), which generates and emits the message when 'zpool
status' is executed, add a case to print an appropriate message for the
new pool status enum value.
Motivation and Context
Before this change, the 'zpool status' output did not indicate MMP might be the reason the pool was suspended, and instructed the user to run 'zpool clear' which is confusing (there may be no device FAULTED) and incorrect (only a reboot clears the condition at this time). The latter is a bug which should be fixed, but the instructions to the user should be correct in the meantime.
Similarly, the libzfs code for parsing the config nvlist and producing an enum value reflecting the pool status did not reflect an MMP failure when that had occurred.
How Has This Been Tested?
I ran zloop and the mmp portion of the ZTS locally.
Types of changes
Checklist:
Signed-off-by.