There were two problems with how libmpathpersist handled unregistering a key while holding the reseravation (which should also release the reservation). 1. If the path holding the reservation is not unregistered first, there will be unregistered paths, while a reservation is still held, which would cause IO to those paths to fail, when it shouldn't. 2. If the path that holds the reservation is down, libmpathpersist was not clearing the resrvation, since the there were no registered keys it could use for the PREEMPT command workaround
To fix these, libmpathpersist now releases the reservation first when trying to unregister a key that is holding the reservation. mpath_prout_rel() has a new option so that if it needs to self preempt to clear the reservation, it won't re-register the paths when called as part of unregistering a key. Also, instead of checking if the device is currently holding a reservation using mpp->reservation_key in check_holding_reservation() (which will already be set to 0 when called as part of unregistering a key), pass in the key to check. Signed-off-by: Benjamin Marzinski <[email protected]> --- libmpathpersist/mpath_persist_int.c | 101 +++++++++++++++++++--------- 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c index 9c71011f..f130f5f5 100644 --- a/libmpathpersist/mpath_persist_int.c +++ b/libmpathpersist/mpath_persist_int.c @@ -564,18 +564,23 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope, return status; } +enum preempt_work { + PREE_WORK_NONE, + PREE_WORK_REL, + PREE_WORK_REL_UNREG, +}; /* * Called to make a multipath device preempt its own reservation (and - * optionally release the reservation). Doing this causes the reservation - * keys to be removed from all the device paths except that path used to issue - * the preempt, so they need to be restored. To avoid the chance that IO - * goes to these paths when they don't have a registered key, the device - * is suspended before issuing the preemption, and the keys are reregistered - * before resuming it. + * optional extra work). Doing this causes the reservation keys to be removed + * from all the device paths except that path used to issue the preempt, so + * they may need to be restored. To avoid the chance that IO goes to these + * paths when they don't have a registered key and a reservation exists, the + * device is suspended before issuing the preemption, and the keys are + * reregistered (or the reservation is released) before resuming it. */ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key, int rq_servact, int rq_scope, unsigned int rq_type, - int noisy, bool do_release) + int noisy, enum preempt_work work) { int status, rel_status = MPATH_PR_SUCCESS; struct path *pp = NULL; @@ -596,7 +601,7 @@ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key, goto fail_resume; } - if (do_release) { + if (work != PREE_WORK_NONE) { memset(¶mp, 0, sizeof(paramp)); memcpy(paramp.key, &mpp->reservation_key, 8); rel_status = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REL_SA, @@ -604,15 +609,26 @@ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key, if (rel_status != MPATH_PR_SUCCESS) condlog(0, "%s: release on alternate path failed.", mpp->wwid); + else if (work == PREE_WORK_REL_UNREG) { + /* unregister the last path */ + rel_status = prout_do_scsi_ioctl(pp->dev, + MPATH_PROUT_REG_IGN_SA, + rq_scope, rq_type, + ¶mp, noisy); + if (rel_status != MPATH_PR_SUCCESS) + condlog(0, "%s: final self preempt unregister failed,", + mpp->wwid); + } + } + if (work != PREE_WORK_REL_UNREG) { + memset(¶mp, 0, sizeof(paramp)); + memcpy(paramp.sa_key, &mpp->reservation_key, 8); + status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope, + rq_type, ¶mp, noisy); + if (status != MPATH_PR_SUCCESS) + condlog(0, "%s: self preempt failed to reregister paths.", + mpp->wwid); } - - memset(¶mp, 0, sizeof(paramp)); - memcpy(paramp.sa_key, &mpp->reservation_key, 8); - status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope, - rq_type, ¶mp, noisy); - if (status != MPATH_PR_SUCCESS) - condlog(0, "%s: self preempt failed to reregister paths.", - mpp->wwid); fail_resume: if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, udev_flags)) { @@ -625,10 +641,10 @@ fail_resume: } static int preempt_self(struct multipath *mpp, int rq_servact, int rq_scope, - unsigned int rq_type, int noisy, bool do_release) + unsigned int rq_type, int noisy, enum preempt_work work) { return do_preempt_self(mpp, mpp->reservation_key, rq_servact, rq_scope, - rq_type, noisy, do_release); + rq_type, noisy, work); } static int preempt_all(struct multipath *mpp, int rq_servact, int rq_scope, @@ -636,7 +652,7 @@ static int preempt_all(struct multipath *mpp, int rq_servact, int rq_scope, { struct be64 zerokey = {0}; return do_preempt_self(mpp, zerokey, rq_servact, rq_scope, rq_type, - noisy, false); + noisy, PREE_WORK_NONE); } static int reservation_key_matches(struct multipath *mpp, uint8_t *key, @@ -661,18 +677,21 @@ static int reservation_key_matches(struct multipath *mpp, uint8_t *key, return YNU_NO; } -static bool check_holding_reservation(struct multipath *mpp, unsigned int *type) +static bool check_holding_reservation(struct multipath *mpp, uint8_t *curr_key, + unsigned int *type) { - if (get_be64(mpp->reservation_key) && + uint64_t zerokey = 0; + if (memcmp(curr_key, &zerokey, 8) != 0 && get_prhold(mpp->alias) == PR_SET && - reservation_key_matches(mpp, (uint8_t *)&mpp->reservation_key, type) == YNU_YES) + reservation_key_matches(mpp, curr_key, type) == YNU_YES) return true; return false; } -static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope, +static int mpath_prout_rel(struct multipath *mpp, int rq_servact, int rq_scope, unsigned int rq_type, - struct prout_param_descriptor * paramp, int noisy) + struct prout_param_descriptor *paramp, int noisy, + bool *unregistered) { int i, j; struct pathgroup *pgp = NULL; @@ -685,6 +704,8 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope, bool all_threads_failed; unsigned int res_type; + if (unregistered) + *unregistered = false; if (!mpp) return MPATH_PR_DMMP_ERROR; @@ -762,7 +783,8 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope, return status; } - if (!check_holding_reservation(mpp, &res_type)) { + if (!check_holding_reservation(mpp, (uint8_t *)&mpp->reservation_key, + &res_type)) { condlog(2, "%s: Releasing key not holding reservation.", mpp->wwid); return MPATH_PR_SUCCESS; } @@ -785,7 +807,10 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope, * 4. Reregistering keys on all the paths * 5. Resuming the device */ - return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy, true); + if (unregistered) + *unregistered = true; + return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy, + unregistered ? PREE_WORK_REL_UNREG : PREE_WORK_REL); } /* @@ -828,7 +853,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd, select_reservation_key(conf, mpp); put_multipath_config(conf); - memcpy(&oldkey, &mpp->reservation_key, 8); + oldkey = mpp->reservation_key; unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0); if (mpp->prkey_source == PRKEY_SOURCE_FILE && (rq_servact == MPATH_PROUT_REG_IGN_SA || @@ -905,7 +930,21 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd, { case MPATH_PROUT_REG_SA: case MPATH_PROUT_REG_IGN_SA: - ret= mpath_prout_reg(mpp, rq_servact, rq_scope, rq_type, paramp, noisy); + if (unregistering && check_holding_reservation(mpp, (uint8_t *)&oldkey, &rq_type)) { + bool unregistered = false; + struct be64 newkey = mpp->reservation_key; + /* temporarily restore reservation key */ + mpp->reservation_key = oldkey; + ret = mpath_prout_rel(mpp, MPATH_PROUT_REL_SA, rq_scope, + rq_type, paramp, noisy, + &unregistered); + mpp->reservation_key = newkey; + if (ret == MPATH_PR_SUCCESS && !unregistered) + ret = mpath_prout_reg(mpp, rq_servact, rq_scope, + rq_type, paramp, noisy); + } else + ret = mpath_prout_reg(mpp, rq_servact, rq_scope, + rq_type, paramp, noisy); break; case MPATH_PROUT_PREE_SA: case MPATH_PROUT_PREE_AB_SA: @@ -921,7 +960,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd, /* if we are preempting ourself */ if (memcmp(paramp->sa_key, paramp->key, 8) == 0) { ret = preempt_self(mpp, rq_servact, rq_scope, rq_type, - noisy, false); + noisy, PREE_WORK_NONE); break; } /* fallthrough */ @@ -932,14 +971,14 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd, paramp, noisy, NULL, &failed_paths); if (rq_servact == MPATH_PROUT_RES_SA && ret != MPATH_PR_SUCCESS && failed_paths && - check_holding_reservation(mpp, &res_type) && + check_holding_reservation(mpp, paramp->key, &res_type) && res_type == rq_type) /* The reserve failed, but multipathd says we hold it */ ret = MPATH_PR_SUCCESS; break; } case MPATH_PROUT_REL_SA: - ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy); + ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy, NULL); break; default: return MPATH_PR_OTHER; -- 2.50.1
