Panic when running 'zpool split'

[rstrlcpy]

Motivation and Context

#5565

Description

From an available crash-dump I see that the freed VDEV is accessed, because it is on txg's DTL of synced VDEV, because was transferred by vdev_top_transfer(), that was called by vdev_remove_parent(), that was called by vdev_split().

"detach" does the cleanup, but it seems for "split" this cleanup was not implemented.

How Has This Been Tested?

That is difficult to reproduce the issue, but we have a host, where it can be reproduced very simple:

zpool attach >>> wait for resilvering >>> zpool split

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)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the ZFS on Linux code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• All new and existing tests passed.
• All commit messages are properly formatted and contain Signed-off-by.
• Change has been approved by a ZFS on Linux member.

[pzakha]

I'm not fully clear on what happens if some of the split vdevs have DTLs. Would that mean that some top-level vdevs in the split pool will end-up lacking data for some txgs? Shouldn't we instead check that there are no DTLs and fail if there are any?

[pzakha]

We should put this on hold until we investigate a fix for: #7865

[behlendorf]

@ramzec shall we close this out given the 733b572 fix was merged.

[rstrlcpy]

I need to double-check. 99% this is another issue.

[lundman]

This fixes the identical problem we are having.

[ikozhukhov]

what is status of this update?

[behlendorf]

Ready to be revisited I believe, and in need of reviewers. @ramzec when you get a moment would to mind rebasing this on master.

[behlendorf]

Shouldn't we instead check that there are no DTLs and fail if there are any?

@pzakha good question. By my reading of spa_vdev_split_mirror() we are already doing exactly this. When looping over the vdevs to validate them zpool split will fail if vdev_dtl_required() determines that removing any of the vdevs would result in a DTL outage.

https://github.com/zfsonlinux/zfs/blob/master/module/zfs/spa.c#L6626L6630

Since the split was determined to be safe it does look like the right thing to do is remove the vdev from the other txg's DTL lists. This is the same procedure employed in spa_vdev_detach.

https://github.com/zfsonlinux/zfs/blob/master/module/zfs/spa.c#L6336L6345

Additionally, we should probably be setting vd->vdev_detached and dirtying the DTL so vdev_dtl_sync() will free the vdev's DTL object.

        vd->vdev_detached = B_TRUE;
        vdev_dirty(tvd, VDD_DTL, vd, txg);

[pzakha]

The original issue that I pointed out was addressed in #7881. This does look like a separate problem, and the fix looks reasonable.

[codecov]

Codecov Report

Merging #7856 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #7856      +/-   ##
==========================================
+ Coverage   78.52%   78.54%   +0.02%     
==========================================
  Files         380      380              
  Lines      116324   116327       +3     
==========================================
+ Hits        91344    91371      +27     
+ Misses      24980    24956      -24

Flag	Coverage Δ
#kernel	79.11% <100%> (+0.06%)	⬆️
#user	67.03% <0%> (+0.12%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 304d469...d453d7d. Read the comment docs.