Skip to content
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.

Sign up for GitHub

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

Jump to bottom

OpenZFS 6736 - ZFS per-vdev ZAPs #4515

Closed
wants to merge 2 commits into from
Closed

OpenZFS 6736 - ZFS per-vdev ZAPs #4515

wants to merge 2 commits into from

Conversation

don-brady
Copy link
Contributor

6736 ZFS per-vdev ZAPs
Reviewed by: Matthew Ahrens [email protected]
Reviewed by: John Kennedy [email protected]
Reviewed by: George Wilson [email protected]
Reviewed by: Don Brady [email protected]
Reviewed by: Dan McDonald [email protected]

References:
https://www.illumos.org/issues/6736
openzfs/openzfs#72

Ported-by: Don Brady [email protected]

@don-brady
Copy link
Contributor Author

A few of the test bots are missing zfs.sh in path scope:

+ sudo -E zfs.sh
sudo: zfs.sh: command not found

@behlendorf
Copy link
Contributor

That's a little strange. Nothing has changed on the buildbot which would cause that. Let me rerun those builds.

Don Brady added 2 commits April 29, 2016 03:15
OpenZFS 6736 - ZFS per-vdev ZAPs
e2b645b 
6736 ZFS per-vdev ZAPs
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: John Kennedy <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Don Brady <[email protected]>
Reviewed by: Dan McDonald <[email protected]>

References:
 https://www.illumos.org/issues/6736
 openzfs/openzfs@215198a

Ported-by: Don Brady <[email protected]>
fix ksh path in cleanup & setup test scripts
afd818c
@don-brady
Copy link
Contributor Author

rebasing to master and hoping missing zfs.sh issue was resolved

@behlendorf
Copy link
Contributor

@don-brady the missing zfs.sh issue is subtle but caused by this. The zfs-test package failed to install because rpm automatically added a dependency on /usr/bin/ksh, but that's not the common location for ksh on some Linux platforms. You'll need to change the new test cases to use #!/bin/ksh -p. Arguably the install step should have failed outright to make spotting the issue easier.

@don-brady
Copy link
Contributor Author

ah, thanks for finding this! Will fix.

@behlendorf
Copy link
Contributor

Great, and I've updated the install step so that if any package fails to install it's treated as a failure so this will be more obvious in the future.

@don-brady
Copy link
Contributor Author

almost made it. Debian test bot died in a ztest while joining the last thread (of 526 threads) so it was close to being done. Not sure what would cause the thread tear-down to die.

@behlendorf
Copy link
Contributor

@don-brady let me send it around again for Debian 8 and we'll see if it happens again.

behlendorf
behlendorf reviewed May 2, 2016
View reviewed changes
module/zfs/spa_config.c
VERIFY(nvlist_size(nvl, &buflen, NV_ENCODE_XDR) == 0);

buf = vmem_alloc(buflen, KM_SLEEP);
buf = fnvlist_pack(nvl, &buflen);
temp = kmem_zalloc(MAXPATHLEN, KM_SLEEP);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This minor restructuring is going to result in us using kmem_alloc() instead of vmem_alloc(). This poses a little bit of a problem since it's possible this allocation can be fairly large (40744 bytes as shown in #3251) which is why it was moved to vmem_alloc(). The SPL will generate console warnings for any allocations which exceed the default 32k threshold. We need to preserve the vmem_alloc() / vmem_free() here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, got it. I can revert it. Perhaps we could eventually make fnvlist_pack/fnvlist_pack_free use vmem allocations under the covers for linux kernel?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea... and now that you mention it it appears we've already done it in commit 77aef6f. So actually, this code is good and we don't need to change anything in this patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

indeed. since the fnvlist_alloc is using KM_SLEEP it should be good to go.

@behlendorf
Copy link
Contributor

@don-brady LGTM, only issue I spotted what that the kmem/vmem issue I mentioned needs to be addressed.

Also FYI if there were significant changes (more than just context / paths) required to apply a OpenZFS commit to ZoL we have been adding a 'Porting Notes' section to the commit comment. It should describe any modifications you needed to make to port the patch. I find it particularly helpful when doing patch review since those areas really need extra scrutiny. And it's helpful to have that documentation in the future when referring back to a change.

@behlendorf
Copy link
Contributor

@don-brady since the kmem/vmem issue I mentioned inline with the patch isn't really an issue this patch LGTM. If you're good with this as the final version of this patch I can merge it.

@behlendorf behlendorf added this to the 0.7.0 milestone May 2, 2016
@don-brady
Copy link
Contributor Author

I'm using this patch with the metadata isolation work to store the vdev class info and so far it works as expected.

@behlendorf
Copy link
Contributor

@don-brady awesome, I'll merge it shortly.

@behlendorf behlendorf closed this in e0ab3ab May 2, 2016
@behlendorf
Copy link
Contributor

Squashed and merged as:

e0ab3ab OpenZFS 6736 - ZFS per-vdev ZAPs

behlendorf added a commit to behlendorf/zfs that referenced this pull request May 6, 2016
@behlendorf
Wrap vdev_count_verify_zaps() with DEBUG
973739a 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with a
DEBUG for clarity and to prevent unused variable warnings during
a production build.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#4515
@behlendorf behlendorf mentioned this pull request May 6, 2016
Merged
behlendorf added a commit to behlendorf/zfs that referenced this pull request May 6, 2016
@behlendorf
Wrap vdev_count_verify_zaps() with DEBUG
7131ef4 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with a
DEBUG for clarity and to prevent unused variable warnings during
a production build.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#4515
behlendorf added a commit to behlendorf/zfs that referenced this pull request May 6, 2016
@behlendorf
Wrap vdev_count_verify_zaps() with ZFS_DEBUG
606346b 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with
ZFS_DEBUG for clarity and to prevent unused variable warnings in
a production build.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#4515
behlendorf added a commit to behlendorf/zfs that referenced this pull request May 6, 2016
@behlendorf
Per-vdev ZAP tests must use $ZPOOL and $ZDB
402a52a 
Commit e0ab3ab introduced new per-vdev ZAP tests which should have
used the $ZPOOL and $ZDB variabled.  The tests passed the automated
testing since both utilities but when running in-tree all of the new
tests fail.

Signed-off-by: Brian Behlendorf <[email protected]>
Issue openzfs#4515
@behlendorf behlendorf mentioned this pull request May 6, 2016
Merged
behlendorf added a commit that referenced this pull request May 7, 2016
@behlendorf
Per-vdev ZAP tests must use $ZPOOL and $ZDB
d516761 
Commit e0ab3ab introduced new per-vdev ZAP tests which should have
used the $ZPOOL and $ZDB variabled.  The tests passed the automated
testing since both utilities but when running in-tree all of the new
tests fail.

Signed-off-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #4515
behlendorf added a commit that referenced this pull request May 7, 2016
@behlendorf
Wrap vdev_count_verify_zaps() with ZFS_DEBUG
33cf67c 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with
ZFS_DEBUG for clarity and to prevent unused variable warnings in
a production build.

Signed-off-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes #4515
rtvd pushed a commit to rtvd/zfs that referenced this pull request May 7, 2016
@behlendorf @rtvd
Wrap vdev_count_verify_zaps() with ZFS_DEBUG
ad9ea9c 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with
ZFS_DEBUG for clarity and to prevent unused variable warnings in
a production build.

Signed-off-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#4515
ryao pushed a commit to ClusterHQ/zfs that referenced this pull request Jun 7, 2016
@ryao
OpenZFS 6736 - ZFS per-vdev ZAPs
1a55f16 
6736 ZFS per-vdev ZAPs
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: John Kennedy <[email protected]>
Reviewed by: George Wilson <[email protected]>
Reviewed by: Don Brady <[email protected]>
Reviewed by: Dan McDonald <[email protected]>

References:
  https://www.illumos.org/issues/6736
  openzfs/openzfs@215198a

Ported-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#4515
ryao pushed a commit to ClusterHQ/zfs that referenced this pull request Jun 7, 2016
@behlendorf @ryao
Per-vdev ZAP tests must use $ZPOOL and $ZDB
1d9263a 
Commit e0ab3ab introduced new per-vdev ZAP tests which should have
used the $ZPOOL and $ZDB variabled.  The tests passed the automated
testing since both utilities but when running in-tree all of the new
tests fail.

Signed-off-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#4515
ryao pushed a commit to ClusterHQ/zfs that referenced this pull request Jun 7, 2016
@behlendorf @ryao
Wrap vdev_count_verify_zaps() with ZFS_DEBUG
e43e79f 
Commit e0ab3ab introduced two blocks of code which are only needed
when debugging is enabled.  These blocks should be wrapped with
ZFS_DEBUG for clarity and to prevent unused variable warnings in
a production build.

Signed-off-by: Don Brady <[email protected]>
Signed-off-by: Brian Behlendorf <[email protected]>
Closes openzfs#4515
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.7.0
Development

Successfully merging this pull request may close these issues.
2 participants
@don-brady @behlendorf