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.

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

special device removal space accounting fixes #11329

Merged
merged 1 commit into from
Dec 17, 2020

Conversation

ahrens
Copy link
Member

@ahrens ahrens commented Dec 11, 2020

Motivation and Context

The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs available property). Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es). And therefore, there is
always enough free space to remove a special device.

Description

However, the checks for free space when removing special devices did not
take this into account. This commit corrects that.

Additionally, when removing the 2nd-to-last special device, its space
would not be reallocated to the last remaining special device, because
mc_groups has already been decremented. That is also fixed.

Closes #9142

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

Checklist:

@ahrens ahrens requested review from jwk404, amotin, a user and don-brady December 11, 2020 23:39
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Dec 13, 2020
Copy link
Contributor

@don-brady don-brady left a comment

Choose a reason for hiding this comment

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

LGTM

module/zfs/vdev_removal.c Outdated Show resolved Hide resolved
@behlendorf
Copy link
Contributor

behlendorf commented Dec 14, 2020

There was an unexpected ZTS failure of the alloc_class_012_pos test case on FreeBSD 12 and 13. This may be unrelated, but it is suspicious.

Tests with results other than PASS that are unexpected:
    FAIL alloc_class/alloc_class_012_pos (expected PASS)

Test: /usr/local/share/zfs/zfs-tests/tests/functional/alloc_class/alloc_class_012_pos (run as root) [00:06] [FAIL]
23:48:57.80 ASSERTION: Removing a special device from a pool succeeds.
23:48:57.82 SUCCESS: disk_setup
23:48:57.98 SUCCESS: zpool create testpool /mnt/device-0 /mnt/device-1 /mnt/device-2 special /mnt/device-3 special /mnt/device-4
23:49:00.07 SUCCESS: display_status testpool
23:49:00.17 SUCCESS: zfs create -o special_small_blocks=32K -o recordsize=32K testpool/testfs
23:49:00.40 SUCCESS: dd if=/dev/urandom of=/testpool/testfs/testfile.1 bs=1M count=25
23:49:00.84 SUCCESS: dd if=/dev/urandom of=/testpool/testfs/testfile.2 bs=1M count=50
23:49:02.18 SUCCESS: dd if=/dev/urandom of=/testpool/testfs/testfile.3 bs=1M count=75
23:49:03.77 SUCCESS: dd if=/dev/urandom of=/testpool/testfs/testfile.4 bs=1M count=100
23:49:04.01 SUCCESS: zpool sync testpool
23:49:04.02 SUCCESS: sync_pool testpool
23:49:04.02 NAME              SIZE  ALLOC   FREE  CKPOINT  EXPANDSZ   FRAG    CAP  DEDUP    HEALTH  ALTROOT
23:49:04.02 testpool         3.75G   269M  3.49G        -         -     0%     7%  1.00x    ONLINE  -
23:49:04.02   /mnt/device-0   960M     6M   954M        -         -     0%  0.62%      -  ONLINE  
23:49:04.02   /mnt/device-1   960M     6M   954M        -         -     0%  0.62%      -  ONLINE  
23:49:04.02   /mnt/device-2   960M  6.12M   954M        -         -     0%  0.63%      -  ONLINE  
23:49:04.02 special              -      -      -        -         -      -      -      -  -
23:49:04.02   /mnt/device-3   480M   121M   359M        -         -     2%  25.3%      -  ONLINE  
23:49:04.02   /mnt/device-4   480M   129M   351M        -         -     2%  26.9%      -  ONLINE  
23:49:04.03 SUCCESS: zpool list -v testpool
23:49:04.04 awk: can't open file { # find DVAs from string "offset level dva" only for L0 (data) blocks if (match($0,"L0 [0-9]+")) { dvas[0]=$3 dvas[1]=$4 dvas[2]=$5 for (i = 0; i < 3; ++i) { if (match(dvas[i],"([^:]+):.*")) { dva = substr(dvas[i], RSTART, RLENGTH); # parse DVA from string "vdev:offset:asize" if (split(dva,arr,":") != 3) { print "Error parsing DVA: <" dva ">"; exit 1; } # verify vdev is "special" if (arr[1] < d) { exit 1; } } } }} source line number 1
23:49:04.04 ERROR: file_in_special_vdev testpool/testfs 2 exited 2

The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs `available` property).  Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es).  And therfore, there is
always enough free space to remove a special device.

However, the checks for free space when removing special devices did not
take this into account.  This commit corrects that.

Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
@ahrens ahrens force-pushed the openzfs/remove_special branch from e995d14 to 24d372c Compare December 14, 2020 18:59
@ahrens
Copy link
Member Author

ahrens commented Dec 15, 2020

The FreeBSD failure is:

20:47:33.90 /usr/local/share/zfs/zfs-tests/tests/functional/cli_root/zpool_initialize/zpool_initialize_verify_initialized.ksh: line 79:  / 512: arithmetic syntax error

It looks like either $offset or $size is the empty string? Perhaps because zdb isn't outputting what it expects? I don't think this is related to my changes.

@behlendorf
Copy link
Contributor

behlendorf commented Dec 15, 2020

Yes, it's unrelated and has recently started failing consistently. There is draft PR #11328 open which is investigating the issue.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 16, 2020
@behlendorf behlendorf merged commit 71e4ce0 into openzfs:master Dec 17, 2020
@ahrens ahrens deleted the openzfs/remove_special branch December 17, 2020 21:36
ghost pushed a commit to zfsonfreebsd/ZoF that referenced this pull request Dec 23, 2020
The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs `available` property).  Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es).  And therefore, there is
always enough free space to remove a special device.

However, the checks for free space when removing special devices did not
take this into account.  This commit corrects that.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Don Brady <don.brady@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#11329
behlendorf pushed a commit that referenced this pull request Dec 23, 2020
The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs `available` property).  Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es).  And therefore, there is
always enough free space to remove a special device.

However, the checks for free space when removing special devices did not
take this into account.  This commit corrects that.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Don Brady <don.brady@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes #11329
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs `available` property).  Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es).  And therefore, there is
always enough free space to remove a special device.

However, the checks for free space when removing special devices did not
take this into account.  This commit corrects that.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Don Brady <don.brady@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#11329
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
The space in special devices is not included in spa_dspace (or
dsl_pool_adjustedsize(), or the zfs `available` property).  Therefore
there is always at least as much free space in the normal class, as
there is allocated in the special class(es).  And therefore, there is
always enough free space to remove a special device.

However, the checks for free space when removing special devices did not
take this into account.  This commit corrects that.

Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Reviewed-by: Don Brady <don.brady@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
Closes openzfs#11329
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Space checking of special device removal is too strict
3 participants