From 1c3d7dfb07443458fd0921b19975e55589affe0c Mon Sep 17 00:00:00 2001 From: David Teigland Date: Mon, 23 Sep 2024 14:46:29 -0500 Subject: [PATCH] lvmlockd: fix locking for thin lvremove of a thin lv while the pool is inactive would leave the pool locked but inactive. lvcreate of a thin snapshot while the pool is inactive would leave the pool locked but inactive. lvcreate of a thin lv could activate the pool to check a threshold before the pool lock was acquired in lvmlockd. --- lib/locking/lvmlockd.c | 11 ++++++--- lib/metadata/lv_manip.c | 51 +++++++++++++++++++++++++++++++++-------- 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/lib/locking/lvmlockd.c b/lib/locking/lvmlockd.c index 917aef518..6ea93d747 100644 --- a/lib/locking/lvmlockd.c +++ b/lib/locking/lvmlockd.c @@ -676,6 +676,10 @@ int handle_sanlock_lv(struct cmd_context *cmd, struct volume_group *vg) * Another host may have extended the lvmlock LV already. * Refresh so that we'll find the new space they added * when we search for new space. + * + * FIXME: we should be able to check if the lvmlock size + * in VG metadata is smaller than lvmlock size reported + * by the kernel, and avoid refresh if they match. */ if (!_refresh_sanlock_lv(cmd, vg)) return 0; @@ -2575,7 +2579,7 @@ int lockd_lv_name(struct cmd_context *cmd, struct volume_group *vg, } retry: - log_debug("lockd LV %s/%s mode %s uuid %s", vg->name, lv_name, mode, lv_uuid); + log_debug("lockd LV %s/%s mode %s uuid %s %s", vg->name, lv_name, mode, lv_uuid, opts ?: ""); /* Pass PV list for IDM lock type */ if (!strcmp(vg->lock_type, "idm")) { @@ -3178,7 +3182,7 @@ int lockd_init_lv(struct cmd_context *cmd, struct volume_group *vg, struct logic } else if (seg_is_thin(lp)) { if ((seg_is_thin_volume(lp) && !lp->create_pool) || - (!seg_is_thin_volume(lp) && lp->snapshot)) { + (!seg_is_thin_volume(lp) && lp->origin_name)) { struct lv_list *lvl; /* @@ -3186,12 +3190,13 @@ int lockd_init_lv(struct cmd_context *cmd, struct volume_group *vg, struct logic * their own lock but use the pool lock. If an lv does not * use its own lock, its lock_args is set to NULL. */ + log_debug("lockd_init_lv thin %s locking thin pool", display_lvname(lv)); if (!(lvl = find_lv_in_vg(vg, lp->pool_name))) { log_error("Failed to find thin pool %s/%s", vg->name, lp->pool_name); return 0; } - if (!lockd_lv(cmd, lvl->lv, "ex", LDLV_PERSISTENT)) { + if (!lockd_lv(cmd, lvl->lv, "ex", 0)) { log_error("Failed to lock thin pool %s/%s", vg->name, lp->pool_name); return 0; } diff --git a/lib/metadata/lv_manip.c b/lib/metadata/lv_manip.c index 5c78188fe..1dce4ba49 100644 --- a/lib/metadata/lv_manip.c +++ b/lib/metadata/lv_manip.c @@ -6887,6 +6887,10 @@ int lv_resize(struct cmd_context *cmd, struct logical_volume *lv, /* * If the LV is locked due to being active, this lock call is a no-op. * Otherwise, this acquires a transient lock on the lv (not PERSISTENT) + * FIXME: should probably use a persistent lock in case the command + * crashes while the lv is active, in which case we'd want the active + * lv to remain locked. This means then adding lockd_lv("un") at the + * end. */ if (!lockd_lv_resize(cmd, lv_top, "ex", 0, lp)) return_0; @@ -7549,6 +7553,7 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv, int visible, historical; struct logical_volume *pool_lv = NULL; struct logical_volume *lock_lv = lv; + struct logical_volume *lockd_pool = NULL; struct lv_segment *cache_seg = NULL; struct seg_list *sl; struct lv_segment *seg = first_seg(lv); @@ -7613,8 +7618,21 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv, return 0; } - if (!lockd_lv(cmd, lock_lv, "ex", LDLV_PERSISTENT)) - return_0; + if (vg_is_shared(vg)) { + if (lv_is_thin_type(lv)) { + /* FIXME: is this also needed for other types? */ + /* Thin is special because it needs to be active and locked to remove. */ + if (lv_is_thin_volume(lv)) + lockd_pool = first_seg(lv)->pool_lv; + else if (lv_is_thin_pool(lv)) + lockd_pool = lv; + if (!lockd_lv(cmd, lock_lv, "ex", LDLV_PERSISTENT)) + return_0; + } else { + if (!lockd_lv(cmd, lock_lv, "ex", 0)) + return_0; + } + } if (!lv_is_cache_vol(lv)) { if (!_lv_remove_check_in_use(lv, force)) @@ -7763,8 +7781,10 @@ int lv_remove_single(struct cmd_context *cmd, struct logical_volume *lv, display_lvname(pool_lv)); } - if (!lockd_lv(cmd, lv, "un", LDLV_PERSISTENT)) - log_warn("WARNING: Failed to unlock %s.", display_lvname(lv)); + if (lockd_pool && !thin_pool_is_active(lockd_pool)) { + if (!lockd_lv_name(cmd, vg, lockd_pool->name, &lockd_pool->lvid.id[1], lockd_pool->lock_args, "un", LDLV_PERSISTENT)) + log_warn("WARNING: Failed to unlock %s.", display_lvname(lockd_pool)); + } lockd_free_lv(cmd, vg, lv->name, &lv->lvid.id[1], lv->lock_args); if (!suppress_remove_message && (visible || historical)) { @@ -9243,6 +9263,11 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg, return_NULL; /* New pool is now inactive */ } else { + if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) { + log_error("Failed to lock thin pool."); + return NULL; + } + if (!activate_lv(cmd, pool_lv)) { log_error("Aborting. Failed to locally activate thin pool %s.", display_lvname(pool_lv)); @@ -9624,6 +9649,10 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg, /* Avoid multiple thin-pool activations in this case */ if (thin_pool_was_active < 0) thin_pool_was_active = 0; + if (!lockd_lv(cmd, pool_lv, "ex", LDLV_PERSISTENT)) { + log_error("Failed to lock thin pool."); + return NULL; + } if (!activate_lv(cmd, pool_lv)) { log_error("Failed to activate thin pool %s.", display_lvname(pool_lv)); @@ -9648,11 +9677,15 @@ static struct logical_volume *_lv_create_an_lv(struct volume_group *vg, } /* Restore inactive state if needed */ - if (!thin_pool_was_active && - !deactivate_lv(cmd, pool_lv)) { - log_error("Failed to deactivate thin pool %s.", - display_lvname(pool_lv)); - return NULL; + if (!thin_pool_was_active) { + if (!deactivate_lv(cmd, pool_lv)) { + log_error("Failed to deactivate thin pool %s.", display_lvname(pool_lv)); + return NULL; + } + if (!lockd_lv(cmd, pool_lv, "un", LDLV_PERSISTENT)) { + log_error("Failed to unlock thin pool."); + return NULL; + } } } else if (lp->snapshot) { lv->status |= LV_TEMPORARY;