-
Notifications
You must be signed in to change notification settings - Fork 1.9k
zpool: Add zpool status -vv error ranges #17864
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
base: master
Are you sure you want to change the base?
Conversation
|
Sample output: |
|
Just the filenames (JSON): |
45b19a7 to
eb6a882
Compare
eb6a882 to
6c63c6a
Compare
1c4134d to
b939af9
Compare
11fbd66 to
6a2cf00
Compare
f33ab4e to
49eb048
Compare
Break out the range_tree, btree, and highbit64 code from kernel space into shared kernel and userspace code. This is needed for the updated `zpool status -vv` error byte range reporting in the next commit. That commit needs the range_tree code in kernel and userspace. Signed-off-by: Tony Hutter <[email protected]>
Print the byte error ranges with 'zpool status -vv'. This works with the normal zpool status formatting flags (-p, -j, --json-int). In addition: - Move range_tree/btree to common userspace/kernel code. - Modify ZFS_IOC_OBJ_TO_STATS ioctl to optionally return "extended" object stats. - Let zinject corrupt zvol data. - Add test case. This commit takes code from these PRs: openzfs#17502 openzfs#9781 openzfs#8902 Signed-off-by: Tony Hutter <[email protected]> Co-authored-by:: Alan Somers <[email protected]> Co-authored-by: TulsiJain <[email protected]>
49eb048 to
0e11fb1
Compare
|
This is now passing all CI test. Reviewers - would you mind taking another look? |
behlendorf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only managed to make it through the first patch but I wanted to post a first round of feedback. I like the direction this is going!
| return (0); | ||
|
|
||
| return (8 * sizeof (uint64_t) - __builtin_clzll(i)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to split up highbit64 and lowbit64 so let's move them both here.
While you're at it we can also define them a little more concisely. Unfortunately, gcc documentation for __builtin_clzll states the result is undefined when the value is zero so we need to keep that check.
#define highbit64(x) (((x) == 0) ? 0 : CHAR_BIT * sizeof(x) - __builtin_clzll(x))
#define lowbit64(x) (__builtin_ffsll(x))This is a purely mechanical bit of cleanup so if you can move the highbit64/lowbit64 change in to its own PR we could get that chunk merged more quickly.
Let's also pull btree and range_tree refactoring in to its own PR. That way we can test it in isolation.
| @@ -0,0 +1,7 @@ | |||
| librange_tree_la_CFLAGS = $(AM_CFLAGS) $(KERNEL_CFLAGS) $(LIBRARY_CFLAGS) | |||
| # librange_tree_la_CFLAGS += -fvisibility=hidden | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't be commented out.
| noinst_LTLIBRARIES += librange_tree.la | ||
|
|
||
| nodist_librange_tree_la_SOURCES = module/zfs/range_tree.c | ||
| nodist_EXTRA_librange_tree_la_SOURCES = module/zfs/btree.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed?
|
|
||
| #include <stdint.h> | ||
|
|
||
| #include <sys/param.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you'll want <limits.h> for CHAR_BIT and the highbit64/lowbit64 changes.
| module/zcommon/zprop_common.c | ||
| module/zcommon/zprop_common.c \ | ||
| module/zfs/btree.c \ | ||
| module/zfs/range_tree.c |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't want to include these as sources. They should be added to the _LIBADD section as you did for libzpool.
|
|
||
| libzfs_la_LIBADD += -lrt -lm $(LIBCRYPTO_LIBS) $(ZLIB_LIBS) $(LIBFETCH_LIBS) $(LTLIBINTL) | ||
|
|
||
| # libzfs_la_LDFLAGS = -pthread -lrange_tree -lbtree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be dropped.
| include $(srcdir)/%D%/libbtree/Makefile.am | ||
| include $(srcdir)/%D%/libicp/Makefile.am | ||
| include $(srcdir)/%D%/libnvpair/Makefile.am | ||
| include $(srcdir)/%D%/librange_tree/Makefile.am |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also update the diagram above.
| #include <sys/nvpair.h> | ||
| #include "zfs_comutil.h" | ||
| #include <sys/zfs_ratelimit.h> | ||
| #include <sys/dmu.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this can be dropped.
| libzfs_core.la | ||
|
|
||
| libzfs_core.la \ | ||
| libbtree.la |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to drop libzpool.la and libzfs_core.la here. I'm pretty sure they were only being added to pull in the btree implementation. Plus dropping them will let us verify the new library can be used stand alone in user space.
| kmem_cache_t *zfs_btree_leaf_cache; | ||
| #ifndef _KERNEL | ||
| #include <stdio.h> | ||
| #include <stdlib.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you recall what you need to pull these two headers in for?
|
@behlendorf let me move over the btree/range_tee commit to another PR as you suggested, and I'll address your comments in there. |
Motivation and Context
Print error byte ranges with
zpool status -vvDescription
Print the byte error ranges with 'zpool status -vv'. This works with all the normal zpool status formatting flags:
-p,-j,--json-intIn addition:
This commit takes code from these PRs: #17502 #9781 #8902
How Has This Been Tested?
Test case added
Types of changes
Checklist:
Signed-off-by.