create zap for root vdev #14405

rob-wing · 2023-01-18T22:38:01Z

Create a ZAP for the root vdev and the functionality to set/get properties on it.

To set a user property on the root vdev:

zpool set user:prop=666 <pool> root-0

The functionality already exists to set a property on a top-level vdev by specifying <type>-<id> (e.g., mirror-0). The changes were minimal to enable this pattern for the root vdev since the root vdev is of type 'root' with an id of '0'.

Motivation and Context

These changes don't enable vdev property inheritance, however, this is a step towards achieving that.

Description

Create a ZAP for the root vdev and store the ZAP in spa_t as spa_root_vdev_zap

Modify libzutil's for_each_vdev_cb() to iterate over the root vdev. Previously, the root vdev was explicitly being filtered out, I looked around a bit to see why, I didn't find a solid answer - perhaps someone may know.

How Has This Been Tested?

manual testing

test0:
- import pool
- set user property on the root vdev
- export pool
- import pool and verify property is set

test1:
- use pool from test0
- export the pool
- unload openzfs kernel module with root vdev patch
- load openzfs kernel module that doesnt have root vdev zap patch
- verify the pool still imports
- export the pool

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)
Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the OpenZFS 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

rob-wing · 2023-01-19T03:06:38Z

It's not super obvious what's failing in the test but, I'm guessing it has something to do with the zap being leaked:

01:47:25.83 MOS object 65 (zap) leaked
01:47:25.83 Verified large_blocks feature refcount of 0 is correct
01:47:25.83 Verified large_dnode feature refcount of 0 is correct
01:47:25.83 Verified sha512 feature refcount of 0 is correct
01:47:25.83 Verified skein feature refcount of 0 is correct
01:47:25.83 Verified edonr feature refcount of 0 is correct
01:47:25.83 Verified userobj_accounting feature refcount of 1 is correct
01:47:25.83 Verified encryption feature refcount of 0 is correct
01:47:25.83 Verified project_quota feature refcount of 1 is correct
01:47:25.83 Verified redaction_bookmarks feature refcount of 0 is correct
01:47:25.83 Verified redacted_datasets feature refcount of 0 is correct
01:47:25.83 Verified bookmark_written feature refcount of 0 is correct
01:47:25.83 Verified livelist feature refcount of 0 is correct
01:47:25.83 Verified zstd_compress feature refcount of 0 is correct
01:47:25.83 Verified zilsaxattr feature refcount of 0 is correct
01:47:25.83 Verified blake3 feature refcount of 0 is correct

I'll dig into it and see what I can figure out.

behlendorf

Functionally this looks good, but let's add some test coverage to the cli_root/zfs_set and cli_root/zfs_get test groups. You could extend the existing test cases or add a new one.

module/zfs/vdev.c

behlendorf · 2023-02-09T20:15:50Z

module/zfs/vdev.c

+		(void) nvlist_lookup_uint64(nv,
+		    ZPOOL_CONFIG_VDEV_ROOT_ZAP, &spa->spa_root_vdev_zap);
+	} else {
+		ASSERT0(spa->spa_root_vdev_zap);


What's the rational for storing this in the spa rather than the root vdev? Modeling this after how the top zaps are stored seems preferable; i.e. vd->vdev_root_zap. This is why we're running afoul of that new ASSERT0 since we're checking spa->spa_root_vdev_zap is zero for all vdev types.

http://build.zfsonlinux.org/builders/CentOS%207%20x86_64%20%28TEST%29/builds/23347/steps/shell_4/logs/console

The rationale was that it was less wasteful (in terms of space) to store the root vdev zap in spa_t as opposed to vdev_t since vdev_root_zap will be zero except for all vdevs except the root vdev.

With that said, my first approach did store the root vdev zap in vd->vdev_root_zap and tracked it in the AVZ. I think this would require a feature flag for the root vdev zap otherwise vdev_count_verify_zaps() will panic when importing a pool using an older module.

I agree with you that the root vdev zap fits better into the system by following the same pattern as top zaps.

bump

Should I update this PR to have the root vdev zap follow the same pattern as top/leaf zaps then?

The one drawback here is that it would require adding a feature flag for adding the root vdev zap to the AVZ and not being backwards compatible with pools that don't have this feature enabled.

As I said earlier, I agree that the approach above fits into the current design more naturally - but doesn't allow for backwards compatibility with older pools.

Just want to double check with the direction to go before updating this pull request.

Well that's unfortunate. I see why you opted for this approach now. In an ideal world I'd prefer this code be aligned with the approach taken for the top/leaf zaps. However, we need retain backwards compatibility and if we can do so without introducing a feature flag so much the better.

Given that let's stick with your current approach. If you can update the PR again to resolve the ASSERTs I think we can move forward with this. Thanks for the reminder to look at this again.

ryao · 2023-03-06T19:02:25Z

module/zfs/vdev.c

+		(void) nvlist_lookup_uint64(nv,
+		    ZPOOL_CONFIG_VDEV_ROOT_ZAP, &spa->spa_root_vdev_zap);
+	} else {
+		ASSERT0(spa->spa_root_vdev_zap);


This assertion tripped in zloop:

Executing /sbin/zdb -bccsv -G -d -Y -e -y -p /var/tmp/zloop-run ztest spa->spa_root_vdev_zap == 0 (0x42 == 0) ASSERT at module/zfs/vdev.c:908:vdev_alloc()Aborted (core dumped) ztest: '/sbin/zdb -bccsv -G -d -Y -e -y -p /var/tmp/zloop-run ztest' exit code 134

Thanks, I'll update this PR when I get a moment to look at this.

@rob-wing Just in case you were unaware (since it is a pain to find them), there are artifacts that can be downloaded from here that might or might not be useful in debugging:

https://github.com/openzfs/zfs/actions/runs/4117171010

We'll want to drop this ASSERT based on the conversation above, #14405 (comment)

rob-wing · 2023-03-13T19:55:49Z

There's one thing that concerns me that the root vdev ZAP is not being handled correctly.

I've observed that the root vdev ZAP is still leaked when running the vdev_zaps_007 test...at the moment this test is disabled because the spacelog map is also leaked causing the test to fail - for what its worth, I understand why the spacelog map is being leaked. The vdev_zaps_007 tests pool splitting and the transferring of ZAP's.

I mention this to see if someone could give the ZAP handling extra scrutiny to see if I've missed a basic understanding of how ZAP's are supposed to be created/handled.

If anyone has any thoughts...let me know if this is a valid concern or not - thanks.

rob-wing · 2023-03-31T00:07:18Z

I ended up adding the root vdev ZAP to the AVZ and creating a feature flag, com.klarasystems@avz_v2.

I think this approach is cleaner and aligns with the reason why the AVZ was introduced in the first place. I'll note that this change would have been backwards compatible for !DEBUG builds.

I've added tests for get/set a property on the root vdev and to verify the ZAP is created for the root vdev.

And add it to the AVZ, this is not backwards compatible with older pools due to an assertion in spa_sync() that verifies the number of ZAPs of all vdevs matches the number of ZAPs in the AVZ. Granted, the assertion only applies to #DEBUG builds - still, a feature flag is introduced to avoid the assertion, com.klarasystems:vdev_zaps_v2 Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

Notably, this allows to get/set properties on the root vdev: % zpool set user:prop=value <pool> root-0 Before this commit, it was already possible to get/set properties on top-level vdevs with the syntax <type>-<vdev_id> (e.g. mirror-0): % zpool set user:prop=value <pool> mirror-0 This syntax also applies to the root vdev as it is is of type 'root' with a vdev_id of 0, root-0. Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

behlendorf

You'll want to update the ABI files with these to sort out the checkstyle failure

https://github.com/openzfs/zfs/actions/runs/4570091230?pr=14405

tests/runfiles/common.run

module/zcommon/zfeature_common.c

tests/zfs-tests/tests/functional/cli_root/zpool_get/vdev/vdev_get_001_pos.ksh

behlendorf · 2023-03-31T16:31:08Z

@don-brady would you mind having a look at this!

Allow retrieving and setting properties on the root vdev using the keyword 'root' as an alias for 'root-0'. Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

The following tests have been added: - zpool get all properties from root vdev - zpool set a property on root vdev - verify root vdev ZAP is created Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

allanjude · 2023-04-13T15:55:27Z

Is there anything outstanding on this PR?

behlendorf

This looks good to me. Thanks for iterating on this.

And add it to the AVZ, this is not backwards compatible with older pools due to an assertion in spa_sync() that verifies the number of ZAPs of all vdevs matches the number of ZAPs in the AVZ. Granted, the assertion only applies to #DEBUG builds - still, a feature flag is introduced to avoid the assertion, com.klarasystems:vdev_zaps_v2 Notably, this allows to get/set properties on the root vdev: % zpool set user:prop=value <pool> root-0 Before this commit, it was already possible to get/set properties on top-level vdevs with the syntax <type>-<vdev_id> (e.g. mirror-0): % zpool set user:prop=value <pool> mirror-0 This syntax also applies to the root vdev as it is is of type 'root' with a vdev_id of 0, root-0. The keyword 'root' as an alias for 'root-0'. The following tests have been added: - zpool get all properties from root vdev - zpool set a property on root vdev - verify root vdev ZAP is created Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc. Closes openzfs#14405

rob-wing force-pushed the root-vdev-zap branch from 73d010c to 66977c9 Compare January 18, 2023 22:47

behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 19, 2023

rob-wing force-pushed the root-vdev-zap branch from 66977c9 to e7a07bb Compare January 30, 2023 21:58

behlendorf reviewed Jan 31, 2023

View reviewed changes

module/zfs/vdev.c Show resolved Hide resolved

behlendorf requested a review from don-brady January 31, 2023 19:31

behlendorf reviewed Feb 9, 2023

View reviewed changes

ryao reviewed Mar 6, 2023

View reviewed changes

rob-wing force-pushed the root-vdev-zap branch from 7084a64 to fa24e65 Compare March 13, 2023 19:17

rob-wing force-pushed the root-vdev-zap branch from fa24e65 to 3450a61 Compare March 30, 2023 23:56

Rob Wing added 3 commits March 31, 2023 10:42

libzutil: allow vdev callback functions to run on the root vdev

fe85831

Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

behlendorf reviewed Mar 31, 2023

View reviewed changes

behlendorf requested a review from tonyhutter March 31, 2023 16:31

Rob Wing added 3 commits March 31, 2023 16:48

zpool_main: implement root alias for the root vdev

60754d7

Allow retrieving and setting properties on the root vdev using the keyword 'root' as an alias for 'root-0'. Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

zpool-features(7): document com.klarasystems@vdev_zaps_v2

5a94a39

Signed-off-by: Rob Wing <rob.wing@klarasystems.com> Sponsored-by: Seagate Technology Submitted-by: Klara, Inc.

rob-wing force-pushed the root-vdev-zap branch 2 times, most recently from 9fa70ae to 5a94a39 Compare April 1, 2023 01:51

behlendorf approved these changes Apr 13, 2023

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Apr 13, 2023

behlendorf merged commit 3e4ed42 into openzfs:master Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create zap for root vdev #14405

create zap for root vdev #14405

rob-wing commented Jan 18, 2023

rob-wing commented Jan 19, 2023

behlendorf left a comment •

edited

Loading

behlendorf Feb 9, 2023

rob-wing Feb 13, 2023

rob-wing Feb 28, 2023

behlendorf Mar 1, 2023

ryao Mar 6, 2023 •

edited

Loading

rob-wing Mar 6, 2023

ryao Mar 6, 2023

behlendorf Mar 9, 2023

rob-wing commented Mar 13, 2023

rob-wing commented Mar 31, 2023

behlendorf left a comment

behlendorf commented Mar 31, 2023

allanjude commented Apr 13, 2023

behlendorf left a comment

create zap for root vdev #14405

create zap for root vdev #14405

Conversation

rob-wing commented Jan 18, 2023

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

rob-wing commented Jan 19, 2023

behlendorf left a comment • edited Loading

Choose a reason for hiding this comment

behlendorf Feb 9, 2023

Choose a reason for hiding this comment

rob-wing Feb 13, 2023

Choose a reason for hiding this comment

rob-wing Feb 28, 2023

Choose a reason for hiding this comment

behlendorf Mar 1, 2023

Choose a reason for hiding this comment

ryao Mar 6, 2023 • edited Loading

Choose a reason for hiding this comment

rob-wing Mar 6, 2023

Choose a reason for hiding this comment

ryao Mar 6, 2023

Choose a reason for hiding this comment

behlendorf Mar 9, 2023

Choose a reason for hiding this comment

rob-wing commented Mar 13, 2023

rob-wing commented Mar 31, 2023

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Mar 31, 2023

allanjude commented Apr 13, 2023

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf left a comment •

edited

Loading

ryao Mar 6, 2023 •

edited

Loading