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

Fix Uberblock Label and Path Handling Issues related to AUX vdevs #15737

Closed
wants to merge 3 commits into from

Conversation

ixhamza
Copy link
Contributor

@ixhamza ixhamza commented Jan 4, 2024

This PR addresses three similar issues for AUX vdevs:

  • Ensure uberblock label always dumped: When spare or l2cache (aux) vdev is added during pool creation, spa->spa_uberblock is not dumped until that point. Subsequently, the aux label is never synchronized after its initial creation, resulting in the uberblock label remaining undumped. The uberblock is crucial for lib_blkid in identifying the ZFS partition type. To address this issue, we now ensure sync of the uberblock label once if it's not dumped initially.
  • Extend aux label to add path information: Pool import logic uses vdev paths, so it makes sense to add path information on AUX vdev as well.
  • Add path handling for aux vdevs in label_path: If the AUX vdev is added using UUID, importing the pool falls back AUX vdev to open it with disk name instead of UUID due to the absence of path information for AUX vdevs previously. Since AUX label now have path information, this PR adds path handling for it in label_path.

How Has This Been Tested?

  1. Uberblock label issue (Used by libblkid to identify zfs partition type)

Before this patch

# zpool create tank sdb spare sdc
# dd if=/dev/sdc1 bs=256K count=1 | hexdump -C -v | grep "0c b1 ba"
# udevadm info /dev/sdc1 | grep zfs_member

After this patch:

# zpool create tank sdb spare sdc
# dd if=/dev/sdc1 bs=256K count=1 | hexdump -C -v | grep "0c b1 ba"
00021000  0c b1 ba 00 00 00 00 00  88 13 00 00 00 00 00 00  |................|
# udevadm info /dev/sdc1 | grep zfs_member
E: ID_FS_TYPE=zfs_member
  1. Extended AUX label

Before the patch

# zdb -l /dev/sdc1
------------------------------------
LABEL 0 
------------------------------------
    version: 5000
    state: 3
    guid: 15986349143942038035
    labels = 0 1 2 3 

After the patch:

# zdb -l /dev/sdc1
------------------------------------
LABEL 0 
------------------------------------
    version: 5000
    state: 3
    guid: 15986349143942038035
    path: '/dev/sdc1'
    devid: 'scsi-0QEMU_QEMU_HARDDISK_drive4-part1'
    phys_path: 'pci-0000:00:05.0-scsi-0:0:1:1'
    labels = 0 1 2 3 
  1. Issue with AUX UUID during pool import

Before the patch:

# zpool create tank 59768b84-1aea-b649-82f8-e837a54be4ff
# zpool add tank spare 2e0105d0-f092-1f43-bb14-52d1e858116f
# zpool status
  pool: tank
 state: ONLINE
config:
	NAME                                    STATE     READ WRITE CKSUM
	tank                                    ONLINE       0     0     0
	  59768b84-1aea-b649-82f8-e837a54be4ff  ONLINE       0     0     0
	spares
	  2e0105d0-f092-1f43-bb14-52d1e858116f  AVAIL   
# zpool export tank
# zpool import tank
# zpool status
  pool: tank
 state: ONLINE
config:
	NAME                                    STATE     READ WRITE CKSUM
	tank                                    ONLINE       0     0     0
	  59768b84-1aea-b649-82f8-e837a54be4ff  ONLINE       0     0     0
	spares
	  sdc  AVAIL   

After the patch:

# zpool create tank 59768b84-1aea-b649-82f8-e837a54be4ff
# zpool add tank spare 2e0105d0-f092-1f43-bb14-52d1e858116f
# zpool status
  pool: tank
 state: ONLINE
config:
	NAME                                    STATE     READ WRITE CKSUM
	tank                                    ONLINE       0     0     0
	  59768b84-1aea-b649-82f8-e837a54be4ff  ONLINE       0     0     0
	spares
	  2e0105d0-f092-1f43-bb14-52d1e858116f  AVAIL   
# zpool export tank
# zpool import tank
# zpool status
  pool: tank
 state: ONLINE
config:
	NAME                                    STATE     READ WRITE CKSUM
	tank                                    ONLINE       0     0     0
	  59768b84-1aea-b649-82f8-e837a54be4ff  ONLINE       0     0     0
	spares
	  2e0105d0-f092-1f43-bb14-52d1e858116f  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:

Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Generally looks good to me, just few cosmetic nits.

lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
module/zfs/vdev_label.c Outdated Show resolved Hide resolved
module/zfs/vdev_label.c Outdated Show resolved Hide resolved
When spare or l2cache (aux) vdev is added during pool creation,
spa->spa_uberblock is not dumped until that point. Subsequently,
the aux label is never synchronized after its initial creation,
resulting in the uberblock label remaining undumped. The uberblock
is crucial for lib_blkid in identifying the ZFS partition type. To
address this issue, we now ensure sync of the uberblock label once
if it's not dumped initially.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
lib/libzutil/zutil_import.c Outdated Show resolved Hide resolved
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
@behlendorf behlendorf added Status: Code Review Needed Ready for review and testing Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jan 9, 2024
behlendorf pushed a commit that referenced this pull request Jan 16, 2024
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes #15737
behlendorf pushed a commit that referenced this pull request Jan 16, 2024
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes #15737
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
When spare or l2cache (aux) vdev is added during pool creation,
spa->spa_uberblock is not dumped until that point. Subsequently,
the aux label is never synchronized after its initial creation,
resulting in the uberblock label remaining undumped. The uberblock
is crucial for lib_blkid in identifying the ZFS partition type. To
address this issue, we now ensure sync of the uberblock label once
if it's not dumped initially.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 29, 2024
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
When spare or l2cache (aux) vdev is added during pool creation,
spa->spa_uberblock is not dumped until that point. Subsequently,
the aux label is never synchronized after its initial creation,
resulting in the uberblock label remaining undumped. The uberblock
is crucial for lib_blkid in identifying the ZFS partition type. To
address this issue, we now ensure sync of the uberblock label once
if it's not dumped initially.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes #15737
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes #15737
behlendorf pushed a commit that referenced this pull request Jan 29, 2024
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes #15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
When spare or l2cache (aux) vdev is added during pool creation,
spa->spa_uberblock is not dumped until that point. Subsequently,
the aux label is never synchronized after its initial creation,
resulting in the uberblock label remaining undumped. The uberblock
is crucial for lib_blkid in identifying the ZFS partition type. To
address this issue, we now ensure sync of the uberblock label once
if it's not dumped initially.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
When spare or l2cache (aux) vdev is added during pool creation,
spa->spa_uberblock is not dumped until that point. Subsequently,
the aux label is never synchronized after its initial creation,
resulting in the uberblock label remaining undumped. The uberblock
is crucial for lib_blkid in identifying the ZFS partition type. To
address this issue, we now ensure sync of the uberblock label once
if it's not dumped initially.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
Pool import logic uses vdev paths, so it makes sense to add path
information on AUX vdev as well.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Mar 13, 2024
If the AUX vdev is added using UUID, importing the pool falls back AUX
vdev to open it with disk name instead of UUID due to the absence of
path information for AUX vdevs. Since AUX label now have path
information, this PR adds path handling for it in `label_path`.

Reviewed-by: Umer Saleem <usaleem@ixsystems.com>
Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#15737
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.

None yet

4 participants