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

Sync AUX label on import #15817

Merged
merged 1 commit into from
Aug 8, 2024
Merged

Sync AUX label on import #15817

merged 1 commit into from
Aug 8, 2024

Conversation

ixhamza
Copy link
Contributor

@ixhamza ixhamza commented Jan 24, 2024

Spare and l2cache vdev labels are not updated during import. Therefore, if disk paths are updated between pool export and import, the AUX label still shows the old paths. This patch syncs the AUX label during import to show the correct path information.

How Has This Been Tested?

Made changes to disk paths between pool export and import, and verified that the latest AUX labels are correctly shown by zdb:

# Added spare with devid: driveOld
root@ub22:~# zpool create tank mirror sdb sdc spare sdd -f

root@ub22:~# zdb -l /dev/sdd1
------------------------------------
LABEL 0 
------------------------------------
    version: 5000
    state: 3
    guid: 4218101932799396375
    path: '/dev/sdd1'
    devid: 'scsi-0QEMU_QEMU_HARDDISK_driveOld-part1'
    phys_path: 'pci-0000:00:05.0-scsi-0:0:0:0'
    labels = 0 1 2 3 

root@ub22:~# zpool export tank

# Change devid to driveNew
root@ub22:~# zpool import tank

root@ub22:~# zdb -l /dev/sdd1
------------------------------------
LABEL 0 
------------------------------------
    version: 5000
    state: 3
    guid: 4218101932799396375
    path: '/dev/sdd1'
    devid: 'scsi-0QEMU_QEMU_HARDDISK_driveNew-part1'
    phys_path: 'pci-0000:00:05.0-scsi-0:0:0:0'
    labels = 0 1 2 3

# Without this patch, zdb does not show updated devid

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:

@ixhamza
Copy link
Contributor Author

ixhamza commented Jan 24, 2024

zfs-linux / functional (part2) appears to be failing on CI. Will check what is the issue.

@ixhamza ixhamza marked this pull request as draft January 24, 2024 11:21
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jan 26, 2024
@ixhamza ixhamza force-pushed the NAS-124699-3 branch 3 times, most recently from 8b0e4a2 to 0ecb114 Compare February 21, 2024 12:03
@ixhamza ixhamza marked this pull request as ready for review February 21, 2024 13:53
@ixhamza ixhamza marked this pull request as draft February 21, 2024 16:23
@ixhamza ixhamza marked this pull request as ready for review February 21, 2024 21:03
@ixhamza
Copy link
Contributor Author

ixhamza commented Feb 21, 2024

The updated commit resolves the CI test failures that were occurring due to this patch. Additionally, it has been rebased with the master. Some 20.04 tests are failing, but they are unrelated to this patch. For example, similar failures can be seen on other PR as well: zfs-2.2.3 patchset Actions. These failures on the recent PRs appear to be persistent whenever the 20.04 tests are run on the 20240218.1.0 image runner.

module/zfs/vdev_label.c Outdated Show resolved Hide resolved
@ixhamza
Copy link
Contributor Author

ixhamza commented Jul 25, 2024

@tonyhutter - It would be nice if you could take a look at this PR. I just rebased it with the latest master today.

@tonyhutter
Copy link
Contributor

Looks like everything is passing except Centos 8:

Tests with results other than PASS that are unexpected:
    FAIL cli_root/zpool_trim/zpool_trim_fault_export_import_online (expected PASS)
    FAIL fault/auto_online_002_pos (expected PASS)
    FAIL fault/auto_spare_shared (expected PASS)
    FAIL mmp/mmp_exported_import (expected PASS)
    FAIL mmp/mmp_inactive_import (expected PASS)

I'm going to manually re-run that test to make sure it's not a red herring.

Spare and l2cache vdev labels are not updated during import. Therefore,
if disk paths are updated between pool export and import, the AUX label
still shows the old paths. This patch syncs the AUX label
during import to show the correct path information.

Signed-off-by: Ameer Hamza <[email protected]>
@ixhamza
Copy link
Contributor Author

ixhamza commented Aug 6, 2024

Rebased with the master branch for CI rerun.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Looks good. I'll get this merged once the CI finishes with it. Thanks for updating this and sorry about the delay.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 6, 2024
@behlendorf behlendorf merged commit 5536c0d into openzfs:master Aug 8, 2024
23 of 24 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Sep 4, 2024
Spare and l2cache vdev labels are not updated during import. Therefore,
if disk paths are updated between pool export and import, the AUX label
still shows the old paths. This patch syncs the AUX label
during import to show the correct path information.

Reviewed-by: Tony Hutter <[email protected]>
Reviewed-by: Umer Saleem <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#15817
@amotin amotin deleted the NAS-124699-3 branch October 14, 2024 18:46
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.

5 participants