From 4d2dad04aaa436256ef756701aff07af824690c4 Mon Sep 17 00:00:00 2001 From: Ameer Hamza Date: Thu, 20 Jul 2023 21:57:16 +0500 Subject: [PATCH] Ignore pool ashift property during vdev attachment Ashift can be set for a vdev only during its creation, and the top-level vdev does not change when a vdev is attached or replaced. The ashift property should not be used during attachment, as it does not allow attaching/replacing a vdev if the pool's ashift property is increased after the existing vdev was created. Instead, we should be able to attach the vdev if the attached vdev can satisfy the ashift requirement with its parent. Reviewed-by: Brian Behlendorf Reviewed-by: Alexander Motin Signed-off-by: Ameer Hamza Closes #15061 --- include/sys/vdev_impl.h | 1 + module/zfs/vdev.c | 14 +++++--- .../cli_root/zpool_attach/attach-o_ashift.ksh | 30 ++++++----------- .../zpool_replace/replace-o_ashift.ksh | 32 +++++++------------ .../zpool_replace/replace_prop_ashift.ksh | 24 +++----------- 5 files changed, 36 insertions(+), 65 deletions(-) diff --git a/include/sys/vdev_impl.h b/include/sys/vdev_impl.h index 2b22b973ba..5f4e82ad86 100644 --- a/include/sys/vdev_impl.h +++ b/include/sys/vdev_impl.h @@ -420,6 +420,7 @@ struct vdev { boolean_t vdev_copy_uberblocks; /* post expand copy uberblocks */ boolean_t vdev_resilver_deferred; /* resilver deferred */ boolean_t vdev_kobj_flag; /* kobj event record */ + boolean_t vdev_attaching; /* vdev attach ashift handling */ vdev_queue_t vdev_queue; /* I/O deadline schedule queue */ spa_aux_vdev_t *vdev_aux; /* for l2cache and spares vdevs */ zio_t *vdev_probe_zio; /* root of current probe */ diff --git a/module/zfs/vdev.c b/module/zfs/vdev.c index 30551feb63..1199bf5d32 100644 --- a/module/zfs/vdev.c +++ b/module/zfs/vdev.c @@ -889,9 +889,15 @@ vdev_alloc(spa_t *spa, vdev_t **vdp, nvlist_t *nv, vdev_t *parent, uint_t id, &vd->vdev_not_present); /* - * Get the alignment requirement. + * Get the alignment requirement. Ignore pool ashift for vdev + * attach case. */ - (void) nvlist_lookup_uint64(nv, ZPOOL_CONFIG_ASHIFT, &vd->vdev_ashift); + if (alloctype != VDEV_ALLOC_ATTACH) { + (void) nvlist_lookup_uint64(nv, ZPOOL_CONFIG_ASHIFT, + &vd->vdev_ashift); + } else { + vd->vdev_attaching = B_TRUE; + } /* * Retrieve the vdev creation time. @@ -2144,9 +2150,9 @@ vdev_open(vdev_t *vd) return (SET_ERROR(EDOM)); } - if (vd->vdev_top == vd) { + if (vd->vdev_top == vd && vd->vdev_attaching == B_FALSE) vdev_ashift_optimize(vd); - } + vd->vdev_attaching = B_FALSE; } if (vd->vdev_ashift != 0 && (vd->vdev_ashift < ASHIFT_MIN || vd->vdev_ashift > ASHIFT_MAX)) { diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_attach/attach-o_ashift.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_attach/attach-o_ashift.ksh index 6ccec6abd6..574cb7654d 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_attach/attach-o_ashift.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_attach/attach-o_ashift.ksh @@ -35,7 +35,7 @@ # # STRATEGY: # 1. Create various pools with different ashift values. -# 2. Verify 'attach -o ashift=' works only with allowed values. +# 2. Verify 'attach' works. # verify_runnable "global" @@ -66,26 +66,14 @@ log_must set_tunable32 VDEV_FILE_PHYSICAL_ASHIFT 16 typeset ashifts=("9" "10" "11" "12" "13" "14" "15" "16") for ashift in ${ashifts[@]} do - for cmdval in ${ashifts[@]} - do - log_must zpool create -o ashift=$ashift $TESTPOOL1 $disk1 - log_must verify_ashift $disk1 $ashift - - # ashift_of(attached_disk) <= ashift_of(existing_vdev) - if [[ $cmdval -le $ashift ]] - then - log_must zpool attach -o ashift=$cmdval $TESTPOOL1 \ - $disk1 $disk2 - log_must verify_ashift $disk2 $ashift - else - log_mustnot zpool attach -o ashift=$cmdval $TESTPOOL1 \ - $disk1 $disk2 - fi - # clean things for the next run - log_must zpool destroy $TESTPOOL1 - log_must zpool labelclear $disk1 - log_must zpool labelclear $disk2 - done + log_must zpool create -o ashift=$ashift $TESTPOOL1 $disk1 + log_must verify_ashift $disk1 $ashift + log_must zpool attach $TESTPOOL1 $disk1 $disk2 + log_must verify_ashift $disk2 $ashift + # clean things for the next run + log_must zpool destroy $TESTPOOL1 + log_must zpool labelclear $disk1 + log_must zpool labelclear $disk2 done typeset badvals=("off" "on" "1" "8" "17" "1b" "ff" "-") diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace-o_ashift.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace-o_ashift.ksh index 37ed0062e6..9595e51241 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace-o_ashift.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace-o_ashift.ksh @@ -35,7 +35,7 @@ # # STRATEGY: # 1. Create various pools with different ashift values. -# 2. Verify 'replace -o ashift=' works only with allowed values. +# 2. Verify 'replace' works. # verify_runnable "global" @@ -66,26 +66,16 @@ log_must set_tunable32 VDEV_FILE_PHYSICAL_ASHIFT 16 typeset ashifts=("9" "10" "11" "12" "13" "14" "15" "16") for ashift in ${ashifts[@]} do - for cmdval in ${ashifts[@]} - do - log_must zpool create -o ashift=$ashift $TESTPOOL1 $disk1 - log_must verify_ashift $disk1 $ashift - # ashift_of(replacing_disk) <= ashift_of(existing_vdev) - if [[ $cmdval -le $ashift ]] - then - log_must zpool replace -o ashift=$cmdval $TESTPOOL1 \ - $disk1 $disk2 - log_must verify_ashift $disk2 $ashift - wait_replacing $TESTPOOL1 - else - log_mustnot zpool replace -o ashift=$cmdval $TESTPOOL1 \ - $disk1 $disk2 - fi - # clean things for the next run - log_must zpool destroy $TESTPOOL1 - log_must zpool labelclear $disk1 - log_must zpool labelclear $disk2 - done + log_must zpool create -o ashift=$ashift $TESTPOOL1 $disk1 + log_must verify_ashift $disk1 $ashift + # ashift_of(replacing_disk) <= ashift_of(existing_vdev) + log_must zpool replace $TESTPOOL1 $disk1 $disk2 + log_must verify_ashift $disk2 $ashift + wait_replacing $TESTPOOL1 + # clean things for the next run + log_must zpool destroy $TESTPOOL1 + log_must zpool labelclear $disk1 + log_must zpool labelclear $disk2 done typeset badvals=("off" "on" "1" "8" "17" "1b" "ff" "-") diff --git a/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace_prop_ashift.ksh b/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace_prop_ashift.ksh index ffdaf91a28..b4ac18e5ea 100755 --- a/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace_prop_ashift.ksh +++ b/tests/zfs-tests/tests/functional/cli_root/zpool_replace/replace_prop_ashift.ksh @@ -34,10 +34,8 @@ # # STRATEGY: # 1. Create a pool with default values. -# 2. Verify 'zpool replace' uses the ashift pool property value when -# replacing an existing device. -# 3. Verify the default ashift value can still be overridden by manually -# specifying '-o ashift=' from the command line. +# 2. Override the pool ashift property. +# 3. Verify 'zpool replace' works. # verify_runnable "global" @@ -72,21 +70,9 @@ do do log_must zpool create -o ashift=$ashift $TESTPOOL1 $disk1 log_must zpool set ashift=$pprop $TESTPOOL1 - # ashift_of(replacing_disk) <= ashift_of(existing_vdev) - if [[ $pprop -le $ashift ]] - then - log_must zpool replace $TESTPOOL1 $disk1 $disk2 - wait_replacing $TESTPOOL1 - log_must verify_ashift $disk2 $ashift - else - # cannot replace if pool prop ashift > vdev ashift - log_mustnot zpool replace $TESTPOOL1 $disk1 $disk2 - # verify we can override the pool prop value manually - log_must zpool replace -o ashift=$ashift $TESTPOOL1 \ - $disk1 $disk2 - wait_replacing $TESTPOOL1 - log_must verify_ashift $disk2 $ashift - fi + log_must zpool replace $TESTPOOL1 $disk1 $disk2 + wait_replacing $TESTPOOL1 + log_must verify_ashift $disk2 $ashift # clean things for the next run log_must zpool destroy $TESTPOOL1 log_must zpool labelclear $disk1