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

Don't allow attach to a raidz child vdev #15564

Merged
merged 1 commit into from
Nov 27, 2023
Merged

Don't allow attach to a raidz child vdev #15564

merged 1 commit into from
Nov 27, 2023

Conversation

don-brady
Copy link
Contributor

Motivation and Context

An automated test in FreeBSD encountered the case where an attach is attempted to a raidz child (#15536). This is not allowed and should fail but it now succeeds.

Regression from #15022.

Description

One of the checks in spa_vdev_attach() was failing to detect this case. The parent vdev ops can be raidz in two different attach cases:

Case 1 (attaching to raidz)

oldvd = pvd -> (raidz)
                / | \
               /  |  \
              /   |   \
            (c1) (c2) (c3)

Case 2 (attaching to a child of raidz)

     pvd -> (raidz)
             / | \
            /  |  \
           /   |   \
oldvd -> (c1) (c2) (c3)

Changed the check to only accept case 1

How Has This Been Tested?

ZTS functional/raidz
Ran reproducible test case from #15536 and it no longer happens

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:

ryao
ryao reviewed Nov 25, 2023
View reviewed changes
module/zfs/spa.c
* vdev.
* For attach, the only allowable parent is a mirror or
* the root vdev. A raidz vdev can be attached to, but
* you cannot attach to a raidz child.
*/
if (pvd->vdev_ops != &vdev_mirror_ops &&
Copy link
Contributor

@ryao ryao Nov 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It might be helpful to clarify in the comment that the condition is a list of permitted cases under de morgan's law. If it were me, I would append the following:

As per De Morgan's law: !(A OR B OR C) === !A AND !B AND !C

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Nov 27, 2023
@behlendorf behlendorf merged commit 7bbd42e into openzfs:master Nov 27, 2023
23 of 26 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@lundman
Don't allow attach to a raidz child vdev
20c0712 
Reviewed-by: Richard Yao <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Don Brady <[email protected]>
Closes openzfs#15536
Closes openzfs#15564
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryao ryao ryao approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

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