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

zpool import -m also removing spare and cache when log device is missing #14794

Merged
merged 2 commits into from
May 3, 2023

Conversation

ixhamza
Copy link
Contributor

@ixhamza ixhamza commented Apr 25, 2023

spa_import() relies on a pool config fetched by spa_try_import() for spare/cache devices. Import flags are not passed to spa_tryimport(), which makes it return early due to a missing log device and missing retrieving the cache device and spare eventually. Passing ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct configuration regardless of the missing log device.

How Has This Been Tested?

  • Create a zfs pool: zpool create -f tank nvme0n1 cache nvme1n1 spare nvme2n1 log nvme3n1
  • Check pool status:
$ zpool status tank
	NAME        STATE     READ WRITE CKSUM
	tank        ONLINE       0     0     0
	  nvme0n1   ONLINE       0     0     0
	logs	
	  nvme3n1   ONLINE       0     0     0
	cache
	  nvme1n1   ONLINE       0     0     0
	spares
	  nvme2n1   AVAIL    
  • Export the pool: zpool export tank
  • Pull nvme3n1 drive, i.e., log device.
  • Import the pool: zpool import -m tank
  • Check the pool status again:
$ zpool status tank
	NAME                   STATE     READ WRITE CKSUM
	tank                   DEGRADED     0     0     0
	  nvme0n1              ONLINE       0     0     0
	logs	
	  1797033476626228610  UNAVAIL      0     0     0  was /dev/nvme3n1p1
  • Expected pool status:
$ zpool status tank
	NAME                    STATE     READ WRITE CKSUM
	tank                    DEGRADED     0     0     0
	  nvme0n1               ONLINE       0     0     0
	logs	
	  15191547688177894754  UNAVAIL      0     0     0  was /dev/nvme3n1p1
	cache
	  nvme1n1               ONLINE       0     0     0
	spares
	  nvme2n1               AVAIL   

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 Apr 25, 2023

cc: @amotin

@ixhamza
Copy link
Contributor Author

ixhamza commented Apr 25, 2023

After discussing with @amotin internally, passing ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct configuration without any obvious side effects. It also shows the correct spare/cache configuration through the zpool import command. Updated the PR.

@behlendorf
Copy link
Contributor

behlendorf commented Apr 26, 2023

@ixhamza the fix here seems reasonable. How about also adding your test case from the top comment to the test suite as part of this PR.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Apr 26, 2023
@ixhamza
Copy link
Contributor Author

ixhamza commented Apr 26, 2023

@behlendorf sure

@ixhamza
Copy link
Contributor Author

ixhamza commented May 3, 2023

@behlendorf Added a test case.

@behlendorf
Copy link
Contributor

@ixhamza great! It looks like the zloop failures were caused by a known leak which was resolved. Please just rebase this on master so we can get a clean run then this is good to go.

spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Signed-off-by: Ameer Hamza <[email protected]>
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels May 3, 2023
@behlendorf behlendorf merged commit 82ac409 into openzfs:master May 3, 2023
ixhamza added a commit to truenas/zfs that referenced this pull request May 4, 2023
spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#14794
ixhamza added a commit to truenas/zfs that referenced this pull request May 4, 2023
spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#14794
ixhamza added a commit to truenas/zfs that referenced this pull request May 4, 2023
spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#14794
behlendorf pushed a commit that referenced this pull request May 5, 2023
spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes #14794
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request May 15, 2023
spa_import() relies on a pool config fetched by spa_try_import() for
spare/cache devices. Import flags are not passed to spa_tryimport(),
which makes it return early due to a missing log device and missing
retrieving the cache device and spare eventually. Passing
ZFS_IMPORT_MISSING_LOG to spa_tryimport() makes it fetch the correct
configuration regardless of the missing log device.

Reviewed-by: Alexander Motin <[email protected]>
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#14794
@ixhamza ixhamza deleted the NAS-117393 branch August 2, 2023 17:19
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.

4 participants